Global analyzers error context?

[gjtorikian]

Hello! Now I can use phrases like, "At GitHub, we've..." 😆

At GitHub, we've got a few query analyzers. Two that report errors back to the user are our scope analyzer and our pagination analyzer. The scope analyzer ensures that, given a query, the requesting user has the right OAuth scopes and access (for example, if your query requests a user's email, you'll need the user:email scope. Our pagination analyzer reports on things like if you're not using first: or last: on connections, or if your first value is insanely large, etc.

We're interested in cascading errors through all our analyzers, such that the errors response a user receives reports on all the errors in a query. After your talk with @bswinnerton, we realized we could use final_value to report the sum of all the errors, but this is scoped per-query. We're trying to avoid a situation where we provide errors on scoping, the user fixes that, and then we provide errors on missing pagination arguments. Let's just throw 'em all at once!

Would you be open to a patch that handles this? I'm thinking of passing along an ivar through the analyzers, and then defining a new method, like schema.query_analysis_result_handler, which a user can then define to do whatever they want.

[rmosolgo]

The goal is to support query analyzers returning errors. Here's the code that's supposed to handle one returned error per analyzer:

https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/query.rb#L184-L185

(That is, #final_value(memo) may return an AnalysisError, which will be reported to the user & prevent the query.)

The intended use case is just what you described: different analyzers can return their own errors to the user. However:

• I don't think the multiple-analyzers case is actually tested so ... maybe it's broken?
• Maybe it would be an improvement to support multiple errors per analyzer (for example, one error per erroneous pagination)

I know it's a bit weird to have an API that expects errors as return values. This is a personal opinion, taken after http://c2.com/cgi/wiki?DontUseExceptionsForFlowControl . I'm open to supporting raise(GraphQL::AnalysisError.new(msg, node: ast_node)) via the most local begin ... rescue possible, which I think would be right here: https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/analysis/analyze_query.rb#L46. I suppose QueryAnalysis would have to maintain an array of raised errors for each analyzer, and rescue them by shoveling them into that array. Then, in .finalize_reducer, it would check for raised errors and return those instead of calling the final_value method.

How does that sound to you? Would any of those improvements work in your case?

[gjtorikian]

Would any of those improvements work in your case?

This part:

I suppose QueryAnalysis would have to maintain an array of raised errors for each analyzer, and rescue them by shoveling them into that array. Then, in .finalize_reducer, it would check for raised errors and return those instead of calling the final_value method.

would be wonderful. Mostly because it separates the notion of a memo, something that is passed around within a single analyzer, apart from an AnalysisError, which is something that can be aggregated and acted upon at the end of the analysis.

I can get to work on a PR if that works for you.

I don't think the multiple-analyzers case is actually tested so ... maybe it's broken?

I think it works as expected and/or designed. Every analyzer is separate and does its own thing. From a usage perspective, this separation of concerns breaks down when you try to do error handling. It's a bit like a linter that would only complain about one error at a time.

[rmosolgo]

Consider these two analyzers (neither one works yet):

# Check for proper permissions & record any errors 
class PermissionAnalyzer1 
  def initial_value(query) 
    { 
      current_user: query.context[:current_user],
      errors: [],
     }
  end 

  def call(memo, visit_type, irep_node)
    required_scope = get_required_scope(irep_node)
    if !has_required_scope(memo[:current_user], required_scope)
      memo[:errors] << GraphQL::AnalysisError.new("Scope required but not present: #{required_scope}")
    end 
    memo 
  end 

  # Return errors ... or an empty array, if there weren't any
  def final_value(memo) 
    memo[:errors]
  end 
end 

# Check for proper permissions & raise errors 
class PermissionAnalyzer2 
  def initial_value(query) 
    {
      current_user: query.context[:current_user],
    }
  end 

  def call(memo, visit_type, irep_node)
    required_scope = get_required_scope(irep_node)
    if !has_required_scope?(memo[:current_user], required_scope)
      raise GraphQL::AnalysisError.new("Scope required but not present: #{required_scope}")
    end 
    memo 
  end 

  def final_value(memo) 
    # pass, any notable event was handled by raising an error 
  end 
end 

These are the two options we're talking about, right? I prefer the first one because, for a reader, you're not required to "jump" to gem internals to see where that error is going. (Or, if you're lucky, it might be documented 😬 )

In any case, implementing the second one includes supporting the first, and since this is Ruby, we should support raise in order to not surprise people! So if you want to take a crack at that, I say go for it!

• AnalyzeQuery#visit_analyzers should rescue an AnalysisError inside the reducer function and store it away somewhere. Then, if it found any errors, it should return those instead of calling #final_value
• Query#query_valid? should be prepared for an array of errors or a single error

[gjtorikian]

I actually prefer the first one, too!