-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix(swagger): Fix logic error in POST JSON parameter validation. #4970
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?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files🚀 New features to boost your workflow:
|
… enum value parsing Correctly handle $ref references for composite types, including structs, pointers, arrays, and map types. Optimize the enum value parsing logic to support mixed enums of boolean and string types.
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.
Pull Request Overview
This PR ensures that Swagger definitions are correctly referenced when UseDefinitions is enabled, fixes POST JSON parameter detection, and updates enum option parsing.
- Add
$refsupport for array item types initemsFromGoTypewhen using definitions - Refactor
propertiesFromTypeto delegate to a newbuildSchemaWithDefinitionshelper - Correct
isPostJsonlogic for POST methods and overhaul enum parsing inenumsValueFromOptions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/goctl/api/swagger/swagger.go | Added handling for array types referencing definitions in itemsFromGoType |
| tools/goctl/api/swagger/properties.go | Introduced buildSchemaWithDefinitions and updated propertiesFromType to use it |
| tools/goctl/api/swagger/parameter.go | Fixed inverted check in isPostJson to correctly detect POST requests |
| tools/goctl/api/swagger/options.go | Refactored enumsValueFromOptions to parse "options=" flag values |
Comments suppressed due to low confidence (1)
tools/goctl/api/swagger/properties.go:53
- This new
UseDefinitionsbranch in propertiesFromType covers multiple composite types but lacks dedicated tests. Please add unit tests for struct, array, and map definitions scenarios to verify correct$refgeneration.
if ctx.UseDefinitions {
| return nil | ||
| } | ||
|
|
||
| if ctx.UseDefinitions { |
Copilot
AI
Jun 29, 2025
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.
The logic for handling struct references in itemsFromGoType duplicates similar code in buildSchemaWithDefinitions. Consider extracting a shared helper to reduce duplication and improve maintainability.
| }) | ||
| for _, field := range fields { | ||
| resp = append(resp, field) | ||
| if strings.HasPrefix(option, "options=") { |
Copilot
AI
Jun 29, 2025
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.
[nitpick] Hardcoding the prefix "options=" instead of using the existing enumFlag constant may cause inconsistency. Consider using enumFlag for matching the enum option.
|
| enums = append(enums, true) | ||
| } else if enum == "false" { | ||
| enums = append(enums, false) | ||
| } else { |
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.
else if !stringx.IsWhiteSpace(enum)?
tools/goctl/api/swagger/parameter.go
Outdated
|
|
||
| func isPostJson(ctx Context, method string, tp apiSpec.Type) (string, bool) { | ||
| if strings.EqualFold(method, http.MethodPost) { | ||
| if !strings.EqualFold(method, http.MethodPost) { |
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.
duplicate with #4997
Fix the Swagger documentation generated when useDefinitions is set to true in the API file info. For POST type interfaces, although the parameter definitions are fully generated in the definitions, the parameters will be missing.