stripe / skycfg

Skycfg is an extension library for the Starlark language that adds support for constructing Protocol Buffer messages.
Apache License 2.0
646 stars 54 forks source link

Fix protomodule.NewMessage handling of protobuf field presence #113

Closed ryanpbrewster closed 8 months ago

ryanpbrewster commented 9 months ago

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 commented 9 months ago

CLA assistant check
All committers have signed the CLA.