search

uniiverse / apollo-tracing-ruby

[Maintainers Wanted] Ruby implementation of GraphQL trace data in the Apollo Tracing format
MIT License
85 stars 10 forks source link

after_query: avoid trying to instrument failed query results #7

Closed olleolleolle closed 6 years ago

olleolleolle commented 6 years ago

This PR adds an early return to the after_query: if query.result is nil, this method will otherwise raise a NoMethodError.

This could happen if something broke "before getting here".

Benefit: the user will not begin suspecting this gem of being the culprit.

Question for reviewer: Would a log message such as "[apollo-tracing] No result to instrument!" be useful?

exAspArk commented 6 years ago

Hey, @olleolleolle! Thanks a lot for the PR!

Could you please write a test which covers the case when result is nil?

Would a log message such as "[apollo-tracing] No result to instrument!" be useful?

Yeah, I think it makes sense! But probably in a separate PR. Ideally, we should then allow configuring the logger and respect its level. For example, something like:

my_logger = Logger.new(STDOUT).tap { |l| l.level = Logger::WARN }

Schema = GraphQL::Schema.define do
  ...
  use ApolloTracing.new(logger: my_logger)
end
olleolleolle commented 6 years ago

@exAspArk Well, npm downloads can easily fail. They just did in CI.

exAspArk commented 6 years ago

Well, npm downloads can easily fail. They just did in CI.

Yeah, not the most reliable package registry :) When the gem is pushed to rubygems, it includes the binaries. So the gem doesn't depend on npm packages after that.

I just ran the tests with the 1.6.8 graphql version, and the tests are failing with the following trace:

expected NoMethodError with message matching /undefined method `title' for/, got #<SystemStackError: stack level too deep> with backtrace:

 # ./lib/apollo_tracing.rb:62:in `after_query'
 ...
 # ./lib/apollo_tracing.rb:62:in `after_query'
 # ./spec/apollo_tracing_spec.rb:37:in `block (4 levels) in <top (required)>'
 # ./spec/apollo_tracing_spec.rb:36:in `block (3 levels) in <top (required)>'

I think it's time either to require using graphql version >= 1.7.0 or find a workaround for this issue. Going to read through the graphql changelog :)

P.S. Thanks for the test!

exAspArk commented 6 years ago

@olleolleolle thanks for contributing! I also added this commit https://github.com/uniiverse/apollo-tracing-ruby/commit/c88e6bb4de575665df15714e23a5cd23755b2cf1 to restrict the graphql gem version since version 1.6 doesn't work properly.

Released your fix in the version 1.5.0 🎉

olleolleolle commented 6 years ago

That’s very kind of you.

On 12 Mar 2018, at 17:01, exAspArk notifications@github.com wrote:

@olleolleolle thanks for contributing! I also added this commit c88e6bb to restrict the graphql gem version since version 1.6 doesn't work properly.

Released your fix in the version 1.5.0 🎉

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.