Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix protomodule.NewMessage handling of protobuf field presence #113

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

ryanpbrewster
Copy link
Contributor

@ryanpbrewster ryanpbrewster commented Feb 3, 2024

I'm running into an issue where going back and forth between protos and JSON is losing information.

Within the protobuf ecosystem, it's common to use google.protobuf.Value (https://protobuf.dev/reference/protobuf/google.protobuf/#value) to represent JSON types.

In such scenarios, what should

pb = proto.package("google.protobuf")
proto.decode_json(pb.Struct, '{"foo":""}')

return? Today, it returns an empty protobuf, with no information about the key "foo".

The underlying issue here is tracking field presence. I've left a few comments and links to the reference (here).

There was actually a TODO left in the code about this. In an attempt to minimize the potential impact, I've left the existing isFieldSet check and adding a specific carve-out for fields with explicit presence. If desired, I can remove that too — as noted in the previous TODO, Range should handle this automatically, and my testing bears this out.

	// Range iterates over every populated field in an undefined order,
	// calling f for each field descriptor and value encountered.
	// Range returns immediately if f returns false.
	// While iterating, mutating operations may only be performed
	// on the current field descriptor.

@CLAassistant
Copy link

CLAassistant commented Feb 3, 2024

CLA assistant check
All committers have signed the CLA.

dl-stripe
dl-stripe previously approved these changes Feb 8, 2024
@dl-stripe dl-stripe merged commit 985d3a7 into stripe:trunk Feb 9, 2024
4 checks passed
@ryanpbrewster ryanpbrewster deleted the rpb/proto_decode_json branch February 9, 2024 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants