Skip to content

to_dict() fails to write boolean fields without default when value is False #95

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

Open
priesgo opened this issue Jun 19, 2020 · 3 comments
Labels
enhancement New feature or request medium priority medium Medium effort issue, can fit in a single PR

Comments

@priesgo
Copy link

priesgo commented Jun 19, 2020

When you have a message with a boolean value as follows:

message Patient {
     bool isRnaAvailable = 1;
}

then an object like

patient = Patient()
patient.is_rna_available = np.random.choice([True, False], 1)[0]

fails to write the is_rna_available field when it is False when calling patient.to_dict(), this does not happen when the value is True.

The issue seems to be in line 824 of file betterproto/__init__py where it runs the following comparison elif v != self._get_field_default(field, meta) or include_default_values:. It should be something like 'elif v != None or include_default_values', otherwise when you set a field to the same value as the default value and do not set include_default values then it is not written. This may be also the case for other types.

@boukeversteegh
Copy link
Collaborator

I assume you have tried include_default_values=True, but it doesn't suite your use case?

If so, could you clarify why this option does not solve your problem?

Background

Protobuf does not distinguish values that are set to the default versus unset values, so betterproto follows the same behavior when calling to_dict. To include all values in the output, you can set include_default_values=True. This will also output fields not touched by the client, and present their default values.

If you are hinting at a version of to_dict that outputs only fields explicitly set by the client, we will need to track set/unset fields, which we currently don't do. A discussion about this was ongoing on slack at #unset-field-detection. I've made a new issue for this topic, please refer to #101 if its relevant.

@boukeversteegh boukeversteegh added enhancement New feature or request bug? Bug or feature? medium Medium effort issue, can fit in a single PR and removed enhancement New feature or request bug? Bug or feature? labels Jul 4, 2020
@boukeversteegh
Copy link
Collaborator

boukeversteegh commented Jul 4, 2020

I've looked at how the Dart implementation handles this:

On a client-side generated object, hasField returns only true when the field was set by the client, no matter the value.
Values are serialized when set, also when they are serialized with the default values. This is a bit of a surprise. Apparently, optimizing the serialization by omitting default values is not standard in protobuf.

message Position {
    int32 x = 1;
    int32 y = 2;
}
var p = Position();
print (p.writeToBuffer()); // prints []

p = Position()..x=0..y=0;
print (p.writeToBuffer()); // prints [8, 0, 16, 0]

Internally, null is stored for all fields by default. When accessing the getter, a default is returned when the internal value is null.

So we could probably do the same, if it works for dart 😃

@priesgo
Copy link
Author

priesgo commented Jul 9, 2020

Thanks for your answer !

include_default_values=True does not cover my use case. The problem is that to check when there is a value set in order to write it, it does not check for it being none, but instead for it to evaluate to true. In the case of a boolean field this fails to write when false, but other numeric types will also behave similarly by not writing 0 values.

This is important in my opinion as having a field being false or 0 is not the same as not having data for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request medium priority medium Medium effort issue, can fit in a single PR
Projects
None yet
Development

No branches or pull requests

2 participants