rmosolgo / graphql-ruby

Ruby implementation of GraphQL
http://graphql-ruby.org
MIT License
5.38k stars 1.39k forks source link

Document how to implement field instruments in the "new runtime" #2852

Closed leehambley closed 4 years ago

leehambley commented 4 years ago

Is your feature request related to a problem? Please describe.

A number of recommended approaches for filtering fields and working with directives no longer work. Sources are all fragments of code in older GitHub issues (12-18 months).

Suspected that some or all of this functionality isn't ported to the "new runtime".

(For a variety of reasons I cannot use the Ruby DSL so most of the documentation doesn't apply since it assumes access to features that someone using the GraphQL IDL doesn't have)

Two key examples:

Describe the solution you'd like

Accurate documentation and/or useful code comments that help navigate what does or doesn't work in the "new runtime" (Excuse the quotes, I just haven't understood what the difference is)

Describe alternatives you've considered

Presently I'm trying to divine enough enough information from a query instrument to implement the same functionality, but this requires me to learn (and the docs are inadequate) what I can find on the incoming query

Additional context

Looking at the code and tests hasn't yielded much, it seems like most of the tests assume the old runtime, so they tests don't differentiate between old and new behaviour (i.e even passing specs from this repo aren't useful in figuring out what should work, or not)

What am I trying to achieve?

Implement one or more middlewares (instruments) which alter the behaviour of the incoming query, to:

What have I tried?

# frozen_string_literal: true

require 'pry'
require 'test_helper'

# https://github.com/rmosolgo/graphql-ruby/issues/544#issuecomment-279786646
# https://github.com/rmosolgo/graphql-ruby/issues/2122#issuecomment-470120530

module SentinelFieldDirectiveInstrument
  module_function
  def instrument(type, field)
    # binding.pry
    raise "boom"
  end
end

module SentinelQueryDirectiveInstrument
  module_function
  def before_query(query)
    # binding.pry
  end
  def after_query(_query)
  end
end

class TestSch3maDirectiveReflection < Minitest::Test
  def definition
    <<~EOSCHEMA
      directive @sentinel on FIELD_DEFINITION
      type Response {
        msg: String!
      }
      type Query {
        echo: Response! @sentinel
      }
    EOSCHEMA
  end

  # rubocop:disable Metrics/MethodLength
  def resolvers
    Sch3ma.default_resolvers.merge(
      {
        'Query' => {
          'echo' => lambda { |_obj, args, _ctx|
            {msg: "hello world"}
          }
        },
        "Response" => {
          "msg" => lambda { |obj, args, _ctx|
            obj[:msg]
          }
        }
      }
    )
  end
  # rubocop:enable Metrics/MethodLength

  def schema
    GraphQL::Schema.from_definition(
      definition,
      default_resolve: resolvers
    ).tap do |s|
        # binding.pry
        s.instrument :field, SentinelFieldDirectiveInstrument
        s.instrument :query, SentinelQueryDirectiveInstrument
    end
  end

  def test_reflecting_on_directives_introspection_query
    res = schema.execute(<<~EOQUERY)
      __schema { types { ... } }
    EOQUERY
    puts res['data']
  end

  def test_reflecting_on_directives
    res = schema.execute(<<~EOQUERY)
      query {
        echo { msg }
      }
    EOQUERY
    assert_equal res['data']['echo']['msg'], "hello world"
  end

end
swalkinshaw commented 4 years ago

Hide fields from the introspection query based on some metadata from the context.

The visibility feature does this: https://graphql-ruby.org/authorization/visibility.html

Though it applies to all operations, not just introspection queries (which I'm assuming is fine since if you don't want somehow to see a type in introspection they shouldn't be able to during a "normal" operation either?).

leehambley commented 4 years ago

Thanks @swalkinshaw I'm familiar with the visible? API but it has a big problem (two problems?):

Maybe this is two problems because it's a) can't do this on a per-field basis, and b) don't have access to enough context if I would monkeypatch ID to know if I'm foo {ID} or bar {ID}


Maybe it's possible to build a query instrument using before_query, walk the AST and dynamically redefine fields I want to hide by monkeypatching in a def visible?; false; end into them?) I'll try that now briefly.

swalkinshaw commented 4 years ago

Visibility can be set on fields like this:

class Types::BaseField
  attr_accessor :visibility

  def initialize(*args, **kwargs, visibility: nil)
    @visibility = visibility
     super
  end

  def visible?(context)
    # any logic using @visibility
  end
end
class SomeType < Types::BaseObject
  field :foo, String, visibility: :hidden
end
leehambley commented 4 years ago

Visibility can be set on fields like this:

Thanks but I'm not using (can't use) the Ruby DSL, I'm using Schema.from_definition so I have no good place to specify Ruby specifics like this.

I tried reopening GraphQL::Language::Nodes::Field to define the visible? method as you suggested, but neither that class name or Types::BaseObject seem to be the right place to hook into this.

