FieldMask in ProtoJSON fails to parse from "null"

[pananton13]

Hello! It looks like null-JSON (i.e. simply top-level "null") is not accepted by FieldMask parsing code https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/json/internal/parser.cc#L1001

This differs from other "well-known" types, for example, Timestamp/Duration, which accept null-JSON as a valid value and simply leave message default-initialized: https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/json/internal/parser.cc#L949 .

Can u please explain - is it intentional (and why)?

[pananton13]

One more question: looks like google.protobuf.Any conversion to JSON is broken a little. It looks like if I have protobuf message with all fields unset (i.e. it serializes as empty string), then such message will trigger an error when packed to Any and converted to JSON, unless legacy syntax is not turned on. Am I right ? Is it an expected behavior or a bug ?

https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/json/internal/unparser.cc#L750

[pananton13]

And one more)
There is no check here

protobuf/src/google/protobuf/json/internal/unparser_traits.h

Line 216 in e3699ac

(void)unerased->ParsePartialFromString(data);

, which allows to pass invalid binary data as Any payload which will be then somehow encoded to json.

[esrauchg]

It looks like if I have protobuf message with all fields unset (i.e. it serializes as empty string), then such message will trigger an error when packed to Any and converted to JSON, unless legacy syntax is not turned on. Am I right ? Is it an expected behavior or a bug ?

This is a bug, I will send a fix.

There is no check here [...] which allows to pass invalid binary data which will be then somehow encoded to json.

Can you give a github source link for which line you mean? You must mean something other than L240 on main.

This differs from other "well-known" types, for example, Timestamp/Duration, which accept null-JSON as a valid value and simply leave message default-initialized: https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/json/internal/parser.cc#L949 .

So, the Duration/etc cases are formally 'wrong' here. Null should never be accepted for an object and leave it default-initialized, instead something like {"some_duration": null} should be treated the same as {}, meaning that has_msg() must be false afterwards, as described here: https://protobuf.dev/programming-guides/json/#null-values

I suspect the listed conditional looks like its probably only reachable through here, behind the 'allow_legacy_nonconformant_behavior' flag (which I think is default-on unfortunately):

https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/json/internal/parser.cc#L1303

Timestamp/Duration behavior of ever default-initializing from a null is pretty weird and non-spec behavior. We may accept PR that makes FieldMask have parity behavior though, since I agree it is weird for one type to do it and not another in the same implementation, even though the behavior is officially 'wrong' if it ever happens.

[pananton13]

Can you give a github source link for which line you mean?

Oh, my bad. Here it is (this function is called when Any is serialized to JSON)

protobuf/src/google/protobuf/json/internal/unparser_traits.h

Line 216 in e3699ac

(void)unerased->ParsePartialFromString(data);

We may accept PR that makes FieldMask have parity behavior though
Not only FieldMask will require updates in this case, but at least Any too, and may be some more types. I have an idea (it is not very good though), that supporting null for Duration/Timestamp specifically was added to provide a way to specify 'default-initialized' duration/timestamp as a top-level object, because there is no other way to do this, except specifying something like "0s".

For example, default-initialized Any is easily represented as "{}", FieldMask as "", etc. But for duration/timestamp there is no such value: "{}" is not accepted by parsers (should be a string), "" is not a valid duration/timestamp.

I will think about the PR.

[pananton13]

Also one more thing to note. When serializing well-known type as an Any payload, the parser will not accept unknown fields besides "@type" and "value" in the json representation no matter 'ignore_unknown_fields' parse option is set or not https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/json/internal/parser.cc#L1335 . I found this a little inconsistent with other parsing code and choose to support this option in this case also in my ProtoJSON implementation. What do you think - is it a better approach or some strong reason exist to avoid this ?

[esrauchg]

Appreciate the feedback for this case 😃

that supporting null for Duration/Timestamp specifically was added to provide a way to specify 'default-initialized' duration/timestamp as a top-level object, because there is no other way to do this, except specifying something like "0s".

I think we just don't really intend to accept null as a top level object to parse for any object type as 'spec'.

I think the more defensible direction would instead that {} should always be accepted for all message types, by virtue that if WKT should be parsable under either their 'non-special-cased' representation or their 'special cased' representation, meaning that {} is always legal for any message, but also that e.g. a Duration should also accept {seconds:17, nanos: 456789} in addition to the special string representation.

However its a fringe topic and I don't think we have a consistent behavior in the ecosystem if its expected for not or parsers to accept WKT under "what the object would look like if it was not WKT". Unfortunately since we are unlikely to get the ecosystem implementations aligned on accepting that shape, I'm not sure that if declaring it as spec or not helps. Note that the same ecosystem babellification problem would also apply to top-level null too even if we aligned that it was a good idea.

When serializing well-known type as an Any payload, the parser will not accept unknown fields besides "@type" and "value" in the json representation no matter 'ignore_unknown_fields' parse option is set or not https://github.com/protocolbuffers/protobuf/blob/main/src/google/protobuf/json/internal/parser.cc#L1335

I can see the premise if the flag was viewed as 'best-effort parse, try not to parse-fail', but I think the current behavior actually makes sense under the premise that the reason you would want 'discard unknown fields' is really just to make it safe to add new fields to a preexisting message and have the parsers silently discard that new field until the deployed schemas can catch up.

When serializing well-known type as an Any payload, this situation will never occur: the WKT are really using the "value" field instead of anything else, there's no possibility of schema skew that another field in the Any would ever make sense for someone to have printed regardless of version/schema skew.

I can raise this one as a question internally after the New Year to see if others have a different consensus but I somewhat think this behavior makes sense as-is and we can document that into the spec for the 'ignore unknowns' flag with the justification for why this case is different.