fix: include nested enums in parent message file instead of separate files#74
Merged
mjheilmann merged 1 commit intoelixir-grpc:mainfrom Mar 5, 2026
Merged
Conversation
3439a05 to
7fa5696
Compare
7fa5696 to
0dc0c0a
Compare
Contributor
Author
|
Since I also worked on it, |
mjheilmann
approved these changes
Mar 5, 2026
Collaborator
mjheilmann
left a comment
There was a problem hiding this comment.
LGTM, thanks for the related test coverage
Contributor
Author
|
@mjheilmann should the pkg take advantage of latest |
Collaborator
|
@yordis i don't see why not. Looking at https://endoflife.date/erlang and the warning from the google grpc dep the Elixir and OTP versions I pinned for development are stale too. While my employer uses this lib, I don't anymore, so I'm not as quick to find issues myself. If you see problems or useful improvements feel free to make PRs or even just create issues. It's not a problem at all for me, I welcome it. |
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.
Problem
When two messages in the same package each define a nested enum with the same local name (e.g. both
ListFoosRequestandListBarsRequesthave a nestedSortOrderenum), the builder was emitting a separate top-level synthetic file for each nested enum:pkg.ListFoosRequest.SortOrder.proto→ top-levelenum SortOrderpkg.ListBarsRequest.SortOrder.proto→ top-levelenum SortOrderBoth files are in the same package with the same top-level symbol (
SORT_ORDER_UNSPECIFIED). gRPC reflection clients that pass the resultingFileDescriptorSetthroughprotodesc.NewFiles()(e.g. k6) fail with:This was reported in grafana/k6#5712.
Root Cause
Util.get_nested_types/3only walkednested_type(nested messages) in aDescriptorProto, notenum_type(nested enums). So when the builder encountered a field referencing a nested enum, it did not recognise it as belonging to the parent message and created a standalone synthetic file for it instead.Fix
Extend
get_nested_types/3to also collectenum_typesymbols from theDescriptorProto. Nested enum symbols are now included in the "nested types" set, so the builder correctly keeps them inside their parent message's file.Tests
priv/protos/nested_enum_conflict.proto— a service with two messages in the same package each having a nestedSortOrderenum (the minimal repro case).does not produce top-level enum files for nested enums with the same nameinbuilder_test.exsthat asserts the nested enum files are not emitted as separate top-level files.