Incorrect field presence with null JSON values

[buraktabn]

syntax = "proto3";

message Property {
  ...
  optional int32 price = 3;
}

When I create dart files using protoc, I get the price field as follow;

@$pb.TagNumber(3)
$core.int get price => $_getIZ(2);
@$pb.TagNumber(3)
set price($core.int v) { $_setSignedInt32(2, v); }
@$pb.TagNumber(3)
$core.bool hasPrice() => $_has(2);
@$pb.TagNumber(3)
void clearPrice() => clearField(3);

Which is fine, If I need to check if the price is null I can use hasPrice() to see it's null. Well, that always returns true.
When we looked at the implentation of has method in field_set.dart, it makes sense to return always true

protobuf.dart/protobuf/lib/src/protobuf/field_set.dart

Lines 468 to 473 in cd0ff30

	bool _$has(int index) {
	var value = _values[index];
	if (value == null) return false;
	if (value is List) return value.isNotEmpty;
	return true;
	}

Since the price has a default value of 0, I can't null check it.

[osa1]

Using nullable types for optional fields is already discussed in #523 and other issues.

If I need to check if the price is null I can use hasPrice() to see it's null. Well, that always returns true.

I'm not sure what you mean by this, hazzer for the price field in your example does not always return true:

void main() {
  final p = Property();
  print(p.hasPrice()); // prints false
  print(p.hasField(3)); // prints false
  p.price = 0;
  print(p.hasPrice()); // prints true
  print(p.hasField(3)); // prints true
}

Why do you say it always returns true?

[buraktabn]

I'm using an SQL database and have to merge data to a proto object in order to send back a response using gRPC. I didn't think it was caused by mergeFromProto3Json and how it does the merge.

I believe the issue is that if the database returns a field null, it is replaced by the default value during the merge.

final pl = Property();
print(pl.toProto3Json()); // {}
print('price: ${pl.price}'); // price: 0
print('has price: ${pl.hasPrice()}'); // has price: false
pl.mergeFromProto3Json({'id': 10, 'price': null});
print(pl.toProto3Json()); // {id: 10, price: 0}
print('price: ${pl.price}'); // price: 0
print('has price: ${pl.hasPrice()}'); // has price: true

[daviddomkar]

Is there any reason to not use native dart null safety? The hasX() methods are error prone and very annoying to work with. In the code you cannot tell if the field is optional or not.

[osa1]

I believe the issue is that if the database returns a field null, it is replaced by the default value during the merge.

This is the bug. 'price': null is not a valid proto3 JSON encoding for the int32 prince = ... field, with or without optional. We should throw an exception in this case.

Is there any reason to not use native dart null safety? The hasX() methods are error prone and very annoying to work with. In the code you cannot tell if the field is optional or not.

The problem is it is too expensive for us to migrate users of this library after such an invasive change. Google stores most of its source code in a mono repo, and third-party dependencies (such as protobuf.dart) only have one version in the repo. So if we do a backwards incompatible change in protobuf.dart, we have to update all affected users in Google before merging it. For an invasive change like changing return type of a getter it will probably require refactoring a few hundred thousands line of code. We can't just bump the major version number and expect users in Google to update their protobuf dependency gradually.

This is also one of the main blockers for a lot of other bug fixes and improvements in this library.

[osa1]

@sigurdm points out this part in proto3 JSON spec:

If a value is missing in the JSON-encoded data or if its value is null, it will be interpreted as the appropriate default value when parsed into a protocol buffer

So it's correct to accept null as the value and set the field to the default value for the type.

I'm not sure what the presence check for the field should return though. I think it should return 'true' as the field is present in the message. Since the spec doesn't have proto3 optional fields yet it doesn't say what the presence checks should return when value of a field is null in proto3 JSON.

[osa1]

I talked to the protobuf team about this.

There is no spec that covers this case, but when we see a : null where a is optional we don't want to make a 'present', i.e. hasA should return false.

[daanporon]

The problem is it is too expensive for us to migrate users of this library after such an invasive change. Google stores most of its source code in a mono repo, and third-party dependencies (such as protobuf.dart) only have one version in the repo. So if we do a backwards incompatible change in protobuf.dart, we have to update all affected users in Google before merging it. For an invasive change like changing return type of a getter it will probably require refactoring a few hundred thousands line of code. We can't just bump the major version number and expect users in Google to update their protobuf dependency gradually.

Wouldn't it be possible to make if available via an experimental kind of attribute. Because now it's very dangerous to use the generated code in my opinion.

[osa1]

This bug should be fixed with #763.