Skip to content

Issue5799#6123

Merged
johanbrandhorst merged 12 commits intogrpc-ecosystem:mainfrom
rohitlohar45:issue5799
Dec 31, 2025
Merged

Issue5799#6123
johanbrandhorst merged 12 commits intogrpc-ecosystem:mainfrom
rohitlohar45:issue5799

Conversation

@rohitlohar45
Copy link
Copy Markdown
Contributor

Fixes #5799

References to other Issues or PRs

Have you read the Contributing Guidelines?

Yes

Brief description of what is fixed or changed

Prevents unnecessary package prefixes in OpenAPI definition names when using package or simple naming strategies.

Problem: The generator was adding package prefixes (e.g., foo.Foo) even when no actual naming collision existed in the generated spec. This happened because collision detection used ALL messages from the registry, including unreferenced imports.

Solution: Filter the name set before collision detection to only include:

  • All names from the current package
  • Names from other packages that are actually referenced in the spec
  • Standard google.* packages

This ensures only real collisions trigger prefixes while maintaining backward compatibility.

Example:

// foo.proto - references other.Other but NOT other.Foo
message Foo {
  other.Other other = 1;
}

Before: "definitions": { "foo.Foo": {...} }
After: "definitions": { "Foo": {...} }

Other comments

  • All existing tests pass
  • No breaking changes - only affects cases with phantom collisions from unreferenced imports
  • Tested with the reproduction case from the issue

@rohitlohar45
Copy link
Copy Markdown
Contributor Author

@johanbrandhorst can you have a look at this?

Copy link
Copy Markdown
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, sorry for the delay here, this looks great, thank you! Do you think you could add an example of this scenario happening to one of our existing example protobuf files, so that we can see the change this triggered? Thank you.

Copy link
Copy Markdown
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, meant to request changes.

@rohitlohar45
Copy link
Copy Markdown
Contributor Author

@johanbrandhorst , after doing make-generate, too many files are getting affected, do I need to all of these? I think they will be needed but not sure.

