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

Resolve default values when using message getters #412

Closed
wants to merge 2 commits into from
Closed

Resolve default values when using message getters #412

wants to merge 2 commits into from

Conversation

kipricker
Copy link

What is accomplished in this PR:

  1. Resolve default values at access time using the field getters, promoting proper usage of accessors instead of accessing properties directly (I'd prefer if they were made "private" properties).
  2. Forward setter to MessagePrototype.set to allow proper handling of oneofs when using generated setters.
  3. If a message type field is unset, calling the field's getter will create and retain a new instance of the message. Mimicking the C++ .mutable_{field_name}() and Java .get{FieldName}() behaviors.
  4. Added a "has" field methods to check whether or not a field has been set or if it is still equivalent to it's default value.
  5. All tests pass without modification. These changes should be fairly non-invasive for existing projects.

@englercj
Copy link

I was looking at the logic for .get() and it seems like you are falling back to default value if the contained value is anything falsey. Would that mean if I have a uint32 field and I set 0, then use the getter that it would return the default value and not my 0? Or did I miss something?

@kipricker
Copy link
Author

You are correct. I'll fix it.

@englercj
Copy link

I don't remember off the top of my head how pb in other langs works, but I feel like I can assign null to a field and it will send null, even if there is a default value. I think default value only comes in if there is no user-set value of any kind. Do we have a way to track/check that?

@kipricker
Copy link
Author

If the field is unset or assigned to a default value, then on the wire the field is just omitted.

Setting a field to null directly should be considered incorrect and a method like clear should be used instead. Which, clear does not yet exist in Protobuf.js, though I have implemented it on my end. I plan to create a PR for that too in the near future.

For checking if the value is set, I only check if the value is null or the default value. The C++ implementation actually returns true even if the user sets the field to it's default value. It would be possible to do the same here by setting an additional flag on the message when the field is set using a setter, but I chose not to do this to continue support for using the dot accessor of the property. Though, as I previously commented, would prefer if direct access to the properties were limited to allow for more correct functionality of things like the has_ methods.

@englercj
Copy link

If the field is unset or assigned to a default value, then on the wire the field is just omitted.

Ah! Gotcha, thanks that was the missing part for me. Thanks for the info!

@kipricker
Copy link
Author

@englercj Here's a proof of concept for how we could make the hasBeenSet method accurately match the C++ and Java implementations. Which is, returns true if decoded as a non default value or if the value is set in any way by the user, even if its set to a default value. Then clearing the value using clear would reset the value back to null causing hasBeenSet to accurately return false.

I plan on cleaning this up a bit and actually obscuring the nullable properties on the message and then using defineProperty to reroute the dot accessors to the get and set prototype methods.

protobuf-has-poc.diff.txt

@seishun
Copy link
Contributor

seishun commented Apr 6, 2016

  1. In proto3, if a field was missing on the wire, ProtoBuf.js already puts the type-specific default value in the object. Fields are never null. "has" methods don't make sense for proto3.
  2. In proto2, if a field has an explicit default and populateDefaults is set to true, ProtoBuf.js already puts that default value in the object if it was missing on the wire. More importantly, your "has" methods will erroneously return true in this case.
  3. In proto2, If a field has an explicit default and populateDefaults is set to false, your getter will return the type-specific default value for missing fields instead of the explicitly specified default.

Basically, the only case I see where this PR is remotely useful is when using proto2 without explicit defaults to fall back to the default value (which you can already do by using e.g. msg.foo || 0). I don't see how it's in any way superior to #380.

promoting proper usage of accessors instead of accessing properties directly (I'd prefer if they were made "private" properties).

Please don't do that. Accessors are not idiomatic in JavaScript.

@kipricker kipricker closed this Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants