-
Notifications
You must be signed in to change notification settings - Fork 137
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
Support multiple response schemas for OpenAPI 2 #433
base: master
Are you sure you want to change the base?
Support multiple response schemas for OpenAPI 2 #433
Conversation
@renatolond - Thanks. I'm busy with holiday and family things this week, but will try to review for you probably early next week once things are more back to normal. Overall this sounds reasonable and though it is potentially a breaking change, it seems likely to be breaking an unintended behavior instead of something that was really there on purpose. Out of curiosity, are there things that are preventing you from migrating to OpenAPI3? I'd still like to fix this for OpenAPI2, but it did make me wonder how we could help you get on the latest as well. |
Hey @geemus, no rush, enjoy your holidays! I'm using grape/grape-swagger and currently it generates the docs in OpenAPI2, there's a few discussions and PRs around going to v3, but they are not final yet. Most notably ruby-grape/grape-swagger#603 and ruby-grape/grape-swagger#928, I'd say. |
def target_schema | ||
target_schemas[status_success] || | ||
target_schemas[200] || | ||
target_schemas[201] || | ||
target_schemas.values.first | ||
end |
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.
Since I already call find_best_fit_response
in the driver, I'm not sure this is needed here. Maybe just calling target_schemas[status_success]
would be enough?
This is mostly for compatibility with hyper schema anyway.
@renatolond sorry for the delays, spent a bit of time today trying to read and think through this. Could you share an (hopefully fairly minimal) example schema where it fails? Just want to make sure I'm on the same page of the exact culprit, and I might try to experiment a little with specifics once I have a repro case as I'm not quite sure how best to approach this. Thanks. |
Yeah, I think it should be easy to test this with the test that already exist on committee, if you apply:
Without my change (on master), this test will pass. In this branch, it will raise:
|
@renatolond Thanks, I started digging through this more today, but haven't reached concrete conclusions quite yet. It's a bit complicated in there, so I may need a little more time to ponder and digest. |
Hi!
I noticed while testing OpenAPI3 vs OpenAPI2 that if a response was missing in the OpenAPI definition, then the OpenAPI2 driver would fail silently.
It's not the case for OpenAPI 3, where it says that the status code one tried to validate is not defined.
I feel that this might be a breaking change (since this will raise an error, where before it was working without problems), but I still think it's worth checking with you if you think it's a change worth merging into committee.
Let me know if you this makes sense or if you'd like me to change something!