-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
[Protobuf] Fix Discriminator Issue and add capability Enum Extraction #22740
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
[Protobuf] Fix Discriminator Issue and add capability Enum Extraction #22740
Conversation
This PR fixes a critical bug in the protobuf schema generator where models using discriminators with �llOf composition were generating invalid import paths when child schemas contained references to other models.
* Improve management of inheritance * Improve management of discriminator * Allow to separate inline enums in external files * Add unit test
…m/Ocsidot/openapi-generator into fix/protobuf-discriminator-issues
Fix issue linked to enum in array when there is inheritance or discriminator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 11 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="modules/openapi-generator/src/test/resources/3_0/protobuf-schema/pet.proto">
<violation number="1" location="modules/openapi-generator/src/test/resources/3_0/protobuf-schema/pet.proto:15">
P1: Imported proto file `models/characteristics.proto` is missing, leaving `Characteristics` unresolved and making the schema uncompilable.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| package openapitools; | ||
|
|
||
| import public "models/characteristics.proto"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Imported proto file models/characteristics.proto is missing, leaving Characteristics unresolved and making the schema uncompilable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/resources/3_0/protobuf-schema/pet.proto, line 15:
<comment>Imported proto file `models/characteristics.proto` is missing, leaving `Characteristics` unresolved and making the schema uncompilable.</comment>
<file context>
@@ -12,14 +12,20 @@ syntax = "proto3";
package openapitools;
+import public "models/characteristics.proto";
+
message Pet {
</file context>
|
Thanks for the PR. Is
|
Hello @wing328, good point. I took some reflection time on this (I've missied this feature at first). I did some test runs to check it and here are my conclusions While both features deal with enums, Technical Comparison
# Before RESOLVE_INLINE_ENUMS
Property: { enum: [A, B, C] }
# After RESOLVE_INLINE_ENUMS
Property: { $ref: '#/components/schemas/Property_inline_enum' }
components/schemas:
Property_inline_enum: { enum: [A, B, C] }
// Generated file: model_a_status.proto
message ModelA_Status {
enum Enum {
UNSPECIFIED = 0;
ACTIVE = 1;
INACTIVE = 2;
}
}
// In model_a.proto
import public "models/model_a_status.proto";
message ModelA {
ModelA_Status.Enum status = 1;
}
The Critical Difference: Message WrappersThe key protobuf-specific innovation in property.vendorExtensions.put("x-protobuf-data-type", enumTypeName + "." + ENUM_WRAPPER_INNER_NAME);
// Results in: ModelA_Status.EnumThis pattern prevents enum value collisions in protobuf's global namespace - a problem Real-World Problem This PR SolvesWithout ModelA:
properties:
status: { enum: [ACTIVE, INACTIVE] }
ModelB:
properties:
status: { enum: [ACTIVE, PENDING] }Both would generate With this PR's solution:
Additional Features in This PRThe new code also fixes critical issues that
I can provide a sample test to verify the behavior of both parameters (I did it to check if I was not missing a point) Relationship Between FeaturesThese features are complementary, not redundant: You can use both together or independently. They solve different problems at different layers. Conclusion
Let me know if you'd like me to clarify any specific aspect! |
Personally I would avoid the overhead in maintaining a specified option in protobuf generator that can potentially be done by RESOLVE_INLINE_ENUMS in inline model resolver but it's entirely your choice. I'm with this option. Just want to make sure you aware of the inline schema resolver option that's already available. cc @lucy66hw @andrewwilsonnew as well to see if they've opinions on this |
Overview
This PR resolves critical a critical issue in the Protobuf schema code generator related to discriminator handling.
Following this fix, during our investigation we also found another issue related on enum management: without extrating enum in separated messages, if enums have similar values, a conflict appears. Thus, we propose to add a new parameter to allow enum extraction, and proper support for enums within array types. The changes ensure correct protobuf generation when models use inheritance, discriminators, and inline/extracted enums.
Issues Fixed
This PR addresses multiple related issues in protobuf code generation:
extractEnumsToSeparateFilesoptionChanges Made
1. Improved Protobuf Generation Core
2. Fixed Enum Value Collision
3. Added Array of Enums Support
property.isArray && property.items.isEnum)ParentModel_FieldName.Enum) to array items4. Fixed Enum Extraction in Complex Scenarios
.EnumsuffixEnum Extraction Pattern
When
extractEnumsToSeparateFilesis enabled, the generator now properly wraps inline enums to prevent naming collisions:Testing
ProtobufSchemaCodegenTestFiles Modified
modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/ProtobufSchemaCodegen.javamodules/openapi-generator/src/test/java/org/openapitools/codegen/languages/protobuf/ProtobufSchemaCodegenTest.javaBackwards Compatibility
✅ Maintains full backwards compatibility:
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)Summary by cubic
Fixes discriminator handling and import paths in the Protobuf schema generator, and adds an option to extract enums to separate files with full support for arrays. This prevents enum name collisions and generates valid schemas for models using inheritance and discriminators.
Bug Fixes
New Features
Written for commit b94931e. Summary will update on new commits.