graphql-ruby 1.9 interpreter support

[swalkinshaw]

1.9 offers a new optional interpreter and AST-based analysis which has breaking changes. Field instrumentation won't be supported and graphql-batch uses it for mutations:

graphql-batch/lib/graphql/batch/setup.rb

Lines 12 to 25 in d1244b7

	def instrument_field(schema, type, field)
	return field unless type == schema.mutation
	old_resolve_proc = field.resolve_proc
	field.redefine do
	resolve ->(obj, args, ctx) {
	GraphQL::Batch::Executor.current.clear
	begin
	::Promise.sync(old_resolve_proc.call(obj, args, ctx))
	ensure
	GraphQL::Batch::Executor.current.clear
	end
	}
	end
	end

Ideally we'd support both and conditionally check the gem version. Here's one idea on how to support 1.9 borrowed from @rmosolgo :

def resolve_with_support(**kwargs)
  GraphQL::Batch::Executor.current.clear
  result = super
  context.schema.sync_lazy(result)
ensure
  GraphQL::Batch::Executor.current.clear
end

We'll have to figure out a good way of hooking into resolve_with_support. I'm not sure it's possible without some manual intervention (including a module for example).

[rmosolgo]

including a module for example

That's what came to mind for me too, the library requires the same thing for subscription root in the new runtime:

https://github.com/rmosolgo/graphql-ruby/blob/master/spec/graphql/subscriptions_spec.rb#L112-L114

https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/subscriptions/subscription_root.rb

[dylanahsmith]

I'm not sure it's possible without some manual intervention (including a module for example).

Possibly the module could be included automatically, although perhaps that is too invasive. At least the field instrumentation is only needed for the mutation root object type.

the library requires the same thing for subscription root in the new runtime

Currently that works through overriding field. That doesn't seem like a general replacement for field instrumentation, since it would miss interface fields. However, in this case it would only assume that the mutation root doesn't implement any interfaces, which seems likely to be true.

[swalkinshaw]

That subscription root example is a little different I think since it works via field extensions. The example I had above just overrides resolve_with_support so it would need to be included in a base mutation (and the gem recommends always inheriting from your own base classes).

class BaseMutation < GraphQL::Schema::Mutation
  include GraphQL::Batch::Mutation
end

class ProductCreate < BaseMutation
  # etc
end

[dylanahsmith]

That is just a less generic replacement for field instrumentation which relies on an external resolver to be used. As https://graphql-ruby.org/mutations/mutation_root.html says

This object is defined like any other GraphQL object

so that could be a problem if the field isn't defined using a mutation class.

[swalkinshaw]

so that could be a problem if the field isn't defined using a mutation class.

That's a good point although I do wonder how much we should cater to that non-standard use case. I guess it depends what the alternatives look like. I guess it's not much different:

module GraphQL::Batch::Mutation
  def field(*args, extensions: [], **rest, &block)
    extensions += [GraphQL::Batch::Mutation::Extension]
    super(*args, extensions: extensions, **rest, &block)
  end

  class Extension < GraphQL::Schema::FieldExtension
    def before_resolve(object:, arguments:, context:)
      GraphQL::Batch::Executor.current.clear
      yield(object, arguments, nil)
    end

    def after_resolve(object:, arguments:, context:, value:, memo:)
      GraphQL::Batch::Executor.current.clear
      value
    end
  end
end

class MutationRoot < BaseObject
  extend GraphQL::Batch::Mutation
end

@rmosolgo I'm a little hazy on extensions, but is that all that's needed? Or do we still need to explicitly call sync_lazy somewhere? I figured it's going to happen during field resolution anyway.

[dylanahsmith]

I'm pretty sure the result will be synced automatically. The reason why GraphQL::Batch::Setup does it explicitly is so that it can call GraphQL::Batch::Executor.current.clear after the sync in case anything is loaded using a batch loader as part of the mutation, but before data is actually mutated.

[swalkinshaw]

What if the gem offered hooks around sync_lazy? Then plugins like graphql-batch could define their own. In our case we'd only need to call GraphQL::Batch::Executor.current.clear before and after. I don't think anything exists right now which would allow for this.

After a quick look at the gem I think the main issue is the lazy related methods only take a value and not a field defn which would enable us to conditionally check if it's a mutation field.

[rmosolgo]

how much we should cater to that non-standard use case

Personally, I'm interested in exploring this more. Historically GraphQL-Ruby has suffered from too much "do it however you want" and not enough, "this is how X is done". So in my opinion, supporting custom batching in GraphQL::Schema::Mutation makes sense.

I'm not sure how to do a completely agnostic approach. Lots of options and their shortcomings are discussed here. Even dynamically, you couldn't check @field.owner == ctx.schema.mutation, because, for interface fields, @field.owner will return the interface.

Maybe you could do something like

schema.mutation.fields.each { |name, f| f.extension(MutationBatching) } 

But some of mutation.fields might be interface fields, which are shared by a few types. Then those other types would also get mutation batching. But is that likely to happen in practice?

[dylanahsmith]

I don't think we should actually worry that much about a mutation field coming from an interface. If there were a simple way to handle that, then it would be relevant though. If we want to be paranoid, then we could try to detect that happening and raise.

I would be more concerned about fields defined directly on a mutation type seemingly like they work but actually having a subtle bug from this extension not applying to them. If we don't want to support that, then perhaps we could detect that as well and raise to make this clear.

[swalkinshaw]

schema.mutation.fields.each { |name, f| f.extension(MutationBatching) }

This seems like the best option so far since it works with the existing use method too. I'll look into it 👌

[rmosolgo]

seems like the best option so far

I took a try like that over at #99, feel free to use it as inspiration (or evidence to try another approach), or I'm happy to improve it according to your review!

[dylanahsmith]

Fixed by #99