Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix required property serialization #98

Closed
allevato opened this issue Oct 15, 2016 · 8 comments
Closed

Fix required property serialization #98

allevato opened this issue Oct 15, 2016 · 8 comments
Assignees
Labels
kind/bug Feature doesn't work as expected.
Milestone

Comments

@allevato
Copy link
Collaborator

For proto2, we need to support two kinds of serialization/parsing:

  • Partial: Required fields are not "required." When writing a message, if a required field is unset, it is simply not written out. After reading a message, if a required field is not present, the partial message is returned.
  • Full: Required fields are required. When writing a message, if a required field is unset, an error is thrown. After reading a message, if a required field is not present, an error is thrown.

The typical pattern is to implement full in terms of partial. Implement an isInitialized method/property. For full writing, call it to validate before calling the partial write method (C++ example). For reading, do the partial read and then call it to validate (C++ example).

proto3 doesn't need to distinguish between partial and full, AFAIK, but we may want keep both to avoid surfacing different APIs (since much of this can be implemented in protocol extensions). For proto3, isInitialized could vacuously return true.

@thomasvl
Copy link
Collaborator

The Java and C++ have some pretty good smarts in the generators to make the code as minimal as possible. We might even be able to just make the default return true and only hook when checks are actually needed.

The serialization apis can also be done in terms of a generated partial api, so the basic impl checks valid before/after calling the partial api.

@thomasvl thomasvl added the kind/bug Feature doesn't work as expected. label Oct 18, 2016
@tbkka
Copy link
Collaborator

tbkka commented Nov 16, 2016

This could affect the API between the generated code and the library, so I've labelled it accordingly. I'd like to resolve all remaining such issues fairly soon.

@thomasvl thomasvl added this to the Version 1.0 milestone Feb 6, 2017
@thomasvl
Copy link
Collaborator

The C++ generator code for an optimized isInitialized - MessageGenerator::
GenerateIsInitialized
. The flow in there is pretty simple/logical. The one nice piece is the recursive check to see if message used for a field also has any required fields, to decided if a call to its isInitialized is needed or if it can be optimized out of the generate. The recursive check for required fields is: HasRequiredFields.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 13, 2017
- Add isInitialized Message protocol
- Provide default isInitialized so the generator can emit a custom one as needed.
- Add isInitialized to ExtensionFields so Message can call to it.
- Add Internal.swift as a place to hold public code (because the generated code
  has to be able to call it), but that is not intended to be part of the
  "developer public" api.
  - Helpers for checking a map and repeated field for being initialized.
- Generate isInitialized - Walk the message fields to decided if
  directly/indirectly can have messages with required fields.

Re apple#98
thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 13, 2017
- Add isInitialized Message protocol
- Provide default isInitialized so the generator can emit a custom one as needed.
- Add isInitialized to ExtensionFields so Message can call to it.
- Add Internal.swift as a place to hold public code (because the generated code
  has to be able to call it), but that is not intended to be part of the
  "developer public" api.
  - Helpers for checking a map and repeated field for being initialized.
- Generate isInitialized - Walk the message fields to decided if
  directly/indirectly can have messages with required fields.

Re apple#98
@thomasvl
Copy link
Collaborator

Once the isInitialized PR (#250) is in, we can move to support within the serialization apis. Glancing at Message.swift, it also shows we don't have a merge api like the other languages; so we likely want to add those and make the init methods call the merge methods. As far as naming, my thought would be Partial should go in the names that don't call isInitialized and the others should call the Partial method and then call isInitialized, but we likely want #12 done first since that will resolve some of the naming.

thomasvl added a commit to thomasvl/swift-protobuf that referenced this issue Feb 14, 2017
- Add isInitialized Message protocol
- Provide default isInitialized so the generator can emit a custom one as needed.
- Add isInitialized to ExtensionFields so Message can call to it.
- Add Internal.swift as a place to hold public code (because the generated code
  has to be able to call it), but that is not intended to be part of the
  "developer public" api.
  - Helpers for checking a map and repeated field for being initialized.
- Generate isInitialized - Walk the message fields to decided if
  directly/indirectly can have messages with required fields.

Re apple#98
@thomasvl thomasvl self-assigned this Feb 21, 2017
@thomasvl
Copy link
Collaborator

Now that the renaming is done, I'll look at adding the initialization checks into the binary serialization apis.

@tbkka
Copy link
Collaborator

tbkka commented Feb 21, 2017

FYI: I'm about to put up a big PR with a performance overhaul of the encoding side. (To match the work I did last week on the decoding side.)

thomasvl added a commit that referenced this issue Feb 22, 2017
Add partial support to the binary decoding initializer.

Progress on #98
@thomasvl
Copy link
Collaborator

PR #290 added the partial decode support.
PR #303 will add partial encoding support.

I'm rethinking adding any merge support at the moment, and thinking we'll hold off adding it until we have developers needing it.

@thomasvl
Copy link
Collaborator

PRs in, this should be complete.

PR #307 adds a merge method and it ended up being useful to clean up some tests.

ahmed-osama-saad pushed a commit to ahmed-osama-saad/swift-protobuf that referenced this issue Oct 12, 2023
…s-android

fix for crash on notifications when device gets disconnected
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Feature doesn't work as expected.
Projects
None yet
Development

No branches or pull requests

3 participants