APIv2: handling of nil map values

[neild]

The v1 API permits nil values in message-valued maps. For example:

m.SomeMapField = map[string]*somepb.Message{
  "key": nil,
}

The v1 API will:

1. Marshal this to wire encoding as a map entry message with no value field.
2. Not complain about unset required fields in the map value.
3. Unmarshal a map entry message with no value field back to this form. That is, the nil-ness is preserved across marshal/unmarshal operations.

The v2 API will currently:

1. Marshal this to wire encoding with an empty, but set, value field.
2. Complain about unset required fields in the map value.
3. Never unmarshal a map entry message back to this form.

The C++ proto API makes no distinction between empty and absent map values. A nil map value will not be preserved if it passes through a C++ unmarshal/marshal layer.

We need to either deliberately choose to drop support for nil messages as map values (as distinct from empty ones--you can have a nil value, it's just no different from an empty &somepb.Message{}), or decide that this would be too hazardous a change and preserve the v1 semantics in the v2 API.

[dsnet]

I vote for the C++ semantics. Doing otherwise will complicate our API for a dubious protobuf semantic. However, we should gather data on what (if anything) depends on this.

[dsnet]

Related: #965

[puellanivis]

C++ semantics seems more reasonable to me, and that the current v1 and v2 behavior are just emergent behavior of the library.

Although, now that I think about it, you say, “We need to either deliberately choose…” which totally says the current behavior is emergent. 😆

If we were to make a deliberate choice, let us then choose the clearest alignment with protobuf semantics: a nil message just does not exist.

[neild]

Global testing turns up three failures in Google.

Two are tests explicitly checking behavior when a map value is nil. So far as I can tell, neither expects this to occur in practice.

One is treating a nil message value as indicating a default, but can be easily updated to treat an empty message in the same way. Since there's currently no way for a C++ client to indicate a default, that seems like a plain bugfix. (I assume there are no C++ clients to this service.)

I think this argues for the C++ semantics.

[neild]

Final decision is to retain the new, C++-compatible semantics: A nil value in a map is treated the same as an empty, non-nil value.

[tandr]

Please reconsider.

It is breaking the notion of "wire protocol", making it "we send/receive something that C++ thinks it is right".

The definition of "wire protocol" or "data transfer/storage protocol" (IMHO) is to preserve whatever got mangled in no matter how wrong it was on 2nd level rules. Most important part - it de-serializes your data back to whatever original was. Rinse-repeat will bring exactly the same result as original was.

With this change above is no longer true. As much as Google internal use covers, it is still does not cover everything. Including bugs, assumptions and historical reasons in outside of Google world. We use proto for storage of data, and I dread the moment when old data deserialized with some values that were set to nil will all of a sudden have "empty" structs attached to them.

To me, if protocol format description document allows it - it is legal. (If protocol allows dropping of required fields - so be it. I guess that's why there are no required fields anymore, but wire encoding did not change - it always allowed it. Encoders were enforcing it.)

Key-Value maps (that used in serialization of maps and lists) in protocol buffers described that Key kind of MUST be something (because nil keys are not allowed), but is "Value" part described as "MUST present"? I don't think it is even possible with all fields being optional in proto3

One more thing - I think you will have to support "bad" maps anyway because
a) wire compatibility with v1-v2 of protobuf itself;
b) old clients/other languages/other implementations;
c) old (current) proto library does it. Communication between old clients and new servers (or wise-versa) should go on, am I right?

Thank you very much for your time.

[dsnet]

To me, if protocol format description document allows it - it is legal. (If protocol allows dropping of required fields - so be it. I guess that's why there are no required fields anymore, but wire encoding did not change - it always allowed it. Encoders were enforcing it.)

The v2 behavior strictly adheres to the protobuf data model. It was a bug in the v1 implementation that transient information not in the protobuf data model was accidentally leaking to/from the wire format.

One more thing - I think you will have to support "bad" maps anyway because...

The Go protobuf compatibility agreement reserves the right to fix bugs. This change in behavior is to fix buggy behavior in v1 to be compliant with the protobuf data model.

That being said, I understand that this change could potentially be problematic for existing data that is written to disk. If so, we would need to understand whether this is a real problem or just a hypothetical. And if real, how often it happens. Supporting the incorrect behavior is costly as well because it causes incompatibility with the other language implementations.

[puellanivis]

I guess that's why there are no required fields anymore,…

No, an author of a widely accepted document “required considered harmful” argued quite successfully that it a) served no good purpose that was not better fulfilled by other means, and b) marking a field as “required” was sticky, i.e. once you apply it, there is no safe and forward-and-backward-compatible way remove it. So there were lots of protobufs out there shoving dummy values into these fields, because they were marked “required” but lost their purpose, and were no longer used.

Basically, there was nothing about the “required” feature that had any redeeming value. So, by the time proto3 was being defined, it was decided to drop the feature entirely.

But more specifically:

If protocol allows dropping of required fields…

The protocol never allowed dropping of required fields. That one could produce an invalid wire-encoded message that was missing a required field is irrelevant, the protocol is a full set of agreements on behavior and handlings, not just a wife format; and that protocol always required that required fields appear.

[tandr]

@dsnet

That being said, I understand that this change could potentially be problematic for existing data that is written to disk.

Problematic? I see it as "devastating" - data that was serialized (maps with nil values) all of a sudden becomes maps with empty values. It is not the same data anymore.

[dsnet]

Are you speaking from experience regarding a real problem or is this hypothetical?

It is not the same data anymore.

The generated Go code is not a 1:1 match to the protobuf data model. There are other ways where you can express things in the Go data structures that cannot be represented in the protobuf data model. One simply cannot expect that a round-trip marshal/unmarshal reproduces the exact same Go data structure. This is a false expectation and never documented as being valid.

[meling]

@dsnet

That being said, I understand that this change could potentially be problematic for existing data that is written to disk.

Problematic? I see it as "devastating" - data that was serialized (maps with nil values) all of a sudden becomes maps with empty values. It is not the same data anymore.

(I have no skin in the game, but...) Still seems hypothetical. Would be nice to have more data to back this up, and whether or not real code actually misbehave as a result; do the tests still pass? Also, remember v2 is a breaking change anyway. Now is the time to fix this.

[tandr]

Are you speaking from experience regarding a real problem or is this hypothetical?

I would need to dive in in (that I know of) 3 different projects that are using protobuf for serialization/storage here. I can do it for 1 (will take at least a day), 2 others are out of my reach and level of understanding of how their data is stored and used. I am not sure how to quantify that time with my manager, I think I will be asked (again) why did we choose protobuf for serialization if it is not deemed stable.

One simply cannot expect that a round-trip marshal/unmarshal reproduces the exact same Go data structure.

I, that that "one", am expecting exactly that from serialization format. Especially when I am using same language, same library.

[tandr]

Now is the time to fix this.

Thank you @meling. You made me thinking - maybe it is a time to change the spec to what really happens in a wild and allow nil values for the maps? (But it that would be impossible if Java or C++ libs are using something that effectively prohibits that, like some home-grown map classes)