GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — Drop paths left empty by tag and operationId filtering by 56steve · Pull Request #2638 · oapi-codegen/oapi-codegen Conversation
Contributor
When include-tags, exclude-tags, include-operation-ids or exclude-operation-ids removed every operation of a path, the path item stayed in the spec as an empty entry, so the embedded spec still listed it (for example
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
|
Uh oh!
There was an error while loading. Please reload this page.
[Medium impact] The change reaches code generation runs that use operation filters. Preserve compatibility for previously generated types before merging. FindingsFix with agent prompt### Issue 1 pkg/codegen/filter.go:40-41 Deleting the whole path also removes references from its shared parameters. If a filtered-out path has a parameter referencing `#/components/schemas/ItemID`, that reference previously kept `ItemID` in the generated Go code. Now `pruneUnusedComponents` deletes it when nothing else references it, breaking downstream code that uses the exported type. Both filter helpers have this problem. The repository requires compatibility protection for changes to generated APIs. Preserve the existing type output by default or make this broader change opt-in. ### Issue 2 pkg/codegen/filter_test.go:161-170 The new test checks only paths with one operation each. Add a path that starts empty and a path with both a kept and a removed operation, then assert that both paths survive under each filter kind. These cases protect the two deletion guards; the current tests would still pass if either guard were removed. Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly. SummaryThe PR removes paths after tag or operation-ID filtering removes their last operation.
Reviews (1) · Last reviewed commit: "Drop paths with no operations left after..." · Reviewed by Greptile |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
| if len(names) > 0 && len(pathItem.Operations()) == 0 { | ||
| paths.Delete(path) |
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.
Previously generated types disappear
Deleting the whole path also removes references from its shared parameters. If a filtered-out path has a parameter referencing #/components/schemas/ItemID, that reference previously kept ItemID in the generated Go code. Now pruneUnusedComponents deletes it when nothing else references it, breaking downstream code that uses the exported type. Both filter helpers have this problem.
The repository requires compatibility protection for changes to generated APIs. Preserve the existing type output by default or make this broader change opt-in.
Prompt To Fix With AI
This is a comment left during a code review. Path: pkg/codegen/filter.go Line: 40-41 Comment: **Previously generated types disappear** Deleting the whole path also removes references from its shared parameters. If a filtered-out path has a parameter referencing `#/components/schemas/ItemID`, that reference previously kept `ItemID` in the generated Go code. Now `pruneUnusedComponents` deletes it when nothing else references it, breaking downstream code that uses the exported type. Both filter helpers have this problem. The repository requires compatibility protection for changes to generated APIs. Preserve the existing type output by default or make this broader change opt-in. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| t.Run("by tag", func(t *testing.T) { | ||
| swagger := load(t) | ||
| filterOperationsByTag(swagger, Configuration{OutputOptions: OutputOptions{IncludeTags: []string{"kept"}}}) | ||
| assert.Equal(t, []string{"/kept"}, swagger.Paths.Keys()) | ||
| }) | ||
|
|
||
| t.Run("by operation id", func(t *testing.T) { | ||
| swagger := load(t) | ||
| filterOperationsByOperationID(swagger, Configuration{OutputOptions: OutputOptions{ExcludeOperationIDs: []string{"getDropped"}}}) | ||
| assert.Equal(t, []string{"/kept"}, swagger.Paths.Keys()) |
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.
Path preservation lacks tests
The new test checks only paths with one operation each. Add a path that starts empty and a path with both a kept and a removed operation, then assert that both paths survive under each filter kind. These cases protect the two deletion guards; the current tests would still pass if either guard were removed.
Prompt To Fix With AI
This is a comment left during a code review. Path: pkg/codegen/filter_test.go Line: 161-170 Comment: **Path preservation lacks tests** The new test checks only paths with one operation each. Add a path that starts empty and a path with both a kept and a removed operation, then assert that both paths survive under each filter kind. These cases protect the two deletion guards; the current tests would still pass if either guard were removed. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Reviewers
At least 1 approving review is required to merge this pull request.
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
1 participant
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.