jsonpb: unmarshal does not properly support null input values

[jeffrngu]

What version of protobuf and what language are you using?
Version: v1.5.2
Language: Golang

What did you do?
Unmarshal JSON message where input data contain key of type google.protobuf.NullValue and value set to "null"

.proto and test file can be found under https://github.com/jeffrngu/null_value

test JSON object:

{
"parameter": {
"key": "$param",
"null_value": null
}
}

What did you expect to see?
In the test driver, I expect both "key" and "null_value" to be in the un-marshaled object.
This is the behavior with v1.3.2.

What did you see instead?
Only"key" exists in the un-marshaled object

=== RUN TestNullValue
null_value_test.go:28: Failed to unmarshall foo/parameter/null_value: expected: KeyValue_NullValue; got:
--- FAIL: TestNullValue (0.00s)

FAIL

Process finished with exit code 1

Anything else we should know about your project / environment?
No

[dsnet]

This looks like a valid regression bug. Can you check with the newer protojson package what the behavior is?

[neild]

Works in protojson; I just tested it.

[jeffrngu]

Thanks @dsnet and @neild for the quick replies. I also confirmed that after updating my test driver to use the newer protojson package, the test case passed. Are you going to fix the issue in v1.5.2 though?
In the mean time, I'll look into migrating the code to use the newer package.

[neild]

Looks like this is broken in github.com/golang/protobuf/jsonpb v1.3.0 as well, so this is not a regression in the 1.4.0 jsonpb.

The jsonpb package has been superseded by google.golang.org/protobuf/encoding/protojson, which is a much more correct implementation of protobuf JSON serialization. As of github.com/golang/protobuf v1.4.0, the jsonpb package exists primarily to be a bug-compatible with older versions of the package. We're committed to fixing any incompatibilities between v1.4.0+ jsonpb and older versions of the the package, but generally not bugs which existed prior to v1.4.0. The problem is that since jsonpb doesn't follow the protobuf JSON specification, it is very difficult to distinguish between a bug and an unfortunate-but-supported behavior that should be preserved.

This specific case is probably safe to change, since the existing behavior is to incorrectly fail to parse the input, but it's probably not going to be high on the priority list.

[puellanivis]

Oh neat, there’s already a somewhat related comment about this bug in the code:

protobuf/jsonpb/decode.go

Lines 91 to 95 in ae97035

	// NOTE: For historical reasons, a top-level null is treated as a noop.
	// This is incorrect, but kept for compatibility.
	if string(raw) == "null" && mr.Descriptor().FullName() != "google.protobuf.Value" {
	return nil
	}

I’ve verified that the code already works as expected if the proto specifies google.protobuf.Value null_value = 3; but NullValue isn’t a message type, so the work arounds for Value to be null does not work for NullValue.

I isolated the buggy code to be here:

protobuf/jsonpb/decode.go

Lines 337 to 339 in ae97035

	if raw == nil || (string(raw) == "null" && !isSingularWellKnownValue(fd) && !isSingularJSONPBUnmarshaler(field, fd)) {
	continue
	}

All we should need to do is make isSingularWellKnownType return true for NullValue by testing fd.Enum().FullName() the same as we do for fd.Message().FullName().

[jeffrngu]

hi @puellanivis, when I debugged this, fd.Enum().FullName() actually returns "undefine" if i recall correctly. Maybe the fd didn't have Enum set correctly.

[puellanivis]

hi @puellanivis, when I debugged this, fd.Enum().FullName() actually returns "undefine" if i recall correctly. Maybe the fd didn't have Enum set correctly.

🤷‍♀️ When added it into isSingularWellKnownValue it passed the test.

[jeffrngu]

Thanks @puellanivis for confirming. Can you submit this fix for the upcoming release?

[puellanivis]

I have not jumped the hoops yet to contribute. 😬 I know they are not big hoops, but I’ve avoided it in order to avoid bureaucracy with my work place to explicitly authorize contributions.

[dsnet]

Thank you for being willing to contribute a fix. Before jumping in, I would wait for @neild to make a call for whether this bug meets the bar for a fix per his earlier comment: #1361 (comment)

[neild]

Since the fix is relatively simple and this is unlikely to affect backwards bug compatibility, SGTM.

[jeffrngu]

Thnx team for the quick fix!