Null message fields should not be instantiated

[darioielardi]

Given, for example, the following proto definitions:

message Address {
  string street = 1;
}

message User {
  Address address = 1;
}

the current behavior, when decoding an incoming User message, is to initialize the Address generated class even if the field is unset, filling it with empty ( zero ) values, which should not happen.

This is crucial for a variety of situations, for example using value wrappers, which are Protobuf Well-Known Types and represents the Google solutions to distinct a scalar field zero value from its default value. This is actually not possible with the current dart implementation, because the wrapper generated class is instantiated anyway and the inner value becomes a zero value.

Also, I think this is what was referred in #245.

[CupNoodleFork]

same issue

[sigurdm]

You can use the hasFoo getters to know if a field is set or not.

I agree that the pre-initialized default values are kind of strange. I do not know the historic origins of those.
I'm just afraid it is too late to change this behavior without breaking too much code.

[yanivshaked]

Hi, any update on this issue?
This may cause huge amount of memory usage - which is not used in any case

[sigurdm]

To clarify a bit. The fields are not initialized if they are not present.

Instead you get a default value (created lazily) when you get an unset field.

[yanivshaked]

Thank you for your quick response!

Using the information you provided, I have changed :

  dynamic _$getND(int index) {
    var value = _values[index];
    if (value != null) return value;
    return _getDefault(_nonExtensionInfoByIndex(index));
  }

to:

  dynamic _$getND(int index) {
    var value = _values[index];
    return value;
  }

This solves the above issue. Instead of generating a ghost object on get, you return null.

Do you see any downsides for the above suggested change?

[yanivshaked]

@sigurdm Appreciate your option on the above suggestion

[osa1]

I think the right thing to do here is to use has... methods before reading the fields. In the original example, you should be using hasAddress before calling the address getter.

The proto3 spec says this about the default values of message fields:

For message fields, the field is not set. Its exact value is language-dependent. See the generated code guide for details.

So I think it's not necessarily a bug in the Dart implementation that a message field is initialized with some default when it's missing in an encoding as long as you can check if it was provided in the message. Here's a test using the proto definition in the bug report:

void main() {
  // Empty message
  {
    User user = User.fromBuffer(<int>[]);
    print(user.hasAddress()); // false
  }

  // address: {}
  {
    User user = User.fromBuffer(<int>[0x0a, 0x00]);
    print(user.hasAddress()); // true
  }
}

---

I will have to think more about @yanivshaked's suggestion, but I think it won't work. The problem is _$getND is used in files generated for proto2 message as well, but the suggested change only makes sense in proto3 message fields. So it will break code generated for proto2 messages.

In any case, we probably won't be able to change the current getters for message fields as it will break a lot of code. We could however provide an alternative that returns null for unset message fields in proto3 messages. (I will have to check what the ideal behavior here should be in proto2)

[rakudrama]

I think returning null plays very nicely with Dart's many facilities for working with nullable types (?., ??, promotion on testing etc), but this does require rethinking of the whole API. It is not something do be done without a comprehensive design that is subject to review. Fiddling with some methods to get an effect is not the way forward.

Regarding _$getND, it is an internal implementation detail.
There is no reason that proto2 and proto3 need to use the same method, or that stored field values are accessed via an index, or that null internally indicates a missing value. These are all implementation details that can change while providing the same API.

[osa1]

I just noticed this part in the proto3 spec default values section, right after the part I quoted in my previous comment:

Note that for scalar message fields, once a message is parsed there's no way of telling whether a field was explicitly set to the default value (for example whether a boolean was set to false) or just not set at all: you should bear this in mind when defining your message types

It seems like the spec assumes that existence of a field is indicated in the value of the field in the deserialized message and there are no hasX methods like the ones we provide in protobuf.dart. With hasX methods it's possible to tell whether a field was explicitly set or not, even when the field type is a scalar.

So when the spec says "For message fields, the field is not set. Its exact value is language-dependent" it doesn't mean (though they're not explicit about this) that we can use an arbitrary value for the message as the default, rather it means it should be indicated as null (e.g. in Java, Dart, ...), or Option/Maybe in languages with sum types and no null (e.g. Rust, OCaml, Haskell, ...), or nil in languages like Scheme, Lua, etc. the exact value is language dependent, but it's not a valid message value.

I still think what we have in protobuf.dart library should be good enough and this issue should not block anyone (just use has... method for your field), but if my reading of the spec, as described above, is correct, then returning null for missing fields with a message type should be more consistent with protobuf implementations in other languages and is more in line with what the spec says.

Maybe something to consider for the next major version bump.

[osa1]

Just came across this design document about field presence in proto3 which is saying the same thing I wrote in my previous comment above:

When serializing, fields with no presence are not serialized if they contain their default value.

• ...
• For messages, the default is the language-specific null value.

[osa1]

#628 is another manifestation of this issue.

Update: b/217596113 reports the same issue.