Implement Query#sanitized_query_string

[rmosolgo]

We want to improve logging, but don't leak private info into the logs. So, let's take a page out of Shopify's book (again 😊 ) and add a custom printer API.

Let's add a couple of steps:

• Implement class GraphQL::Language::Printer which has print_* methods corresponding to each node type; this is the base class for custom printers.
• Extend GraphQL::Language::Nodes::AbstractNode#to_query_string(printer: GraphQL::Language::Printer)
• Add Schema#sanitizing_printer_class; accept options via sanitize(whitelist:, blacklist:, ... ?)
• Add Query#sanitized_query_string which prints the document with the sanitization settings.

(I'm not sure I've got the right split of which methods go where; I'm open to reshuffling that, for sure. But I think it'd be nice to support an injected printer and a base implementation for printers.)

TODO: What does it look like to integrate this with Rails' parameter filtering?

If the approach works, maybe we can do something similar for #791

[xuorig]

Working on implementing a prototype of this, will take the opportunity to refactor GraphQL::Schema::Printer and Language::Generation which implement the same logic twice in a number of places 👍

[swalkinshaw]

When I was working on our query filterer, I did wonder about moving all the string generation into each node's class. Is it still a goal to keep these separated?

Right now only AbstractNode has to_query_string, but every node type could too?

[theorygeek]

Just for posterity, thought I'd add this code that @rmosolgo referenced, because the GraphQL Slack only lets you see the last 10k messages:

module FilteredLanguageGeneration
  include GraphQL::Language::Generation
  extend self

  def generate(node, indent: "")
    case node
    when GraphQL::Language::Nodes::Argument
      filtered_values = FilterParameters.filtered_graphql_values(node.name => node.value)
      "#{node.name}: #{generate(filtered_values[node.name])}"
    when GraphQL::Language::Nodes::InputObject
      generate(FilterParameters.filtered_graphql_values(node.to_h))
    else
      super
    end
  end
end

[xuorig]

When I was working on our query filterer, I did wonder about moving all the string generation into each node's class. Is it still a goal to keep these separated?

Right now only AbstractNode has to_query_string, but every node type could too?

Most nodes inherit from AbstractNode were the NameOnlyNodes the annoying part?

[swalkinshaw]

I described that terribly. I meant what about moving the string output into to_query_string for each node class instead of separated in the printer.

It would have made my filter simpler, but the new API might also accomplish that while also keeping the concerns separated.

[xuorig]

Ah interesting!

I think the problem there is that if you wanted your custom query string produced, you'd have to inherit from the nodes themselves or have some kind of hook in there.

I can see that being tricky since the parser wouldn't generate your "custom" nodes. WDYT?

[swalkinshaw]

👌 correct that's not good. I was remembering my issues from before. In the current situation it would have been a little more direct to monkey patch a node's to_query_string instead of re-doing that generate method. But with a better printer API then that's not needed.

[rmosolgo]

I posted our query printer for logging here: https://gist.github.com/rmosolgo/3452d2bf98837381df25949498e78e79 Maybe it could be upstreamed in some way.