Decode option extensions from the descriptor set - #17
Merged
Conversation
getOptions previously decoded extensions two ways: reflection Range,
which only sees extensions already resolved via the global registry,
plus a hardcoded table (getKnownExtensions) mapping this repo's own test
extensions at field 20000 to a hand-rolled varint parser. The practical
result was that a consumer's extensions only decoded if they were
registered in the global type registry before parsing, and anything
non-varint or unregistered silently dropped.
Build an extension resolver from the request's own FileDescriptorProto
set (protodesc.NewFiles -> dynamicpb.NewTypes), thread it onto each
FileDescriptor, and re-decode each options message's bytes through
proto.UnmarshalOptions{Resolver} before enumerating extensions via
reflection. The resolver prefers the global registry and falls back to
the descriptor set, so consumers no longer need to register extensions
globally, while concrete registered types (e.g. google.api.http ->
*annotations.HttpRule) are still returned when available. If the set is
incomplete and protodesc.NewFiles fails, it degrades to the global
registry alone rather than crashing. The hardcoded table and custom
varint parser are removed.
Behavior change for OptionExtensions consumers: values now come from
protobuf reflection, so proto2 optional scalars are returned by value
rather than pointer. extend_file is now bool, not *bool; downstream type
assertions must drop the pointer.
There was a problem hiding this comment.
Pull request overview
This PR improves how protobuf option extensions are decoded by building an extension resolver from the CodeGeneratorRequest descriptor set and re-decoding options bytes with that resolver, removing the previous hardcoded “known extensions” table and varint-only unknown-field parser.
Changes:
- Build a combined extension resolver that prefers
protoregistry.GlobalTypesbut can fall back to types derived from the request’sFileDescriptorProtoset (protodesc.NewFiles→dynamicpb.NewTypes). - Re-decode options messages with
proto.UnmarshalOptions{Resolver}so extensions not globally registered can still be reflected and surfaced inOptionExtensions. - Update tests to reflect reflection-returned scalar semantics (e.g.
boolinstead of*bool) and addprotocto mise toolchain config.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| types.go | Adds resolver plumbing and replaces the legacy unknown-field extension parser with resolver-based re-decoding + reflection enumeration. |
| parser.go | Builds and threads an extension resolver derived from the request descriptor set into parsed file descriptors. |
| parser_test.go | Updates extended-options assertions to match new scalar return types (value vs pointer). |
| mise.toml | Adds protoc to the pinned tool list. |
| mise.lock | Locks protoc 35.1 artifacts/checksums for supported platforms. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sions Two follow-ups from the PR review. getOptions round-tripped every options message through Marshal/Unmarshal to promote descriptor-set extensions, even when there was nothing to promote. The message was already unmarshaled with the global registry, so only unregistered extensions ever sit in the unknown fields; the re-decode is now gated on their presence and messages without unregistered extensions skip the extra work entirely. The extension tests previously exercised only bool (varint) extensions, leaving the new resolver path unverified for other wire types and for message-typed extensions. extend.proto now also defines a string extension and a message-typed extension, both applied in booking.proto, and the test asserts the string decodes by value and the unregistered message decodes as a dynamic message whose fields remain readable via reflection. Regenerating the fixture set needs protoc and the jsonator example's generate step needs protoc-gen-go, so protoc-gen-go is now pinned in mise alongside protoc.
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
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.
getOptions previously decoded extensions two ways: reflection Range, which only sees extensions already resolved via the global registry, plus a hardcoded table (getKnownExtensions) mapping this repo's own test extensions at field 20000 to a hand-rolled varint parser. The practical result was that a consumer's extensions only decoded if they were registered in the global type registry before parsing, and anything non-varint or unregistered silently dropped.
Build an extension resolver from the request's own FileDescriptorProto set (protodesc.NewFiles -> dynamicpb.NewTypes), thread it onto each FileDescriptor, and re-decode each options message's bytes through proto.UnmarshalOptions{Resolver} before enumerating extensions via reflection. The resolver prefers the global registry and falls back to the descriptor set, so consumers no longer need to register extensions globally, while concrete registered types (e.g. google.api.http -> *annotations.HttpRule) are still returned when available. If the set is incomplete and protodesc.NewFiles fails, it degrades to the global registry alone rather than crashing. The hardcoded table and custom varint parser are removed.
Warning
Behavior change for OptionExtensions consumers: values now come from protobuf reflection, so proto2 optional scalars are returned by value rather than pointer. extend_file is now bool, not *bool; downstream type assertions must drop the pointer.