ahx / openapi_first

openapi_first is a Ruby gem for request / response validation and contract-testing against an OpenAPI API description. It makes APIFirst easy and reliable.
MIT License
97 stars 12 forks source link

rails 6 - error when parsing non-array response body #246

Closed samanthawritescode closed 4 months ago

samanthawritescode commented 4 months ago

Hey! I'm trying to set up the response validation middleware and got a rather opaque error like:

     NoMethodError:
       undefined method `each' for nil:NilClass

             @body.__send__(method_name, *args, &block)
                  ^^^^^^^^^
     # /usr/local/bundle/gems/rack-2.2.8.1/lib/rack/body_proxy.rb:41:in `method_missing'
     # /usr/local/bundle/gems/rack-2.2.8.1/lib/rack/etag.rb:69:in `digest_body'
     # /usr/local/bundle/gems/rack-2.2.8.1/lib/rack/etag.rb:33:in `call'
     # /usr/local/bundle/gems/rack-2.2.8.1/lib/rack/conditional_get.rb:40:in `call'
     # /usr/local/bundle/gems/rack-2.2.8.1/lib/rack/head.rb:12:in `call'
     # /usr/local/bundle/gems/rack-2.2.8.1/lib/rack/session/abstract/id.rb:266:in `context'
     # /usr/local/bundle/gems/rack-2.2.8.1/lib/rack/session/abstract/id.rb:260:in `call'

When debugging, it seems that body from this line is nil. I had a hunch that it was because it wasn't able to match the API that I'm testing with an actual path in my spec. For context, I have endpoints like https://example.com/api/v1/object. When setting up my spec initially configured the server url to be https://example.com/api/v1, so all the path names could simply be something like object. When I changed the path name to be /api/v1/object -- I moved on to a new error. I think this means that my hunch was correct, but I'm not really sure. If it's helpful, here's the new error, but I'm not sure it's related:

OpenapiFirst::ResponseInvalidError:
       Response body is invalid: Failed to parse response body as JSON
     # /usr/local/bundle/gems/openapi_first-1.3.4/lib/openapi_first/failure.rb:62:in `raise!'
     # /usr/local/bundle/gems/openapi_first-1.3.4/lib/openapi_first/runtime_request.rb:140:in `validate_response'
     # /usr/local/bundle/gems/openapi_first-1.3.4/lib/openapi_first/middlewares/response_validation.rb:28:in `call'
samanthawritescode commented 4 months ago

Actually, body is still nil in this case, I think that's why now it says it can't parse. I'm honestly kind of at a loss here, any help would be appreciated. My OAS is here, but I changed the URL parameters to continue testing according to this issue.

ahx commented 4 months ago

Thanks for the report @samanthawritescode. I am not fully understanding what's going on here. Can you reproduce this in a test, Maybe a rack-test test or Rails' request test and can share a part of that maybe?

MrBananaLord commented 4 months ago

I am wondering... what happens when you run the same request without the validation middleware? could it be that you are getting a 302? 🤔

samanthawritescode commented 4 months ago

@MrBananaLord I ran the same request without the response validation middleware after installing the gem, upgrading any dependencies, etc... just commenting out the config.middleware.use OpenapiFirst::Middlewares::ResponseValidation line. It succeeds successfully.

I'll see if I can get a reproducible example.

samanthawritescode commented 4 months ago

Alright, fresh eyes after the weekend did me good, I think I figured out the issue.

The problem is actually on this line in Middlewares::ResponseValidation.call:

body = body.to_ary if body.respond_to?(:to_ary)

The problem is that body.respond_to?(:to_ary) returns true, but it shouldn't, because the response is not an array (it's a JSON object). So when we call body.to_ary, body ends up nil.

So why are we incorrectly calling to_ary and why does it end up nil?

I'm using Rails 6.1.7.7, and the respond_to? and to_ary methods are defined as such in ActionDispatch:

      def respond_to?(method, include_private = false)
        if method.to_sym == :to_path
          @response.stream.respond_to?(method)
        else
          super
        end
      end

      def to_ary
        nil
      end

This has since changed in rails 7, and I assume that's what the intended behavior was.

As for what the actual fix is, I'm not sure, this is a quite a bit more in the weeds of Rails/Rack than I typically get. I hope that's helpful, though.

ahx commented 4 months ago

@samanthawritescode Thanks for the update. This is helpful. I still have a hard time understanding the Rack spec about these parts. That's probably why this slipped through. If I understand you correctly this happens only in tests, right? This looks similar to something I tested/implemented here. Maybe we can come up with a (failing) test that looks similar.

ahx commented 4 months ago

What I don't get about the spec is what middlewares should check first: call, to_ary or each. I will have to find some time to look into that. I hope that this issue does not block you. If it does, let's look for a quick solution instead.

ahx commented 4 months ago

I did not find the time to reproduce this. Can you maybe share a tiny example app via a gist or the like?

ahx commented 4 months ago

Hi. I was able to reproduce this and will probably release a fix this weekend.

samanthawritescode commented 4 months ago

Great, thanks! Sorry, I had to pivot to work on something else, so I haven't had further time to poke at this.

ahx commented 4 months ago

This should be fixed and now just work in 1.3.6, which has just been released. Thanks for the report.