Defer calling field do ... end blocks until needed

[rmosolgo]

This way, in theory, loading my_object.rb doesn't necessary load the field return types of these fields. (Rails won't try to load files until the constants are actually referenced, and in this case, it can be deferred until execution.)

cc @gmcgibbon -- would this approach work in your type-checking setup? Like this:

field :thing do 
  type Types::Thing 
end 

# or 
field :thing { type(Types::Thing) } 

# or, we could add sugar, 
# where the block's return value is used as the field's return type 
# if no return type was already configured: 
# ~~field :thing { Types::Thing } ~~
# Nevermind, that's not valid ruby :'( You could do it with params: 
field(:thing) { Types::Thing } # <- not implemented yet

I think that would give you plain Ruby constants in the source.

The downside is the need to call ensure_loaded everywhere. It's too bad and reminds me of how GraphQL-Ruby used to be full of ensure_defined, yuck 😩 . But, I think it's the right thing, and thanks to caching by the schema / Warden / Subset, calls to that method were surprisingly manageable.

ensure_loaded could even be helpful. For example, lots of field.rb checks to see if @resolver has a return type, arguments, etc, and uses those if they're given. But those checks could be done eagerly in ensure_loaded, and maybe even a freeze call to avoid surprises if the field is modified after ensure_loaded has run.

Part of #5014

TODO:

• Write up an integration test or something to make sure that this actually delivers the expected Rails behavior
• Write a Rubocop rule to migrate these automatically (excluding built-in scalars)
• Update Field#type(t) to updated connection? as needed
• Document support for this I'm going to wait and document the whole thing when it's ready

[rmosolgo]

I was able to confirm this has the desired effect in Rails by making a new app and adding fields like this to Query:

    field :things, fallback_value: [{name: "Abacus"}] do
      type [Types::Thing]
    end

    field :other_things, fallback_value: [{name: "Abacus"}] do
      type [Types::OtherThing]
    end

I also added puts ">>>> loading #{__FILE__}" to several GraphQL-related files.

Then, from GraphiQL, I made two different queries to the backend:

# first 
query {
  things { __typename }
}

# second 
query {
  otherThings { __typename }
}

In the Rails server log, I saw the desired load messages. The first query loaded thing.rb, but not other_thing.rb:

Processing by GraphqlController#execute as */*
  Parameters: {"query"=>"query {\n  things { __typename }\n}", "graphql"=>{"query"=>"query {\n  things { __typename }\n}"}}
>>>> loading /Users/rmosolgo/code/graphql_lazy_load_test/app/graphql/graphql_lazy_load_test_schema.rb
>>>> loading /Users/rmosolgo/code/graphql_lazy_load_test/app/graphql/types/query_type.rb
>>>> loading /Users/rmosolgo/code/graphql_lazy_load_test/app/graphql/types/thing.rb
Completed 200 OK in 16ms (Views: 0.3ms | ActiveRecord: 0.0ms | Allocations: 6871)

The second query loaded other_thing.rb but not thing.rb:

Started POST "/graphql" for 127.0.0.1 at 2024-08-07 16:08:40 -0400
Processing by GraphqlController#execute as */*
  Parameters: {"query"=>"query {\n  otherThings { __typename }\n}", "graphql"=>{"query"=>"query {\n  otherThings { __typename }\n}"}}
>>>> loading /Users/rmosolgo/code/graphql_lazy_load_test/app/graphql/graphql_lazy_load_test_schema.rb
>>>> loading /Users/rmosolgo/code/graphql_lazy_load_test/app/graphql/types/query_type.rb
>>>> loading /Users/rmosolgo/code/graphql_lazy_load_test/app/graphql/types/other_thing.rb
Completed 200 OK in 9ms (Views: 0.1ms | ActiveRecord: 0.0ms | Allocations: 3308)

🎉

[swalkinshaw]

We have a few places where we override field methods with a pattern like this:

def field(*args, **kwargs, some_custom_option: true, &block)
  super(*T.unsafe(args), **options).tap do |field_defn|
    field_defn.extension(SomeExtension)
  end
end

This PR no longer yields the field instance of course. Two questions:

• should ensure_loaded ever be used outside of the gem?
• is there an alternative for patterns like this and would work going forward? In some simple cases (like above), we can just add the extension directly but there's cases with much more conditional logic and options.

[rmosolgo]

Hey, sorry it wasn't working the same right out of the box. I'm sure there's something that will work for this...

What goes wrong in the example above? (I see .tap do ... which I would expect to still work fine.)

One thing that comes to mind is passing a new block to super, something like:

field(*args, **kwargs, &block)
  super(*args, **kwargs) do |field_inst|
     block.call(field_inst)
     field_inst.extension(SomeExtension)
  end 
end 

IIUC that new block would be passed to super and be used when needed.

I'd love to keep ensure_loaded a private concern to the gem. Could you share some other examples of things that don't work right after this change?

[swalkinshaw]

😓 my example was a bad one. We have a few patterns and it's actually the other one that has the issue:

def field(*args, **kwargs, some_custom_option: true)
  field_defn = super(*T.unsafe(args), **options)
  field_defn.extension(SomeExtension) if some_custom_option
  field_defn
end

super returns nil now. Moving that logic to within a block passed to super should work because the block will be called during ensure_loaded.

So I think it all makes sense and should work to support the same use cases.

[rmosolgo]

Ok, thanks for those details. I'm surprised super returns nil now ... I would expect it to still return the new instance:

graphql-ruby/lib/graphql/schema/member/has_fields.rb

Lines 11 to 15 in 5dcdb59

	def field(*args, **kwargs, &block)
	field_defn = field_class.from_options(*args, owner: self, **kwargs, &block)
	add_field(field_defn)
	field_defn
	end

I'd be happy to debug further if you don't mind sharing the full error message and backtrace. Maybe it would have another clue as to where that nil is coming from.