encoding/protojson: serialization should forbid NaN and infinity for google.protobuf.Value.number_value

[dsnet]

The purpose of struct.proto is to provide an exact mapping of protobufs to raw JSON. The fact that the marshaler currently converts NaN and Infinity into strings means that it does not properly preserve properties of round-trip serialziation. As such, the serializer should reject these such values.

\cc @cybrcodr

[cybrcodr]

I think this is a design/spec issue that should be raised with the Protobuf team. I've pointed this out in their internal doc before. The reasons why protojson does not prevent marshaling out NaN or Infinity for Value.number_value are due to compatibility with C++ and Go V1 implementations.

C++ also unmarshals "NaN" and "Infinity" to Value.string_value, same as protojson.

[puellanivis]

Hard question. If we just keep doing a Wrong Thing just because everyone else is doing the same Wrong Thing, then no one will ever switch and do the Right Thing. 😟

[cybrcodr]

Sorry, I should have added more background explanation in my comment.

The developers guide mentions that the JSON representation for Value.number_value "Represents a double value." The guide also mentions for "float, double" --

"JSON value will be a number or one of the special string values "NaN", "Infinity", and "-Infinity". Either numbers or strings are accepted. Exponent notation is also accepted."

Hence both the C++ and the Go implementations are implemented that way. I agree that this presents a flaw in round-trip serialization. I don't think this is about a right vs wrong thing but more of a design/spec issue that needs to be brought up. The way I view it (subjective of course), the current implementation is following the spec.

[dsnet]

The design document for JSON serialization also says:

When converting a Value message to JSON, if the message has none of its fields set, it will be printed as null it will be an error. Previously the spec said that it will print as null, but this would cause Value to not round-trip correctly, as a Value message with null_value field set will also be printed as null.

While this is in regards to null, I think it communicates that the intention of the original design that it should preserve round-trip semantics.

[cybrcodr]

Yeah, I recall raising the issue regarding Value with unset fields last year and had the team made the changes but I'm not sure if the other languages made any changes to their implementations.

Anyways, I've filed a protobuf issue as I think it's better to have the Protobuf team agree on this before we make any changes here.

[puellanivis]

Ah yes, I forgot about the strings as special-case part of the spec.

Yeah, I agree that C/C++ and Go are probably on-spec at this time. As such, also agree that it’s definitely a ball entirely in protobuf’s court.