Skip to content
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

cannot stub request anymore #100

Closed
Nghi93 opened this issue Oct 12, 2022 · 11 comments · Fixed by #110
Closed

cannot stub request anymore #100

Nghi93 opened this issue Oct 12, 2022 · 11 comments · Fixed by #110
Labels

Comments

@Nghi93
Copy link

Nghi93 commented Oct 12, 2022

Describe the bug
Since 0.7.0 it is not possible to stub_request in ones specs anymore:
<NoMethodError: undefined method `values_at' for #String:0x00007f9defb82e20

To Reproduce
Steps to reproduce the behavior:

let(:response) do
      {
        'data' => {
          'someQuery' => {
            '__typename' => 'SomeType',
            'id' => 'A123'
          }
        }
      }.to_json
end

before do
  client = Graphlient::Client.new(url, schema_path: x)
  stub_request(:post, url).to_return(status: 200, body: response)
end

it do
  client.execute(some_query)
end

We get <NoMethodError: undefined method `values_at' for #String:0x00007f9defb82e20

I could track it down to graphql-client-0.18.0/lib/graphql/client.rb:358:

358         execute.execute(
359           document: document,
360           operation_name: operation.name,
361           variables: variables,
362           context: context
363        )

On a regular call this will result a hash. During a stubbed call it will return my mock "response" string, which does not have the method values_at which causes the error above.

I am not sure if this is a bug in graphlient or graphql-client. Could you provide further informations, please?

@ashkan18
Copy link
Owner

ashkan18 commented Nov 4, 2022

Hi, thanks for reporting this! are you still dealing with this issue?

@Nghi93
Copy link
Author

Nghi93 commented Nov 18, 2022

Hi @ashkan18,

yes, it's still an issue.

@ashkan18
Copy link
Owner

Sorry to hear that, i haven't had a chance to look into it! but feel free to look into it and open a PR if you had a chance.

@mikecx
Copy link

mikecx commented Mar 9, 2023

In case anyone else runs into this, I was able to get around it by adding a content-type header to the stub-request call like so:

stub_request(:post, MyGem::Client::ENDPOINT).to_return(status: 200, body: json_response.to_json, headers: { 'Content-Type' => 'application/json' })

@dblock
Copy link
Collaborator

dblock commented Mar 9, 2023

@mikecx Do we need to document this? Is there a fix to avoid the problem? Care to PR?

@mikecx
Copy link

mikecx commented Mar 10, 2023

@dblock Yeah, seems like it would be good to update the README.md with the updated stubbing. I'll put together a doc update PR when I get some free time, though it might be a few days.

@ashkan18
Copy link
Owner

thanks @mikecx ❤️ 💜 💚

@dblock dblock added the bug label Dec 26, 2023
@taylorthurlow
Copy link
Contributor

taylorthurlow commented Jan 4, 2024

I just upgraded from 0.5 to 0.7 and ran into the same problem. It's possible that I haven't thought this through fully - I think it's related to changes that happened surrounding the Faraday upgrade and how the JSON middleware is used. The bottom line is that it seems like Graphlient should not be assuming that a response body from a graphql API is JSON. A normally operating one would be, but there is no graceful failure here. Especially in the case where the content type of the response is not application/json. I assume that there was an exception being raised in an earlier version of Graphlient that is no longer being raised.

I don't quite have the time to dig further right now, though, sorry.

@taylorthurlow
Copy link
Contributor

taylorthurlow commented Jan 4, 2024

Ok, looks like I had the time 😆 - I think this is the result of a change in the Faraday json response middleware from 1.x to 2.x.

Here is the faraday_middleware implementation of the response JSON parser (i.e. how Faraday 1.x works):

https://github.com/lostisland/faraday_middleware/blob/e68ff84470705c8a59d9aa99b98cb36101fa10ad/lib/faraday_middleware/response/parse_json.rb#L13

Regardless of content type, if the body contains any non-whitespace characters, it'll parse it as JSON. Presumably this gets caught somewhere and raised as a Faraday::ParsingError.

Contrast this with the Faraday 2.x built-in JSON response middleware:

https://github.com/lostisland/faraday/blob/074506e67c20e5c79f7f8fb544a318eb932d6bc8/lib/faraday/response/json.rb#L24

We #process_response (by default call JSON.parse) if #parse_response? is true. It would be false if the content type did not match /\bjson$/.


So to summarize this is just an (undocumented?) change in how the faraday JSON response middleware works and a change should probably be made to Graphlient to raise an appropriate exception if the response from the GraphQL API is not valid JSON.


EDIT: I might suggest changing this single line:

To something like:

    case (body = response.body)
    when Hash, Array
      body
    when String
      begin
        JSON.parse(body)
      rescue JSON::ParserError => e
        raise Graphlient::Errors::ServerError, "Failed to parse response body as JSON"
      end
    else
      inner_exception = StandardError.new("Unexpected response body type '#{body.class}")
      raise Graphlient::Errors::ClientError, inner_exception
    end

@ashkan18
Copy link
Owner

ashkan18 commented Jan 5, 2024

Thanks @taylorthurlow for investigative work 🔬 work! do you mind opening a PR with that proposed change when have a chance?

@taylorthurlow
Copy link
Contributor

@ashkan18 Opened 👍

ashkan18 pushed a commit that referenced this issue Jan 6, 2024
…e header (#110)

See my investigation here:
#100 (comment)

In the Faraday 1 to Faraday 2 process, the ensure JSON response
middleware was rewritten from scratch, which caused a change in how
permissive the middleware was to response Content-Type headers which did
not properly indicate a JSON response.

In Faraday 1.x, the JSON middleware would attempt a JSON parse of the
body, regardless of the presence or value of a Content-Type header. This
meant that Graphlient would have raised an exception (or re-raised one
caught from Faraday) if the response body was not valid JSON.

In Faraday 2.x, the new JSON middleware will only attempt to parse the
response body if it determines that the Content-Type response header
indicates a JSON body. By default it uses a regex on the header value
(/\bjson$/ at the time of writing). Crucially, if the header is missing,
or does not match the regex, instead of raising an exception,
`Faraday::Response#body` returns a String, rather than the
previously-assumed Hash or Array.

This change restores the parsing logic which is tolerant to incorrect or
missing Content-Type header values. Response bodies which are not valid
JSON will raise an exception. Any unforseen situation where `#body`
returns a type that is not String, Hash, or Array, will raise a separate
exception detailing that this is unexpected behavior and a bug should be
filed.

Resolves #100
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants