Replace jhump/protoreflect with Google's official protobuf library #123
Conversation
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
This is a major refactoring that replaces the jhump/protoreflect dependency with Google's official protobuf library (google.golang.org/protobuf). Changes: - Created internal/desc package that wraps protoreflect.* descriptors with an API compatible with jhump's desc package - Created internal/protoparse package using buf's protocompile for parsing - Replaced all imports of github.com/jhump/protoreflect/desc with github.com/aep-dev/api-linter/internal/desc - Replaced all imports of github.com/jhump/protoreflect/desc/protoparse with github.com/aep-dev/api-linter/internal/protoparse - Implemented all necessary descriptor methods (FileDescriptor, MessageDescriptor, FieldDescriptor, ServiceDescriptor, MethodDescriptor, EnumDescriptor, etc.) - Updated cmd/api-linter and cmd/buf-plugin-aep to use new packages - Project builds successfully with new dependencies Note: Some test files still use jhump/protoreflect/desc/builder for test infrastructure (constructing test descriptors). This is test-only code and doesn't affect production runtime. Files changed: 211
This completes the migration by removing all jhump/protoreflect
dependencies from test files:
- Replaced builder.FieldType with descriptorpb.FieldDescriptorProto_Type
in production code (common_lints.go, request_max_page_size_field.go,
request_skip_field.go)
- Fixed map[string]struct{} initializations to use {} instead of nil
- Converted test files to use proto string templates instead of builder
- Removed jhump/protoreflect from go.mod completely
The codebase now uses only Google's official protobuf library with zero
dependencies on jhump/protoreflect.
Created testutils helpers for lint package tests and fixed all test files to use the new Google protobuf library: - Added lint/testutils_test.go with builder-style helpers for test proto creation - Fixed lint test files to use new testutils (rule_test.go, rule_enabled_test.go, problem_test.go) - Updated rules/internal/testutils/problems_test.go to use ParseProto3String - Added timestamp import to support well-known types in tests - Fixed common_lints_test.go and resource_test.go to use descriptorpb types - Added WithStandardImports to protocompile for well-known types support - Partially implemented registry resolver for aep/api and google/api imports Test status: - lint package: all tests pass - rules/internal/testutils: all tests pass - rules/internal/utils: build error (registry resolver needs completion) - Other rule packages: need registry resolver to be completed The registry resolver implementation is in progress to handle imports of aep/api/* and google/api/* proto files that are registered but not available as source files.
This allows protocompile to resolve imports from the global proto registry using linker.NewFileRecursive to wrap FileDescriptors properly. This fixes compilation of test files that import registered proto files like google/protobuf/timestamp.proto. However, there is a remaining issue with extension type bindings: - Extensions from registry files (like aep/api/field_info.proto) are accessed as dynamic messages instead of typed Go structs - This causes tests that access proto extensions to panic - The issue is that protocompile-generated FileDescriptors don't share extension type registrations with the global registry Tests affected: - Most utils tests pass (ToUpperCamelCase, Lint SingularStringField, etc.) - Tests accessing aep/api extensions fail (TestLintRequiredField, etc.) - Approximately 20+ rule package tests affected This is a known limitation of using protocompile with pre-registered proto files that define extensions. Further investigation needed.
Comment on lines
+71
to
+72
| wrapped protoreflect.EnumDescriptor | ||
| file *FileDescriptor |
Member
There was a problem hiding this comment.
just OOC: does protoreflect not contain the filedescriptor?
Just trying to understand why it needs a wrapper type.
Member
toumorokoshi
left a comment
toumorokoshi
left a comment
There was a problem hiding this comment.
Left a quick review. I think this might be a pretty tricky refactor. Maybe we can try to get a beta out for others to test before publishing an official version.
Comment on lines
+152
to
+153
| func (md *MethodDescriptor) IsClientStreaming() bool { | ||
| return md.wrapped.IsStreamingClient() |
Member
There was a problem hiding this comment.
seems like the wrapper should be consistent with the naming of the wrapped object?
|
|
||
| // MessageDescriptor wraps a protoreflect.MessageDescriptor. | ||
| type MessageDescriptor struct { | ||
| wrapped protoreflect.MessageDescriptor |
Member
There was a problem hiding this comment.
any reason to name the wrapped component rather than just use it implicitly? then you can just re-use the functions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This is a major refactoring that replaces the jhump/protoreflect dependency with Google's official protobuf library (google.golang.org/protobuf).
Changes:
Note: Some test files still use jhump/protoreflect/desc/builder for test infrastructure (constructing test descriptors). This is test-only code and doesn't affect production runtime.
Files changed: 211