danielgtaylor / python-betterproto

Clean, modern, Python 3.6+ code generator & library for Protobuf 3 and async gRPC
MIT License
1.5k stars 210 forks source link

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

Open priesgo opened 4 years ago

priesgo commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.