git status --short | head -20
M       examples/internal/clients/abe/.swagger-codegen/VERSION
M       examples/internal/clients/abe/api/swagger.yaml
M       examples/internal/clients/abe/api_a_bit_of_everything.go
M       examples/internal/clients/abe/api_camel_case_service_name.go
M       examples/internal/clients/abe/api_echo_rpc.go
M       examples/internal/clients/abe/api_snake_enum_service.go
M       examples/internal/clients/abe/client.go
M       examples/internal/clients/abe/configuration.go
M       examples/internal/clients/abe/model_a_bit_of_everything.go
M       examples/internal/clients/abe/model_a_bit_of_everything_1.go
M       examples/internal/clients/abe/model_a_bit_of_everything_2.go
M       examples/internal/clients/abe/model_a_bit_of_everything_3.go
M       examples/internal/clients/abe/model_a_bit_of_everything_4.go
M       examples/internal/clients/abe/model_a_bit_of_everything_nested.go
M       examples/internal/clients/abe/model_a_bit_of_everything_service_deep_path_echo_body.go
M       examples/internal/clients/abe/model_a_bit_of_everything_service_deep_path_echo_body_single_nested.go
M       examples/internal/clients/abe/model_a_bit_of_everything_service_post_with_empty_body_body.go
M       examples/internal/clients/abe/model_a_bit_of_everything_service_update_v2_body.go
M       examples/internal/clients/abe/model_examplepb_a_bit_of_everything.go
M       examples/internal/clients/abe/model_examplepb_a_bit_of_everything_repeated.go
M       examples/internal/clients/abe/model_examplepb_a_bit_of_everything_service_update_body.go
M       examples/internal/clients/abe/model_examplepb_bar.go
M       examples/internal/clients/abe/model_examplepb_body.go
M       examples/internal/clients/abe/model_examplepb_book.go
M       examples/internal/clients/abe/model_examplepb_check_status_response.go
M       examples/internal/clients/abe/model_examplepb_error_object.go
M       examples/internal/clients/abe/model_examplepb_error_response.go
M       examples/internal/clients/abe/model_examplepb_numeric_enum.go
M       examples/internal/clients/abe/model_examplepb_required_message_type_request.go
M       examples/internal/clients/abe/model_examplepb_snake_enum_response.go
M       examples/internal/clients/abe/model_examplepbsnake_case_0_enum.go
M       examples/internal/clients/abe/model_examplepbsnake_case_enum.go
M       examples/internal/clients/abe/model_message_path_enum_nested_path_enum.go
M       examples/internal/clients/abe/model_nested_deep_enum.go
M       examples/internal/clients/abe/model_oneofenum_example_enum.go
M       examples/internal/clients/abe/model_pathenum_path_enum.go
M       examples/internal/clients/abe/model_pathenumsnake_case_for_import.go
M       examples/internal/clients/abe/model_protoexamplepb_foo.go
M       examples/internal/clients/abe/model_sub_string_message.go
M       examples/internal/clients/abe/model_the_book_to_update_.go
M       examples/internal/clients/abe/response.go
M       examples/internal/clients/echo/.swagger-codegen/VERSION
M       examples/internal/clients/echo/api/swagger.yaml
M       examples/internal/clients/echo/api_echo_service.go
M       examples/internal/clients/echo/client.go
M       examples/internal/clients/echo/configuration.go
M       examples/internal/clients/echo/model_examplepb_dynamic_message.go
M       examples/internal/clients/echo/model_examplepb_dynamic_message_update.go
M       examples/internal/clients/echo/model_examplepb_embedded.go
M       examples/internal/clients/echo/model_examplepb_nested_message.go
M       examples/internal/clients/echo/model_examplepb_simple_message.go
M       examples/internal/clients/echo/model_protobuf_null_value.go
M       examples/internal/clients/echo/response.go
M       examples/internal/clients/generateunboundmethods/.swagger-codegen/VERSION
M       examples/internal/clients/generateunboundmethods/api/swagger.yaml
M       examples/internal/clients/generateunboundmethods/api_generate_unbound_methods_echo_service.go
M       examples/internal/clients/generateunboundmethods/client.go
M       examples/internal/clients/generateunboundmethods/configuration.go
M       examples/internal/clients/generateunboundmethods/docs/ExamplepbGenerateUnboundMethodsSimpleMessage.md
M       examples/internal/clients/generateunboundmethods/docs/GenerateUnboundMethodsEchoServiceApi.md
M       examples/internal/clients/generateunboundmethods model_examplepb_generate_unbound_methods_simple_message.go
M       examples/internal/clients/generateunboundmethods/model_rpc_status.go
M       examples/internal/clients/generateunboundmethods/response.go
M       examples/internal/clients/responsebody/.swagger-codegen/VERSION
M       examples/internal/clients/responsebody/api/swagger.yaml
M       examples/internal/clients/responsebody/api_response_body_service.go
M       examples/internal/clients/responsebody/client.go
M       examples/internal/clients/responsebody/configuration.go
M       examples/internal/clients/responsebody/docs/ExamplepbRepeatedResponseBodyOut.md
M       examples/internal/clients/responsebody/docs/ExamplepbRepeatedResponseBodyOutResponse.md
M       examples/internal/clients/responsebody/docs/ExamplepbRepeatedResponseStrings.md
M       examples/internal/clients/responsebody/docs/ExamplepbResponseBodyOut.md
M       examples/internal/clients/responsebody/docs/ExamplepbResponseBodyOutResponse.md
M       examples/internal/clients/responsebody/docs/ExamplepbResponseBodyValue.md
M       examples/internal/clients/responsebody/docs/ResponseBodyServiceApi.md
M       examples/internal/clients/responsebody/docs/ResponseResponseType.md
M       examples/internal/clients/responsebody/docs/StreamResultOfExamplepbResponseBodyOut.md
M       examples/internal/clients/responsebody/model_examplepb_repeated_response_body_out.go
M       examples/internal/clients/responsebody/model_examplepb_repeated_response_body_out_response.go
M       examples/internal/clients/responsebody/model_examplepb_repeated_response_strings.go
M       examples/internal/clients/responsebody/model_examplepb_response_body_out.go
M       examples/internal/clients/responsebody/model_examplepb_response_body_out_response.go
M       examples/internal/clients/responsebody/model_examplepb_response_body_value.go
M       examples/internal/clients/responsebody/model_response_response_type.go
M       examples/internal/clients/responsebody/model_stream_result_of_examplepb_response_body_out.go
M       examples/internal/clients/responsebody/response.go
M       examples/internal/clients/unannotatedecho/.swagger-codegen/VERSION
M       examples/internal/clients/unannotatedecho/api/swagger.yaml
M       examples/internal/clients/unannotatedecho/api_unannotated_echo_service.go
M       examples/internal/clients/unannotatedecho/client.go
M       examples/internal/clients/unannotatedecho/configuration.go
M       examples/internal/clients/unannotatedecho/model_examplepb_unannotated_embedded.go
M       examples/internal/clients/unannotatedecho/model_examplepb_unannotated_nested_message.go
M       examples/internal/clients/unannotatedecho/model_examplepb_unannotated_simple_message.go
M       examples/internal/clients/unannotatedecho/model_rpc_status.go
M       examples/internal/clients/unannotatedecho/response.go

