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

APIv2: decide reset behavior of unmarshal #890

Open
dsnet opened this issue Jul 7, 2019 · 3 comments

Comments

Projects
None yet
2 participants
@dsnet
Copy link
Member

commented Jul 7, 2019

There are 3 unmarshal functions:

  • proto.Unmarshal
  • encoding/protojson.Unmarshal
  • encoding/prototext.Unmarshal

Currently, proto.Unmarshal does not reset the message when unmarshaling, while the later 2 do. I believe these should be consistent.

If the message is not reset, then there needs to be well-defined merge semantics for the serialization. Such semantics exist for wire and text serialization. However, JSON lacks any defined merge semantics and we should not invent any.

Given that JSON has no merge semantics, it seems to me that all 3 Unmarshal functions should default to reseting the message (to be consistent within our module). To provide merge-semantics, we should provide a AvoidReset option for wire and text unmarshaling.

\cc @neild @cybrcodr

@cybrcodr

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

V1 text unmarshaling does reset the message, while interestingly V1 JSON unmarshaling does not.

Having consistency in V2 would be nice, and I like your suggestion.

@dsnet dsnet removed the api-v2 label Jul 10, 2019

@dsnet

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

Something to investigate: The C++ JsonStringToMessage function takes in a *Message. Investigate whether they clear this message or merge into it. If it merges, then there is existing precedence of merge-semantics in JSON.

@cybrcodr

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

On head, JsonStringToMessage source shows it calls into JsonToBinaryString which constructs a serialized binary and then calls Message::ParseFromString which resets the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.