protomodule: return error instead of panicking on non-message args to proto.merge#149
Open
LeSingh1 wants to merge 1 commit into
Open
protomodule: return error instead of panicking on non-message args to proto.merge#149LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
… proto.merge proto.merge used unchecked type assertions on its arguments, which would panic if either was not a *protoMessage. Since proto.merge is a builtin exposed to user Starlark code, this means any config could crash the host program with a Go panic instead of producing a Starlark error. Replace the assertions with checked conversions that return a regular error matching the style of proto.decode_json and proto.decode_text.
|
|
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The proto.merge builtin used unchecked type assertions on its two arguments, so passing any non-proto-message value from Starlark code would panic the host Go program instead of producing a regular Starlark error. Since proto.merge is exposed directly to user configs, this is reachable from any config that calls it with the wrong type, for example by accident or while iterating on a script.
The fix replaces the assertions with checked conversions that return a normal error, matching the style already used in proto.decode_json, proto.decode_text, and wantSingleProtoMessage in the same file. The error format mirrors those callers ("proto.merge: for parameter N: got X, want proto.Message") so it slots in with the existing error vocabulary.
Added a TestProtoMergeNonMessage test that exercises both argument positions with a Starlark string and verifies the expected error text. The existing TestProtoMergeDiffTypes only covered the case where both args were proto messages of different types, so this new test fills the gap on the non-message path.
Verified with go build ./... and go test on the packages that build outside of Bazel locally. The protomodule unit tests need the bazel-generated test protos to compile, so the new test was written to match the existing TestProtoMergeDiffTypes pattern that CI already runs.