Defining the `load_<resource>` in superclass raises `GraphQL::RequiredImplementationMissingError`

[jgrau]

Hi

First of all thank you so much for a great gem!

I've started to refactor our codebase using loads: true, as: :foo in mutation arguments. I don't use any form of global id so the ID argument I am adding those options to would just contain the id. It works as expected until I hit a mutation that uses inheritance. I figured I could define load_foo in the superclass and it would work the same way but I get GraphQL::RequiredImplementationMissingError.

The follow code

module Mutations
  module Rooms
    class Base < Mutations::Base
      def load_room(hashid)
        Room.find_by(hashid: hashid)
      end

      def authorized?(room:, **_args)
        return false, error_response('Room not found') unless room.host == viewer

        true
      end
    end
  end
end

module Mutations
  module Rooms
    class Validate < Mutations::Rooms::Base
      graphql_name 'ValidateRoom'

      argument :id, ID, required: true, loads: true, as: :room

      ....

      def resolve(room:, **args)
        ...
      end
    end
  end
end

Fails with

Failures:

  1) Mutations::Rooms::Validate updates a room as the room's host
     Failure/Error:
       result = DuneSchema.execute(
         query,
         variables: variables,
         context: graphql_context,
         operation_name: operation_name,
       )

     GraphQL::RequiredImplementationMissingError:
       DuneSchema.object_from_id(node_id, ctx) must be implemented to load by ID (tried to load from id `40e4763b1d`)
     # ./app/controllers/graphql_controller.rb:8:in `execute'
     # ./spec/support/authorization.rb:31:in `post'
     # ./spec/support/graphql_request.rb:15:in `make_request'
     # ./spec/support/graphql_request.rb:31:in `expect_query_to_equal'
     # ./spec/graphql/mutations/rooms/validate_spec.rb:50:in `block (2 levels) in <top (required)>'

Fun fact: adding

      def load_room(hashid)
        super
      end

to the child class fixes the problem.

Another fun fact is that authorize? seems to work correctly.

My system:

❯ bundle info graphql
graphql (1.9.15)

❯ ruby -v
ruby 2.6.2p47 (2019-03-13 revision 67232) [x86_64-darwin19]

[rmosolgo]

Hi, oops! Yes, I can see why that wouldn't work. The child class overrides the parent class's implementation with its own method:

https://github.com/rmosolgo/graphql-ruby/blob/05a7b028f3bdc1c277a64a75db6553a09992a7e2/lib/graphql/schema/resolver.rb#L300-L306

https://github.com/rmosolgo/graphql-ruby/blob/05a7b028f3bdc1c277a64a75db6553a09992a7e2/lib/graphql/schema/member/has_arguments.rb#L88-L90

Since that generated code uses object_from_id, we get the error like you mentioned 😖 .

How could we improve this? I can think of two options:

• First, a workaround. See how the generated method calls load_application_object? If you implemented load_application_object in your base mutation class (instead of load_thing), it would be called by argument loading methods. For example, you might implement it like:

  # called by graphql-ruby when we use `loads:` with mutation arguments 
  def load_application_object(argument_defn, object_type, value) 
  if object_type.graphql_name == "Room" 
    ::Room.find_by(hashid: value) 
  else 
    super 
  end 
  end 

  (Maybe it would require more customization than that; you could add a custom argument config like hashid: true if you need to distinguish between UUIDs and Room hashids.)

• Second, maybe we could provide an extension point customizing the generated method. Currently, there's really no way to customize them method that's generated when you use loads:. But, instead of generating the Ruby code in a string in the middle of that big method, we could call out to another method like #generate_load_method(...) which returns a string of the method body. That way, in your application, you could generate implementations differently. I don't really have the bandwidth to implement that, but if you want to take a try at it, I'd be happy to review the PR!

What do you think of those? Let me know how it goes if you try one of them!

[jgrau]

Thanks for your reply. Quick question: Would it work - and how would you like - if the generated method def load_#{arg_defn.keyword}(value) as the first thing checks if a method of the same name is already defined and if so, returns super? If my logic is correct then it would work and be a oneliner and allow for load_<resource> to be defined anywhere in the callstack.

Let me know. If you like it and believe it would work I can create the PR

[rmosolgo]

I agree it's worth a try. Eventually we might run into a problem where someone is modifying the parent class after implementing the child class. In that case, a check for the method would fail (because the super method wasn't defined yet), and then, after the super method is added to the parent class, the user would encounter the same confusing problem you found here, where the super method is not called for some reason.

But, I suppose that's unlikely and the change you suggested is still an improvement.

I still want to push for a more general solution in your application though. I mean, the current solution (def load_roam) works because all Room arguments are named room. But is there a case where that assumption won't hold true? For example, I can imagine (without knowing about your app 🙈 😆 ) a mutation which accepts two room IDs:

class Mutations::MoveThing < Mutations::BaseMutation
  argument :from_room, loads: Types::Room, # ... 
  argument :to_room, loads: Types::Room, # ... 
end 

In that case, the loader methods are load_from_room and load_to_room. Would you implement more methods in the base class? It seems like it could get messy over time.

However, using load_application_object directly would handle the case above without any change, because it doesn't care about the argument name in the child class, only the type of object being loaded. Removing that kind of "implicit communication" between parent and child class (where names must match between the two classes, but the system files silently if they don't) is an improvement in my opinion.

However, I agree in any case that failing to call super is an unexpected behavior and the change you've suggested would be an improvement. Feel free to submit a patch if you're interested!