graphql-ruby icon indicating copy to clipboard operation
graphql-ruby copied to clipboard

Passing information between IDs and interface resolution

Open eapache opened this issue 5 years ago • 10 comments

Imagine you have a single rails model

class Foo < ActiveRecord::Base
end

and two GraphQL types both implementing Node:

type Type1 implements Node { id ... }
type Type2 implements Node { id ... }

For weird implementation reasons, Foo is the backing model for both GraphQL types. Not in that Foo has some sort of attribute that tells you whether it's Type1 or Type2, but in that every Foo backs a Type1 and a Type2 in the API.

In order to implement Node, we give the two types ID schemes, e.g. gid://shopify/Type1/1 and gid://shopify/Type2/1. Note that from the API perspective these IDs refer to different objects, but at the implementation layer they end up loading the same ActiveRecord object.

As it turns out, it's really difficult to make the following GraphQL query work:

{
  node(id: "gid://shopify/Type1/1") {
    id
  }
}

This is because the concrete type gets resolved twice. First, to load the record, we get the GID, parse out the type, convert that to the appropriate underlying model, and load from the database. The resolver for the node field thus returns an instance of Foo which is correct.

However, we then have to resolve the type again. node is a field of type Node, which means we go through the interface type resolution to determine which concrete GraphQL type (Type1 or Type2) is backing this instance of Node. In this second iteration, all we have are the abstract type (Node) and the implementation object (Foo). This is insufficient information to determine the correct concrete type, since we no longer have access to the GID.

Ideally this second resolution wouldn't even need to happen. We've already resolved the type once to load the object, there's no point in trying to resolve it again. At the very least, this second step needs access to the GID so it can determine the correct concrete type.

(We've worked around this issue by always wrapping Foo in a proxy object at load time so that it can itself hold the type it was loaded to serve, but the code for that is super-gross.)

@rmosolgo @theorygeek

cc @felix-d in case I've missed anything from your case

eapache avatar Aug 23 '18 13:08 eapache

Thanks for the great write up, I agree that

Ideally this second resolution wouldn't even need to happen

I wonder how that could be accomplished 🤔

rmosolgo avatar Aug 23 '18 16:08 rmosolgo

Allow resolve methods for interfaces to call e.g. hint_concrete_type_is(Foo) and then just use that instead of running through the interface type resolution again?

eapache avatar Aug 23 '18 16:08 eapache

However, we then have to resolve the type again... all we have are the abstract type (Node) and the implementation object (Foo)

I think I'm a little fuzzy on this part. Why isn't the concrete type (Type1) also available here? Maybe I'm not understanding the place in the code that is executing.

For example, couldn't the resolver method on the interface just check self.class and see that it's Type1? Unless you're referring to the .define API...

theorygeek avatar Aug 23 '18 17:08 theorygeek

To determine which concrete type to use, the gem calls either resolve_type(obj, ctx) on the interface (in which case self is Node and obj is the instance of Foo), or resolve_type(type, obj, ctx) on the schema (in which case self is the schema, type is Node, and obj is the instance of Foo). The reference to Type1 that is generated during the resolve is discarded completely.

eapache avatar Aug 23 '18 17:08 eapache

I think what confuses me is that here, after the resolve_type call, the concrete type is preserved in that the object is wrapped in it:

image

I think you're saying that in your case, your code is falling into that else block, if you have types using the .define API. I'm just wondering if the better path here is to encourage upgrading to the class-based API.

(Source Code)

theorygeek avatar Aug 29 '18 23:08 theorygeek

We're not using the .define API, we're fully class-based at this point. The problem is in the call to resolve_type, not after it.

eapache avatar Aug 30 '18 01:08 eapache

Ooooh 🤦‍♂️ I'm sorry, I understand what you're saying now. You'd like the field-level resolver (for the node field in this case) to be able to return an object, and basically move the "wrapping" concept into the framework.

Your field resolver is essentially both returning the object, and telling the framework that it can skip the work of resolving it to a particular object type.

We're not using the .define API, we're fully class-based at this point

I am jealous :(

theorygeek avatar Aug 30 '18 22:08 theorygeek

@rmosolgo is this something that the new interpreter might have unintentionally fixed? We just noticed by accident that it seems to work now without the workaround we put in place?

edit: Nevermind, we were looking at the wrong part of our schema. I do think the the interpreter might make this a trivial fix instead of a complicated one though?

eapache avatar Jun 14 '19 19:06 eapache

I'm ... not sure 😬 What does it look like is required?

rmosolgo avatar Jun 14 '19 19:06 rmosolgo

Either https://github.com/rmosolgo/graphql-ruby/issues/1802#issuecomment-415491269, or find a way to make the arguments of the field available to the resolve_type method on interfaces or unions.

eapache avatar Jun 14 '19 19:06 eapache