github
Allow dynamic queries
Hi!
I'm just trying out "graphql-client" gem and I hit the GraphQL::Client::DynamicQueryError:
https://github.com/github/graphql-client/blob/master/guides/dynamic-query-error.md
I'm wondering why, especially in development, when I'm trying out the API in the console, this error is raised? It's a performance overhead. That is a valid point.
But I'd argue that it's never a reason to raise an error. Perhaps a warning would suffice?
There could at least be a configuration option that disables this error from being raised.
Which performance overhead are we talking in the first place?
Does this code make any performance overheard:
# lib/tasks/no_overhead.rake
namespace :overheadder do
task no: :environment do
graphql_endpoint_url = 'https://payments.braintree-api.com/graphql'
braintree_graphql_connection = GraphQL::Client::HTTP.new(graphql_endpoint_url) do
def headers(_context)
{
Authorization: "Basic #{ENV.fetch('BRAINTREE_GRAPHQL_API_KEY')}",
'Braintree-Version': '2019-01-01',
'Content-Type': 'application/json',
}
end
end
braintree_graphql_schema = GraphQL::Client.load_schema('json/braintree_graphql_schema.json')
client = GraphQL::Client.new(schema: braintree_graphql_schema, execute: braintree_graphql_connection)
query = <<~GRAPHQL
query search($after: String) {
search {
customers(input: {}, first: 50, after: $after) {
edges {
node {
email legacyId
}
}
pageInfo {
hasNextPage endCursor
}
}
}
}
GRAPHQL
customers = []
after = nil
loop do
result = client.query(client.parse(query, variables: { after: }))
customers += result.data.search.customers.edges.map(&:node)
break unless result.data.search.customers.pageInfo.hasNextPage
after = result.data.search.customers.pageInfo.endCursor
end
{ customers: }
end
end
I'd argue that it doesn't. It's probably 100% fine to have this Rake task executed on a live site. Why would the gem prevent me from doing that? I think this is doing a bit too much to protect developers from themselves.
Thanks for considering this issue!
The above example is the opposite of me having the majority of the code written in Ruby constants:
class BraintreeCustomersFetcher < ApplicationService
GRAPHQL_ENDPOINT_URL = 'https://payments.braintree-api.com/graphql'
BRAINTREE_GRAPHQL_CONNECTION = GraphQL::Client::HTTP.new(BraintreeCustomersFetcher::GRAPHQL_ENDPOINT_URL) do
def headers(_context)
{
Authorization: "Basic #{ENV.fetch('BRAINTREE_GRAPHQL_API_KEY')}",
'Braintree-Version': '2019-01-01',
'Content-Type': 'application/json',
}
end
end
BRAINTREE_GRAPHQL_SCHEMA = GraphQL::Client.load_schema('json/braintree_graphql_schema.json')
BRAINTREE_GRAPHQL_CLIENT = GraphQL::Client.new(schema: BRAINTREE_GRAPHQL_SCHEMA,
execute: BRAINTREE_GRAPHQL_CONNECTION)
BRAINTREE_GRAPHQL_CUSTOMER_SEARCH_QUERY = <<~GRAPHQL
query($after: String) {
search {
customers(input: {}, first: 50, after: $after) {
edges {
node {
email legacyId
}
}
pageInfo {
hasNextPage endCursor
}
}
}
}
GRAPHQL
def call
# the implementation starts here
end
end
I could see the benefits of both approaches, but I don't see the need to prevent one or the other.
I'm also wondering how big of an overhead it actually is to parse the query dynamically? It could easily be negligent for an average web application.
I also just starting trying out the graphql-client gem, and I completely agree with @vfonic here. Forcing a design decision like this on your users is rarely the right course, unless there's a technical reason it must be this way.
In fact there are cases where constant static queries are entirely the wrong approach. Consider for example a command-line client capable of calling a large number of enterprise endpoints - which is actually what I am writing. Such a client will define many, infrequently called queries and may in fact invoke only one out of dozens of potential queries per run. Constant static definitions will require the client to compile all of those queries every time, whether they will get used or not. That's less efficient than compiling the one dynamic query actually needed.
In addition, there are other one-time initialization techniques than static constants to avoid recompiling the same queries over and over if that's the concern - for example creating class variables in the initialization method of a class.
I found the Client.allow_dynamic_queries setting, and also found that it's deprecated. I would encourage you to drop this restriction entirely, and instead recommend your approach in the readme.
I use graphlient (which uses graphql-client under the hood). And this whole dynamic query thing has been tickling my brain.
I totally respect the idea that the library is trying to guard against unintentionally producing a situation where the same query is parsed over and over again. Especially if that means HTTP calls each time.
However, I also think its almost frivolous to attempt forcing that onto the users with the "must be a const check". As others have pointed out, there are other ways to approach the design goal (parse as close to 'just once') as possible, while acknowledging that how the users of the library use the library is what ultimately determines whether or not the "parse just once" goal is achieved.
To prove my point that "the users will fail or succeed despite the library", here's an example using graphql-client where I've essentially worked around the dynamic query check (stealing the OPs brain tree examples)
require "graphql-client"
# This client is just to hold the various setup in a single place and
# use the Singleton + DelegateClass pattern to "just do it once"
class Client < DelegateClass(GraphQL::Client)
include Singleton
def initialize
@connection = GraphQL::Client::HTTP.new("https://payments.braintree-api.com/graphql") do
def headers(_context)
{
Authorization: "Basic #{ENV.fetch('BRAINTREE_GRAPHQL_API_KEY')}",
'Braintree-Version': '2019-01-01',
'Content-Type': 'application/json',
}
end
end
@schema = GraphQL::Client.load_schema('json/braintree_graphql_schema.json')
@client = GraphQL::Client.new(schema: @schema, execute: @connection)
super(@client)
end
end
# Encapsulate GraphQL::Client parsing and execution so that we
# 1) only parse the query _once_ **but**
# 2) only parse the query _when demanded_
class Query
def self.new(query)
klass = Class.new do
def self.definition
const_set(:DEFINITION, Client.instance.parse(@query)) unless const_defined?(:DEFINITION)
const_get(:DEFINITION)
end
def self.execute(**variables)
Client.instance.query definition, variables: variables
end
end
klass.class_exec { @query = query }
klass
end
end
# Then define queries all day long
my_query = Query.new(<<~GRAPHQL)
query($after: String) {
search {
customers(input: {}, first: 50, after: $after) {
edges {
node {
email legacyId
}
}
pageInfo {
hasNextPage endCursor
}
}
}
}
GRAPHQL
my_query.execute(after: "something")
#=> #<GraphQL::Client::Response:0x000000010abcd123
The Query class does what this lib wants, creates a class, parses the query on demand once and assigns that to a constant in the class. However, because it's just creating a new class every time, if the query doesn't change, we're parsing it every time we do Query.new.
In effect, we've broken what the DynamicQueryError error and check were design to stop - because in the end we can't control what people do. Recommendation, guides, and examples are far more powerful. e.g.,
# Assign you Queries to Constants to avoid parsing the same query
# over and over again
GetCustomers = Query.new(<<~GRAPHQL)
query($after: String) {
search {
customers(input: {}, first: 50, after: $after) {
edges {
node {
email legacyId
}
}
pageInfo {
hasNextPage endCursor
}
}
}
}
GRAPHQL
GetCustomers.execute(after: "something")