I posted a more-or-less complete WIP test of my current approach in the initial report, perhaps you can suggest what class I should reopen to implement visible? (and the same, I assume will hold true for the authorized?)

Thanks for pointing me in the right direction @swalkinshaw

swalkinshaw commented 4 years ago

Ah sorry I wasn't sure if you used the SDL originally just for the example. Unfortunately you're probably going to run into a lot of issues using the SDL (from_definition) approach since this library is designed to write schemas code-first so there's not feature parity between the two. And all examples will be in Ruby as well as you've found.

You might be better off customizing https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/schema/build_from_definition.rb instead of the types in the schema after its built?

leehambley commented 4 years ago

Ah sorry I wasn't sure if you used the SDL originally just for the example. Unfortunately you're probably going to run into a lot of issues using the SDL (from_definition) approach since this library is designed to write schemas code-first so there's not feature parity between the two. And all examples will be in Ruby as well as you've found.

Indeed, in fact it's dire enough that I even wonder if Ruby is the right language at all. All of this that I need to do is trivial in the Apollo-derived land in Node.js.

I hadn't realized that the GraphQL IDL support in this Gem was essentially unmaintained, and just here for backwards compatibility, and had mostly assumed it was simply a case of documentation lagging behind this change to the new runtime (which to the best of my knowledge exists to make better use of caching and lazy loading).

:warning: I may even have noticed that directives aren't properly parsed by the from_definition at least the AST node for echo @sentinel above claims to have no directives when I examine the field/ast_node in the query instrument code :(

You might be better off customizing https://github.com/rmosolgo/graphql-ruby/blob/master/lib/graphql/schema/build_from_definition.rb instead of the types in the schema after its built?

I regret that I may have to, or try and find some place (in the query instrumenter) that will give me access to the context, incoming query AST and the schema AST so I can compare their directives/accesses/context/scopes/etc.

Thanks for all your suggestions, I'll keep digging :pick:

rmosolgo commented 4 years ago

tests don't differentiate between old and new behaviour

Funny, I think of that as a big feature. I had to migrate a big app from the old runtime to the new one, and the fewer behavior differences, the better! (I still ended up with one bug, but not too bad.)

cannot use the Ruby DSL

Yes, as @swalkinshaw pointed out, you're off the beaten track 😅 Personally, I only parse schemas from definition for tooling (like https://github.com/xuorig/graphql-schema_comparator), but I know some other folks build runable schemas that way.

I hadn't realized that the GraphQL IDL support in this Gem was essentially unmaintained, and just here for backwards compatibility

I haven't realized that either! Where did you learn that? I maintain that behavior, but I don't push new features, because I don't use it.

customizing build_from_definition.rb

I agree with @swalkinshaw's leaning that improvements to that file would be the best approach.

Specifically, I think the way to go would be supporting custom base classes. If you look at the source, you'll see it always uses the built-in base classes (GraphQL::Schema, GraphQL::Schema::Object, GraphQL::Schema::Union, etc), so there's not a good opportunity to redefine methods like def visible?.

However, it could be improved to somehow take custom base objects, then you could do the kind of extensions you're talking about.

trivial in the Apollo-derived land in Node.js

Sounds like it's worth a try!

⚠️ I may even have noticed that directives aren't properly parsed

I haven't noticed that! This works fine for me:

schema = GraphQL::Schema.from_definition <<-GRAPHQL 
type Query {
  echo: String @sentinel 
}
GRAPHQL

p schema.query.fields["echo"].ast_node.directives.map(&:name)
# => ["sentinel"]

If it doesn't work for you, please open a new issue with what you find!

leehambley commented 4 years ago

Prefix: I appreciate all your work on this gem, and hope that with a little :hammer: it can be bent into the shape I need, I hope any implied bad feeling in this thread can be written off as a hazard of long-form sniping over text based comms on the internet!

Funny, I think of that as a big feature. I had to migrate a big app from the old runtime to the new one, and the fewer behavior differences, the better! (I still ended up with one bug, but not too bad.)

I'd agree, except that it's not clear when digging in the tests that something I'm looking at working sample code for won't actually work in the real world because I used from_definition not the DSL.

as @swalkinshaw pointed out, you're off the beaten track sweat_smile

I find that really surprising, I consider the interoperability around the IDL as the absolute core principle feature of GraphQL, something to be cherished. I'm sure the decision to make it a 2nd class citizen wasn't an easy one. I inferred a bit from reading the issues in this project that there were some limitations that couldn't be worked around.

I haven't realized that either! Where did you learn that?

It doesn't support most of the documented features, and it doesn't come with any formal deprecation warnings, it definitely "feels" like it's just been left by the wayside, and not explicitly "maintained" or deprecated. I'll concede though that using the IDL seems to be unusual (which I find really surprising, given my love for it), so maybe my perception is just wildly different to others who use (and maintain) the gem.

