Interface lazy loading, field definitions as closure not working

[ivome]

I am having some issues with the new lazy loading of interfaces that was implemented:

I have all of my fields defined in callback functions and inside of those callback functions I have a typeRegistry that creates the types on demand. That has the massive advantage that the instances of the types are only created when they are needed.
That worked fine until the latest version when lazy loading of interfaces was introduced. The loading of the interfaces seems to be called on schema creation:

https://github.com/webonyx/graphql-php/blob/master/src/Schema.php#L36

So the types with interfaces are not known at this point in my setup, because the callback functions that create the types did not execute yet.
When the interface is called, I'm getting an error, because the types are not recognized properly.

The following example query:

query Query {
  node(id: "UmVnaW9uOjRhYWNkZTQwNzBhZWIxY2RhZTAy") {
    ... on User {
      firstname
    }
    id
  }
}

Results in an error:

{
  "data": null,
  "errors": [
    {
      "message": "Fragment cannot be spread here as objects of type \"Node\" can never be of type \"User\".",
      "locations": [
        {
          "line": 3,
          "column": 5
        }
      ]
    }
  ]
}

If I change the function getPossibleTypes to the following, it works again, at least the error is gone

    public function getPossibleTypes()
    {
        if (self::$_lazyLoadImplementations) {
            self::loadImplementationToInterfaces();
        }
        return $this->_implementations;
    }

The node is resolved properly but I don't get the fields returned from the fragments. That was also not working with the previous version v0.5.7.
So I'm wondering if there is something else wrong with the InterfaceType, possibly in combination with the definition of the fields in the callback functions. But I feel like creating them on demand would be the most performant way.
What do we need the lazy loading for, if the function loadImplementationToInterfaces is called in the Schema constructor?

Any ideas how I should fix that?

[mcg-web]

In older versions of the lib, even while using callbacks to describe interfaces, it was called when an object implemented this interface. The full explanation of the why is detail in PR #33. But your trouble seems to come from somewhere else, because we use the same implementation with latest lib version without no error. Here's the code with node relay and fragment that works https://github.com/overblog/GraphQLBundle/blob/master/Tests/Functional/Relay/Node/NodeTest.php#L72-L79. Can you provide some PHP example please?

[vladar]

First of all, there is a new branch which reflects graphql-js 0.5 with new way how interface resolution is handled. It is different now. Possible interface implementations are all moved to schema vs staying at interface object itself. It makes sense because same interface may have different implementations in different schemas (even if it is just theoretical).

So you could try this branch before applying any fixes. Maybe your case is covered there. If not - we can dig deeper.

The problem with new branch though is that it is the first version with breaking changes since the initial release and it requires some changes in app code. I planned to write changelog or migration guide on this, but didn't have a chance to do this yet.

In short - there are several breaking changes:

1. Schema constructor will accept config now vs separate arguments for query, mutation and subscription types
2. signature of GraphQL::execute and Executor::execute is changed - contextValue is added to arguments alongside rootValue
3. 3rd argument of resolve method now accepts this contextValue and ResolveInfo is now 4th argument.
4. There is a way to pass types to schema in constructor separately. It is required for cases when some object types are only used as interface implementations and are never added to schema.

This pretty much reflects breaking changes of graphql-js 0.5. Most of fixes are trivial. I plan to move this branch to master and release new version soon.

Also make sure to check #35

[ivome]

Thanks for your responses and for keeping up with the latest changes!

I created a test and a fix for that specific issue:
ivome@53928c9

I only have one test failing now:

1) GraphQL\Tests\Type\SchemaValidatorTest::testRejectsWhenAnImplementationIsNotAPossibleType
Failed asserting that true matches expected false.

/Users/ivo/Documents/workspace/opensource/graphql-php/tests/Type/SchemaValidatorTest.php:298

Does that need to fail there or could we remove that check? The interface is injected and through the lazy initialization that I added becomes a valid type... Could also be a valid behavior IMHO or am I missing something?

[vladar]

FYI I added your test class to april-2016 branch version and it worked fine without any code modifications. So I guess you will just need to update once new version is released (somewhere on week-end).

Thanks for investigating this! And we'll use your test to catch regressions if it's ok.

[ivome]

Great, then I'll just use my fork until the new version is released. Thanks for adding my test to the new branch!

[ivome]

I tried out the new branch april-2016 and upgraded my codebase. Thanks for the great work, btw!! There are still some issues with the LazyInterfaces. I updated the testcase slightly so that it fails on the new branch now.

ivome@c1897ff

Without that change the test runs fine. But it seems that right now all types that you want to call through the interface would have to be created in some other place of the Graph. Do you guys think this is solvable?
I really would love to keep the lazy loading in place and not create ALL types on each request.

[ivome]

I upgraded graphql-relay-php to the new spec-april2016 branch and had the same issues there. Here is an example:

https://github.com/ivome/graphql-relay-php/blob/feature/spec-april2016/tests/Node/NodeTest.php#L282

If I don't pass the types manually to the schema, the test fails because graphql does not initialize the types properly.

[vladar]

I see what you mean. That's exactly the reason why types option was added to schema in graphql-js and now in graphql-php.

When your type is only referenced in schema as interface implementation and is not referenced anywhere else, it remains invisible for GraphQL during validation phase.

So one way to make such types reveal themselves for GraphQL is to pass them to Schema explicitly. That's the route that graphql-js chose. Another option would be to allow interface define the list of possible implementations as a hint.

For now I keep this library as close as possible to graphql-js, so we also have types option for this purpose now. But the other option seems viable as well and may give us better granularity for types initialization some day.

I will create separate feature request for this. But if you have other ideas on how to make this work better, please let me know.

[vladar]

Also note that it is not really lazy initialization of implementors, but rather late or deferred one. All types, including all implementors are initialized on schema creation anyway.

This is something I want to improve one day, but now there is no on-demand or lazy initialization of types.

[ivome]

Thanks for the explanations. I guess late or deferred initialization is probably the better term.

Too bad that PHP cannot keep the thread alive so that all the types have to be initialized on each request....
Your proposal in #40 sounds like a viable solution. But for now it also works with passing all types to the schema, even though it's a little bit slower.
I haven't fully built my graph yet, so it might be the case that all the types have to be initialized anyways once I have all the fields in place.
Right now there is a difference of > 10ms that gets added to each request with my initialization logic compared to deferred initialization. I haven't done any further optimizations yet though.

[smolinari]

Too bad that PHP cannot keep the thread alive so that all the types have to be initialized on each request....

@ivome - This is exactly the reason why we've chosen to work with appserver.io. http://appserver.io/

😄

Scott