ashkan18 / graphlient

Ruby GraphQL Client
MIT License
251 stars 44 forks source link

NoMethodError in FaradayServerError #51

Closed chmorik closed 5 years ago

chmorik commented 6 years ago

Hi, We've encounter a situation where we get undefined method `[]' for nil:NilClass in graphlient-0.3.3/lib/graphlient/errors/faraday_server_error.rb:7 This is very frustrating because we don't know what is the error behind it We think that changing to @response = inner_exception.response.try(:[],:body) @status_code = inner_exception.response.try(:[],:status)

will fix it

dblock commented 6 years ago

Try writing a spec that reproduces the problem?

chmorik commented 6 years ago

e = Faraday::ClientError.new(nil) raise Graphlient::Errors::FaradayServerError, e

dblock commented 6 years ago

In which scenario does this actually happen? What's the stack?

The code should be fixed either way. Appreciate a pull request.

chmorik commented 6 years ago

The thing is I don't know. If it'll be fixed I can tell you. Stack trace for example:


NoMethodError: undefined method `[]' for nil:NilClass
1
File "/app/vendor/bundle/ruby/2.3.0/gems/graphlient-0.3.3/lib/graphlient/errors/faraday_server_error.rb" line 7 in initialize
2
File "/app/vendor/bundle/ruby/2.3.0/gems/graphlient-0.3.3/lib/graphlient/adapters/http/faraday_adapter.rb" line 19 in exception
3
File "/app/vendor/bundle/ruby/2.3.0/gems/graphlient-0.3.3/lib/graphlient/adapters/http/faraday_adapter.rb" line 19 in raise
4
File "/app/vendor/bundle/ruby/2.3.0/gems/graphlient-0.3.3/lib/graphlient/adapters/http/faraday_adapter.rb" line 19 in rescue in execute
5
File "/app/vendor/bundle/ruby/2.3.0/gems/graphlient-0.3.3/lib/graphlient/adapters/http/faraday_adapter.rb" line 8 in execute
6
File "/app/vendor/bundle/ruby/2.3.0/gems/graphql-client-0.13.0/lib/graphql/client.rb" line 328 in block in query
7
File "/app/vendor/bundle/ruby/2.3.0/gems/activesupport-4.1.14/lib/active_support/notifications.rb" line 161 in instrument
8
File "/app/vendor/bundle/ruby/2.3.0/gems/graphql-client-0.13.0/lib/graphql/client.rb" line 327 in query
9
File "/app/vendor/bundle/ruby/2.3.0/gems/graphlient-0.3.3/lib/graphlient/client.rb" line 25 in execute

I can't open one right now. but


@response = inner_exception.response.try(:[],:body)
@status_code = inner_exception.response.try(:[],:status)

should fix it

ashkan18 commented 6 years ago

Looks like we run into a case where inner_exception, possibly a Faraday one, doesn't have body or status, I can look into it in a bit but would be interesting to find out how that happens as @dblock mentions. If that's a valid case, we should possibly replace body and status to a more generic error instead of just setting them to nil

dblock commented 6 years ago

If someone wants to just PR a defensive implementation for Faraday::ClientError taking a nil I'd be happy to review it, but I think there's more going on here.

yuki24 commented 6 years ago

It's probably the case that the request timed out and the client threw an error without a response object since there's nothing you could assign to the exception when the request didn't go through.

dfritsch86 commented 5 years ago

On that note, is there a way to pass options to Faraday, e.g. to increase the standard request time out duration? I find myself having to query an API that requires more than the default 30s to generate a response, so raising that would be nice.

dblock commented 5 years ago

On that note, is there a way to pass options to Faraday, e.g. to increase the standard request time out duration? I find myself having to query an API that requires more than the default 30s to generate a response, so raising that would be nice.

I believe this will work (haven't tested)

Graphlient::Client.new('http://test-graphql.biz/graphql') do |client|
      client.http do |h|
        h.connection do |c|
          c.options.timeout = 30
        end
      end
    end

If it does, please add to docs?

We could also expose http options, I wouldn't mind supporting something like

Graphlient::Client.new('http://test-graphql.biz/graphql', http: { connection: { timeout: 30 } }) 
dblock commented 5 years ago

Closed via https://github.com/ashkan18/graphlient/pull/68.