@johanbrandhorst
Copy link
Copy Markdown
Collaborator

@johanbrandhorst , after doing make-generate, too many files are getting affected, do I need to all of these? I think they will be needed but not sure.

This looks like too many files, and also you can see that the generate job failed, which means some files were changed that shouldn't be. Did you try doing the generation inside the container? That should be the same environment that CI uses.

@rohitlohar45
Copy link
Copy Markdown
Contributor Author

@johanbrandhorst I believe the large number of changes is expected, but I might be mistaken, so I wanted to check with you.

From my understanding, adding the name-collision example (Status messages in sub/ and sub2/ imported by echo_service.proto) introduced real collisions in some swagger specs. The naming logic then added package prefixes to resolve those collisions, which is why a few *.swagger.json files and the generated Go swagger clients changed.

Files without collisions remain unchanged, and only the affected ones now use more specific names. I regenerated everything using the same CI container to stay consistent with CI.

Please let me know if you see this differently or if you think these changes should be handled another way.

Copy link
Copy Markdown
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think most of the changes seem to stem from this:

Standard google.* packages

Why do we need to special case these? It seems now instead of generating rpcStatus we generate googlerpcStatus. If we do need to special case it, can we capitalize the rpc to googleRpcStatus?

@rohitlohar45
Copy link
Copy Markdown
Contributor Author

The registriesSeen cache is global and shared across all proto files being processed. When we filter names for a specific file and overwrite this cache, any types filtered out at that stage are no longer available to later files. I might be concluding this incorrectly, but this is what I inferred from the test results.

The special-casing for google.* and grpc.* exists to ensure that standard types (such as google.protobuf.Any and google.rpc.Status) and runtime types (grpc.gateway.*) are always available for cross-file resolution.

Regarding googlerpcStatus:

I can update the legacy naming strategy to title-case intermediate package components. With this change:

  • google.rpc.Status would become googleRpcStatus (instead of googlerpcStatus)
  • [.a.b.C] would become aBC (instead of abC)

This change would require updating related tests, including TestResolveFullyQualifiedNameToOpenAPIName.

Please let me know if you’re okay with this approach or if you’d prefer a different solution.

@johanbrandhorst
Copy link
Copy Markdown
Collaborator

That sounds fine, please go ahead and make the change.

This reverts commit 554563d.
- Generated GooglerpcStatus models (replacing RpcStatus references)
- Added StatusCheckRequest/Response models for echo service
- Added protosub.Status and protosub2.Status models
- Updated API clients to reference googlerpcStatus definitions
- Regenerated using CI container to ensure consistency
Copy link
Copy Markdown
Collaborator

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

protoc-gen-openapiv2: Unnecessary package prefixes in definitions when no actual collision exists

2 participants