-
-
Notifications
You must be signed in to change notification settings - Fork 232
fix: stable OpenAPI property and param ordering #71
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
Conversation
danielgtaylor
left a comment
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.
Awesome work! 👍 Ordering by Go struct field definition is a good way to quickly fix this bug. I took a quick peek at the router, resource, operation, request, response and I think this PR fixes the only things preventing stable OpenAPI generation!
Definitely not a blocker, but one improvement I can think of for the future might be to order them based on their appearance in the URL template string, regardless of struct order. That way any inconsistency in the Go code ordering would not impact the generated parameter order given two operations with the same URL template. That's more difficult/complex to implement though and I think the current implementation is good.
|
|
||
| var p opschema | ||
| json.Unmarshal(openapi, &p) | ||
| assert.Equal(t, p.Parameters, []parameters{{Name: "menu-category"}, {Name: "item-id"}}) |
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.
non-blocking: there's a 'JSONEq' that could be useful here. Something like:
assert.JSONEq(t, `[{"name": "menu-category", ...}]`, openapi)`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.
I considered using that, but unfortunately, the output of the gabs.Search ends at the operation level. This results in openapi containing the full spec for the get operation (ex. responses, additional properties for a parameter). JSONEq only returns true if the entire JSON payload matches (ignoring order). Given we're only interested in using the name to check order was correctly preserved, I decided with a temp type to only parse the necessary field.
Codecov ReportBase: 87.18% // Head: 87.23% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #71 +/- ##
==========================================
+ Coverage 87.18% 87.23% +0.04%
==========================================
Files 25 25
Lines 2138 2146 +8
==========================================
+ Hits 1864 1872 +8
Misses 193 193
Partials 81 81
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
danielgtaylor
left a comment
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.
LGTM 👍
Addresses two issues with stable/consistent OpenAPI generation:
Properties will now output in lexicographical order for consistency. Operation parameters will honor the order they're defined in the struct.
A new example called
bookstoreis also added to demo a slightly more complex OpenAPI configuration where the schema ordering matters.