feature: replace deprecated deps, rewrite to use bufbuild#7
feature: replace deprecated deps, rewrite to use bufbuild#7
Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
📝 WalkthroughWalkthroughSwitches registry from descriptor/protoparse to protoreflect/protocompile; public APIs now use protoreflect types; registry building uses protocompile.Compiler and protoresolve.Registry.FromFiles; go.mod updated to add github.com/bufbuild/protocompile and remove several indirect protobuf-related deps. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Compiler as protocompile.Compiler
participant Files as CompiledFiles
participant Resolver as protoresolve.Registry
participant ProtoReg as ProtoRegistry
Caller->>Compiler: instantiate Compiler with FileDescriptors
Caller->>Compiler: Compile()
Compiler-->>Files: return compiled FileDescriptors
Caller->>Resolver: FromFiles(Files...)
Resolver-->>ProtoReg: protoresolve.Registry with Files
ProtoReg->>ProtoReg: populate services map from fd.Services()
Caller->>ProtoReg: FindMethodByFullPath("Service/Method")
ProtoReg->>ProtoReg: lookup service by name -> method by name
alt method found
ProtoReg-->>Caller: protoreflect.MethodDescriptor
else not found
ProtoReg-->>Caller: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the protobuf registry implementation by replacing deprecated dependencies with maintained alternatives. The main change migrates from jhump/protoreflect v1 to bufbuild/protocompile for parsing proto files, while using the standard protoreflect types from google.golang.org/protobuf/reflect/protoreflect.
Key changes:
- Replaced deprecated
jhump/protoreflectv1 andprotoparsewithbufbuild/protocompilefor proto file compilation - Updated API to use standard protoreflect types (
protoreflect.ServiceDescriptorandprotoreflect.MethodDescriptor) - Simplified implementation by removing manual recursive dependency registration (now handled automatically by protocompile)
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| registry.go | Core migration: replaced parser with compiler, updated to use protoreflect v2 APIs, removed manual dependency registration logic |
| tests/protoreg_plugin_test.go | Added missing error assertion in test case for non-existent method lookup |
| go.mod | Updated dependencies: moved bufbuild/protocompile from indirect to direct, removed deprecated dependencies |
| go.sum | Updated checksums to reflect dependency changes |
| go.work.sum | Updated workspace checksums for new dependency versions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
registry.go (1)
74-87: Breaking change: method not found now returns (nil, nil) instead of error.Line 86 returns
service.Methods().ByName()directly, which returnsnilwhen the method doesn't exist. This creates inconsistent error handling:
- Service not found → returns error (lines 81-84)
- Method not found → returns (nil, nil) silently
This breaking change can cause issues for callers expecting an error when a method isn't found, potentially leading to nil pointer dereferences.
🔧 Proposed fix for consistent error handling
func (reg *ProtoRegistry) FindMethodByFullPath(method string) (protoreflect.MethodDescriptor, error) { parts := strings.Split(strings.TrimPrefix(method, "/"), "/") if len(parts) != 2 { return nil, errors.Errorf("Unexpected method") } service, ok := reg.services[parts[0]] if !ok { return nil, errors.Errorf("Service not found: %s", parts[0]) } - return service.Methods().ByName(protoreflect.Name(parts[1])), nil + methodDesc := service.Methods().ByName(protoreflect.Name(parts[1])) + if methodDesc == nil { + return nil, errors.Errorf("Method not found: %s", parts[1]) + } + return methodDesc, nil }
🤖 Fix all issues with AI agents
In @go.mod:
- Line 8: The go.mod dependency line "github.com/bufbuild/protocompile v0.14.1"
references a non-existent release; update that module version to a valid
released tag (e.g., change the version string to "v0.14.2") or the intended
correct tag, then run "go mod tidy" to verify and update go.sum accordingly so
the module resolves correctly.
In @tests/protoreg_plugin_test.go:
- Around line 48-50: The test was changed to expect no error from
registry.FindMethodByFullPath for unknown methods, reflecting the current
behavior of FindMethodByFullPath returning (nil, nil); if you revert
FindMethodByFullPath to return an error for missing methods, update
tests/protoreg_plugin_test.go to assert an error instead of NoError by replacing
the assert.NoError(t, err) with an assertion that err is non-nil and that the
returned method is nil (e.g., assert.Error(t, err); assert.Nil(t, unknown)),
referencing the call to registry.FindMethodByFullPath("service.v1.Test/Unknown")
and ensuring the test language matches the intended error semantics.
🧹 Nitpick comments (1)
registry.go (1)
36-37: Consider adding timeout to compilation context.Using
context.Background()without a timeout means compilation operations can run indefinitely. Consider usingcontext.WithTimeoutto bound execution time and provide better control over resource usage.⏱️ Proposed enhancement
- ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() fds, err := compiler.Compile(ctx, p.config.Files...)Don't forget to add
"time"to imports.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.modregistry.gotests/protoreg_plugin_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
registry.go (1)
plugin.go (1)
Plugin(29-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🔇 Additional comments (5)
registry.go (5)
3-12: LGTM! Imports correctly reflect the migration to protocompile.The imports have been properly updated to support the new bufbuild-based tooling.
14-18: Breaking API change: interface methods now return protoreflect types.The interface signature changes from descriptor-based to protoreflect-based types. This is a breaking change for consumers but is expected as part of the migration to bufbuild tooling.
20-23: LGTM! Struct field type correctly updated.The
servicesfield type properly reflects the migration to protoreflect-based descriptors.
42-54: LGTM! File registration and service collection correctly implemented.The migration to
protoregistry.Filesand collection of services from file descriptors is properly structured.
69-72: LGTM! Services method correctly returns the descriptor map.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
1 participant
Reason for This PR
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Chores
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.