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

Pass symbol to respond_to? to fix NameError #3303

Merged
merged 1 commit into from
Feb 5, 2021

Conversation

mikepack
Copy link
Contributor

@mikepack mikepack commented Jan 27, 2021

I encountered this error while attempting to monkey patch GraphQL::ExecutionError:

require 'graphql/execution_error'

module ExecutionErrorMonkeyPatch
  def extensions
    {
      code: 'EXECUTION_ERROR',
      group: 'GRAPHQL',
    }.merge(Hash(super))
  end
end

GraphQL::ExecutionError.prepend(ExecutionErrorMonkeyPatch)

This change should fix the error, but I'm going to use this opportunity to ask how I can accomplish this without monkey patching. Any ideas?

@rmosolgo
Copy link
Owner

Wow, how did it ever work 🤔 !?

accomplish this without monkey patching

It seems like you want all ExecutionErrors, including ones raised by gem internals, to include those keys in their hashes. Besides extending the base class in some way, the only thing I can think of is adding those keys to the response after execution has finished, for example:

result = MySchema.execute(...) 
result["errors"].each do |err|
  if !err.key?("code")
    err["code"] = "EXECUTION_ERROR"
  end 
  if !err.key?("group")
    err["group"] = "GRAPHQL"
  end 
end 

Something like that might go in graphql_controller.rb, or it might be accomplished in a query instrumenter.

I'd love to merge this patch, but I'd like to include a test for the expected behavior (which was previously untested, evidently 😖 ). Are you willing to add a test for it? Honestly, I can't think of how to test this without monkey-patching the whole gem. I'll take a look.

merge variable is undefined
@mikepack
Copy link
Contributor Author

Something like that might go in graphql_controller.rb, or it might be accomplished in a query instrumenter.

Thanks for the tips, that's helpful.

I updated this PR with a spec which does reproduce the error. I used a bunch of Structs as doubles, which may not be ideal. Happy to iterate if you think there's a more concrete way.

@rmosolgo
Copy link
Owner

rmosolgo commented Feb 5, 2021

This is great, thanks for your work on it (and sorry it slipped off my radar til now)!

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

Successfully merging this pull request may close these issues.

2 participants