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

Unclear how to instrument all fields with new interpreter #2087

Closed
dylanahsmith opened this issue Feb 4, 2019 · 3 comments
Closed

Unclear how to instrument all fields with new interpreter #2087

dylanahsmith opened this issue Feb 4, 2019 · 3 comments
Milestone

Comments

@dylanahsmith
Copy link
Contributor

The new Interpreter documenation says Field Instrumentation is no longer supported and suggests

To wrap resolve behaviors, try Field Extensions

However, the Field Extensions documentation doesn't show how to apply the extensions to all the fields on an object type or schema.

Shopify/graphql-batch#93 (comment) suggests that the way to do this would be to wrap the field method to provide the extension. E.g.

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

but this would miss inherited interface fields. Which can be shown with the following example

require 'graphql'

Product = Struct.new(:id, :title)

ENTITY = Product.new(1, "Shirt")

module NodeType
  include GraphQL::Schema::Interface
  field :id, ID, null: false

  def id
    "#{self.class.graphql_name}/#{object.id}"
  end
end

class InstrumentExtension < GraphQL::Schema::FieldExtension
  def before_resolve(object:, arguments:, **rest)
    # yield the current time as `memo`
    yield(object, arguments, Time.now)
  end

  def after_resolve(value:, memo:, **rest)
    puts "Time to resolve #{field.owner.graphql_name}.#{field.graphql_name}: #{Time.now - memo}"
    value
  end
end

module FieldInstrumentation
  def field(*args, extensions: [], **rest, &block)
    extensions += [InstrumentExtension]
    super(*args, extensions: extensions, **rest, &block)
  end
end

class ProductType < GraphQL::Schema::Object
  extend FieldInstrumentation
  implements NodeType
  graphql_name "Product"

  field :title, String, null: false
end

class QueryType < GraphQL::Schema::Object
  graphql_name "Query"

  field :entity, NodeType, null: true
  def entity
    ENTITY
  end
end

class Schema < GraphQL::Schema
  query QueryType
  orphan_types [ProductType]
  use GraphQL::Execution::Interpreter

  def self.resolve_type(type, obj, ctx)
    case obj
    when Product
      ProductType
    end
  end
end

query_string = <<GRAPHQL
{
  entity {
    id
    ... on Product {
      title
    }
  }
}
GRAPHQL
puts GraphQL::Query.new(Schema, query_string).result.to_h

which outputs the following when run against the master branch

Time to resolve Product.title: 1.0e-05
{"data"=>{"entity"=>{"id"=>"Product/1", "title"=>"Shirt"}}}

showing that the interface field id wasn't instrumented.

One option would be to extend all interfaces as well to hook into their field methods, however, this would prevent the use types defined in more generic libraries, like the relay types provided by this gem.

It look like the new Interpreter uses get_field on the object's type class to get the field dynamically, so this could be wrapped to change the method definition. However, this is an undocumented method, so I'm not sure how stable this behaviour would be for future versions of this gem.

Previously, the schema was build by converting the type classes to graphql definition objects, which could have been a place to hook into (e.g. using the to_graphql method) to add extensions to the field objects. However, the new interpreter won't be dependant on this, so I'm not sure if there even is something similar to the schema build phase to hook into.

@rmosolgo rmosolgo added this to the 1.9.0 milestone Feb 5, 2019
@rmosolgo
Copy link
Owner

rmosolgo commented Feb 9, 2019

Thanks for raising this issue. It's an interesting downside to the new APIs. Of all the things that becomes easier, this becomes harder 😖 .

I understand the need for this in root types, such as Shopify/graphql-batch#93 and Subscription root fields. Are there other object types which need their fields wrapped, in practice?

To help brainstorm, I'm going to list the different ways to wrap field execution:

So, if the the only requirement for this in practice is subscriptions and mutations, could we accomplish it by investing in the convention that those fields should be created using Schema::Mutation and Schema::Subscription?

Personally, I think that's a good move. These fields are often complex (actually, I speak only about Mutation fields from experience), like Rails controller actions unto themselves, so they benefit from their own file and structure.

@dylanahsmith
Copy link
Contributor Author

Looks like tracing is generally the replacement for instrumentation of all fields in the query. It looks like that was getting GraphQL::Query::Context::FieldResolutionContext passed to it which wasn't created in the interpreter, so I didn't think that would be supported. Perhaps the interpreter documentation should mention the tracing API as the replacement for instrumenting fields across the schema.

Wanting to wrap fields all the fields for a specific object type I suspect is less common, but I think that can be done by wrapping both the field and implements class methods on the object type class to handle both owned and inherited fields. That way it would only depend on documented methods.

@rmosolgo
Copy link
Owner

👍 Glad that one looks good to you, updated the docs in 03643be !

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

No branches or pull requests

2 participants