fix(sr-normalize): reference inlined deps by their local name#175
Open
birdayz wants to merge 1 commit into
Open
fix(sr-normalize): reference inlined deps by their local name#175birdayz wants to merge 1 commit into
birdayz wants to merge 1 commit into
Conversation
The plugin inlines every non-WKT transitive dependency as a top-level type in the target's package, then references it by name. The naming logic stripped only the package prefix the referred type shared with the referring message. That works when everything lives in one package, and breaks the moment a dependency comes from another package: the emitted reference keeps the non-shared package segments while the type was inlined under its bare name, so the reference dangles and Schema Registry rejects the schema. Reference each type by its path within its own file instead: strip the type's own package, not the prefix it happens to share with the referrer. Top-level types resolve to their bare name, nested types to Parent.Nested. Well-known google/protobuf types stay imports and keep their full name. Same-package output is byte-identical, so existing schemas are unchanged. Add a cross-package example (example.other.v1.ProviderType referenced by example.v1.Event) with an SR round-trip test that registers the schema against a real Schema Registry, plus a unit test for the naming logic. The order/product .pb.go changes are a regeneration: their checked-in go_package string had been renamed textually from protoc-gen-go-sr to protoc-gen-go-sr-normalize without regenerating, leaving a stale descriptor length prefix that panics at package init. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
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.
What
Fix cross-package dependency references in
protoc-gen-go-sr-normalize. A message whose field type lives in a different proto package produced a schema with a dangling type reference that Schema Registry rejects. Same-package schemas are byte-identical to before.Why
The plugin builds a single self-contained schema by inlining every non-WKT transitive dependency as a top-level type in the target's package. The reference-naming logic stripped only the package prefix that the referred type shared with the referring message. That is fine while everything sits in one package, but wrong across packages.
Concrete case that motivated this:
redpanda.api.adp.v1alpha1.LLMProviderTypereferenced from a message inredpanda.adp.v1alpha1. The shared prefix is justredpanda., so the reference came out asapi.adp.v1alpha1.LLMProviderTypewhile the enum was inlined top-level asLLMProviderType. Dangling reference, schema rejected. The workaround so far was to duplicate the enum into the referring package, which is exactly what this makes unnecessary.Implementation details
Reference each type by its path within its own file: strip the type's own package prefix, not the prefix it happens to share with the referrer. A top-level type resolves to its bare name; a nested type to
Parent.Nested, matching how the generator renders inlined types (flattened into one package, nesting preserved). Well-knowngoogle/protobuf/*types are kept as imports, so they retain their full name.The change is contained to the naming path:
relativeName(shared-prefix stripping) is replaced bylocalTypeName(own-package stripping) behindschemaTypeName, which also handles the well-known case.New example proves the cross-package path end to end:
example.other.v1.ProviderType(a separate package) referenced by an annotatedexample.v1.Event.TestSchemaRegistry_EventRoundTripregisters the generated schema against a real Schema Registry and round-trips a message. A unit test pins the naming logic directly.The
order.pb.go/product.pb.godiffs are a regeneration, not a functional change. Their checked-ingo_packagehad been renamed textually fromprotoc-gen-go-srtoprotoc-gen-go-sr-normalizewithout regenerating, which left the descriptor's varint length prefix stale (87 vs the actual 97 bytes) and panics at package init. Regenerating corrects the descriptor; the.sr.pb.gooutputs are unchanged.References
Discovered while moving proto packages around, where the cross-package enum reference forced a duplicate enum definition.