Support multiple response schemas for OpenAPI 2

[renatolond]

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!

[geemus]

@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.

[renatolond]

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.
I would like to move to OpenAPI 3, but still haven't had the time to dive into the spec to see if I can help in the effort :)

[geemus]

@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.

[renatolond]

Yeah, I think it should be easy to test this with the test that already exist on committee, if you apply:

diff --git a/test/schema_validator/hyper_schema/response_validator_test.rb b/test/schema_validator/hyper_schema/response_validator_test.rb
index dee312b..573f275 100644
--- a/test/schema_validator/hyper_schema/response_validator_test.rb
+++ b/test/schema_validator/hyper_schema/response_validator_test.rb
@@ -101,6 +101,11 @@ describe Committee::SchemaValidator::HyperSchema::ResponseValidator do
     call
   end
 
+  it "returns a 404 response, not described in the schema" do
+    @status = 404
+    call
+  end
+
   it "raises errors generated by json_schema" do
     @data.merge!("name" => "%@!")
     e = assert_raises(Committee::InvalidResponse) { call }

Without my change (on master), this test will pass.

In this branch, it will raise:

  1) Error:
Committee::SchemaValidator::HyperSchema::ResponseValidator#test_0012_returns a weird response:
Committee::InvalidResponse: Invalid response./apps/{(%23%2Fdefinitions%2Fapp%2Fdefinitions%2Fname)} status code 404 definition does not exist
    lib/committee/schema_validator/hyper_schema/response_validator.rb:56:in `call'
    test/schema_validator/hyper_schema/response_validator_test.rb:121:in `call'
    test/schema_validator/hyper_schema/response_validator_test.rb:106:in `block (2 levels) in <top (required)>'

[geemus]

@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.

[geemus]

@renatolond Sorry for the delays, I'd looked at this a couple times to see if I could find another way and never really landed on a different approach. As the tests pass it seems worth giving this a shot more or less as-is. That said, it seems it no longer cleanly applies (again, sorry for delays). Could you take a peak at getting this updated to be compatible with main again and I'll try to get it in more quickly this time around?

[renatolond]

@geemus Pushed a rebase commit, should be good now :)

[geemus]

@renatolond thanks, lets bring this in then. I'll just note that given that we aren't quite sure the broader impact/etc I may end up needing to revert this if it causes bigger issues that we can't otherwise easily resolve. Hopefully it doesn't come to that, but I just want to set expectations.

[renatolond]

Makes sense, thanks for taking the time to look into this! 💯