I agree with @swalkinshaw's leaning that improvements to that file would be the best approach.

Great, thanks for the extra motivation to go explore that method <3

However, it could be improved to somehow take custom base objects, then you could do the kind of extensions you're talking about.

Definitely something I'm going to look into. I may also look into some general purpose modifications to see if I can define directives that my specific base-classes could use. Either something to hint the schema builder which class to use @rubyHint(class: "Foo::Bar") or something like @visible(method: ".."), if

trivial in the Apollo-deried land in Node.js

Sounds like it's worth a try!

Indeed! I've had great success with the visitor pattern they document for modifying the AST at runtime for directives https://www.apollographql.com/docs/graphql-tools/schema-directives/#implementing-schema-directives

I expected to find something similar to this in Ruby land, but I can see now that a greater focus has been placed on OO/DSL concepts which I more comfortable for most Rubyists.

Having seen in this gem that the AST was exposed I naïvely assumed something similar would be possible, especially when in a query instrument the query AST is available.

Whilst we're back-and-forth in dialogue here maybe it's worth refocusing the discussion, I made a couple of observations, in a query instrument query.schema.ast_node is always nil for me, I assumed, especially as my schema is parsed from a document (rather than assembled with the DSL) that I'd get access to the root of the AST from here, and be able to write my own visitors that ran as part of the query instrument process.

If it doesn't work for you, please open a new issue with what you find!

My mistake, I had compared #inspect output of the incoming query document, not the underlying schema, thanks, can confirm this was a red herring:


[12] pry(#<Class>)> q.document
=> #<GraphQL::Language::Nodes::Document:0x0000000002f9e1e0
 @definitions=
  [#<GraphQL::Language::Nodes::OperationDefinition:0x0000000002f9e898
    @col=1,
    @directives=[],
    @filename=nil,
    @line=1,
    @name=nil,
    @operation_type="query",
    @selections=[#<GraphQL::Language::Nodes::Field:0x0000000002f9f298 @alias=nil, @arguments=[], @col=9, @directives=[], @filename=nil, @line=1, @name="echo", @selections=[]>],
    @variables=[]>],

(empty directives @directives=[] here indicates no directives on the incoming query, not on the underlying field)

leehambley commented 4 years ago

Quick edit, my preferred approach here would definitely be something like a query instrument that can visit all the nodes in the AST with access to the context (which we have) and do runtime redefinition of things.

Is it possible to do something like this? Assume I have a class with visit_field visit_enum, etc, what would be my entrypoint to begin to walk the query.schema.<ast_node> (which for me is always nil)

swalkinshaw commented 4 years ago

GraphQL server implementations are really split between two philosophies: code-first and schema/SDL-first.

Usually they only do one of them (or specialize in one). So while graphql-ruby supports building a schema from SDL, it's meant as a utility tool and not as the primary way to build a schema with resolver behaviour.

You'll find a lot of JS implementations are SDL-first but there's code-first ones like https://nexus.js.org/ as well.

rmosolgo commented 4 years ago

query.schema.ast_node is always nil for me

If the schema isn't defined in the IDL, then there's no AST node for it:

implicit_schema = GraphQL::Schema.from_definition <<-GRAPHQL
type Query {
  f: Int 
}
GRAPHQL
p implicit_schema.ast_node 
# => nil 

However, if the schema is present in the IDL, then its AST node is present:

explicit_schema = GraphQL::Schema.from_definition <<-GRAPHQL
type Query {
  f: Int 
}
schema {
  query: Query 
}
GRAPHQL
p explicit_schema.ast_node
# => #<GraphQL::Language::Nodes::SchemaDefinition:0x00007f9ce9a55ab8 @definition_line=4, @line=4, @col=1, @filename=nil, @query="Query", @mutation=nil, @subscription=nil, @directives=[]>

runtime redefinition of things

What things are you trying to redefine? If you mean redefining the schema on-the-fly, there's no built-in support for that. (.visible?, discussed above is the closest thing.)

what would be my entrypoint to begin to walk ...

There are two classes like this:

I hope one of those helps!

leehambley commented 4 years ago

Thanks for all these suggestions/pointers and discussion @swalkinshaw and @rmosolgo .

I think I understand all the trade-offs and limitations now, and regrettably if I understand an AST analyzer/etc is the wrong place to do a runtime redefinition of visible? or accessible? on a field based on the context (looks like it has no access to the context) anyway.

Seems like the only viable approach is to hook into the schema builder class and make it use different classes. Thanks!

rmosolgo commented 4 years ago

looks like it has no access to the context

The passed-in visitor exposes the query:

https://graphql-ruby.org/api-doc/1.10.5/GraphQL/Analysis/AST/Visitor.html#query-instance_method

So, you can get it there:

visitor.query.context

Glad the discussion was helpful!