Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interfaces stopped working #768

Closed
murtukov opened this issue Jan 25, 2021 · 12 comments
Closed

Interfaces stopped working #768

murtukov opened this issue Jan 25, 2021 · 12 comments
Labels

Comments

@murtukov
Copy link
Contributor

murtukov commented Jan 25, 2021

After updating to 14.5.0 I started to see the error:

Runtime Object type "User" is not a possible type for "NodeInterface"

Simplified schema:

final class QueryType extends ObjectType
{
    public function __construct($typeRegistry)
    {
        $config = [
            'name' => 'Query',
            'fields' => fn() => [
                'allObjects' => [
                    'type' => Type::listOf($typeRegistry->get('NodeInterface')),
                    'resolve' => function ($value, $args, $context, $info) {
                        // ...
                    },
                ],
            ],
        ];
        
        parent::__construct($config);
    }
}

final class UserType extends ObjectType
{   
    public function __construct($typeRegistry)
    {
        $config = [
            'name' => 'User',
            'fields' => fn() => [
                'id' => [
                    'type' => Type::nonNull(Type::id()),
                    'resolve' => function ($value, $args, $context, $info) {
                        // ...
                    },
                ],
                'name' => Type::string(),
            ],
            'interfaces' => fn() => [
                $typeRegistry->get('NodeInterface'),
            ],
        ];
        
        parent::__construct($config);
    }
}

final class NodeInterfaceType extends InterfaceType
{
    public function __construct($typeRegistry, $typeResolver)
    {
        $config = [
            'name' => 'NodeInterface',
            'fields' => fn() => [
                'id' => Type::nonNull(Type::id()),
            ],
            'resolveType' => fn($value, $context, $info) => $typeResolver->resolve($value),
        ];
        
        parent::__construct($config);
    }
}

Query:

{
  allObjects {
    id
  }
}

This is a simplified example, but the thing is it was working before I updated from 14.4.0 to 14.5.0. Seems like it doesn't see anymore that the User type implements the NodeInterface interface

@vladar
Copy link
Member

vladar commented Jan 25, 2021

CC @Kingdutch any thoughts? Somehow this was not caught by tests?

@vladar vladar added the bug label Jan 25, 2021
@Kingdutch
Copy link
Contributor

I don't see anything wrong directly. The only hypothesis that I can come up with is that the type or implementation map is being made too early, which causes the implementation check to fail.

The simplest method to verify that would be to comment out if (! isset($this->implementationsMap)) { in src/Type/Schema.php on line 451 along with its closing parenthesis on line 481.

If that solves the issue then it could possibly be related by the instantiation order of the used $typeRegistry 🤔

@murtukov
Copy link
Contributor Author

@Kingdutch just for the info: the error appears in the tests of the library i'm working on (overblog/GraphQLBundle), which is working for years now. I don't think the problem lies with our library, but I tried to comment out the line you told me to and it didn't solve the issue. I'll continue to investigate it and will try to reproduce the error in a more simplified environment.

@Kingdutch
Copy link
Contributor

@murtukov Do you have a link to where the test is failing? I can't find an update branch/test run in the repo you linked.

Being able to look at the failing test can help to see if we can develop a failing test for the graphql-php library as well.

@murtukov
Copy link
Contributor Author

@Kingdutch

here is the PR: overblog/GraphQLBundle#786. You can open details of any "CI / PHP" test, they all fail of the same reason.

Meanwhile I tried to create an empty project with webonyx/graphql-php 14.5.0 and defined exactly the same schema as described in this issue and it works as expected.

@Kingdutch
Copy link
Contributor

Kingdutch commented Jan 25, 2021

Meanwhile I tried to create an empty project with webonyx/graphql-php 14.5.0 and defined exactly the same schema as described in this issue and it works as expected.

That seems to indicate that it's not in this library but in the consuming code. Possibly an assumption that is no longer true?

I see three test failures.

/~https://github.com/overblog/GraphQLBundle/pull/786/checks?check_run_id=1758427937#step:13:1782
This is expected since the interfaces field is now valid for interfaces so the description has been updated.

/~https://github.com/overblog/GraphQLBundle/pull/786/checks?check_run_id=1758427937#step:13:1799
I'm not sure what the internal server error is, so I'm not sure how to further explore this.

/~https://github.com/overblog/GraphQLBundle/pull/786/checks?check_run_id=1758427937#step:13:1933
For this test I've traced the schema to global types which defines NodeInterface as relay-node. From my limited understanding the RelayProcessor maps this to NodeDefinition which implements MappingInterface which doesn't implement any further interfaces.

This means that a relay-node defined this way doesn't implement this library's InterfaceType which would be used by this library.

I think that would explain your spread check failure. Your internal server error is then possibly caused by the strict type check for implementsInterface which was previously implicit.

@murtukov
Copy link
Contributor Author

/~https://github.com/overblog/GraphQLBundle/pull/786/checks?check_run_id=1758427937#step:13:1782
This is expected since the interfaces field is now valid for interfaces so the description has been updated.

This check is already fixed by me locally, so it isn't an issue RN.


/~https://github.com/overblog/GraphQLBundle/pull/786/checks?check_run_id=1758427937#step:13:1799
I'm not sure what the internal server error is, so I'm not sure how to further explore this.

The internal server error is what I described in this issue:
[critical] [GraphQL] GraphQL\Error\InvariantViolation: Runtime Object type "User" is not a possible type for "NodeInterface".[0] (caught throwable) at /vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php line 1287.


/~https://github.com/overblog/GraphQLBundle/pull/786/checks?check_run_id=1758427937#step:13:1933
For this test I've traced the schema to global types which defines NodeInterface as relay-node. From my limited understanding the RelayProcessor maps this to NodeDefinition which implements MappingInterface which doesn't implement any further interfaces.

The GraphQLBundle generates PHP classes, that are compatible with the webonyx/graphql-php's type system. The NodeDefinition class is just one of the intermediate builders that are used to generate the final result. And the final result is what I provided in the first post (which I edited for the sake of simplicity). Here is the final NodeInterfaceType class, which, as you can see, does implement the InterfaceType:

<?php

namespace Overblog\GraphQLBundle\RelayGlobal\__DEFINITIONS__;

use GraphQL\Type\Definition\InterfaceType;
use GraphQL\Type\Definition\Type;
use Overblog\GraphQLBundle\Definition\ConfigProcessor;
use Overblog\GraphQLBundle\Definition\GlobalVariables;
use Overblog\GraphQLBundle\Definition\Type\GeneratedTypeInterface;

/**
 * THIS FILE WAS GENERATED AND SHOULD NOT BE EDITED MANUALLY.
 */
final class NodeInterfaceType extends InterfaceType implements GeneratedTypeInterface
{
    public const NAME = 'NodeInterface';
    
    public function __construct(ConfigProcessor $configProcessor, GlobalVariables $globalVariables)
    {
        $config = [
            'name' => self::NAME,
            'description' => 'Fetches an object given its ID',
            'fields' => fn() => [
                'id' => [
                    'type' => Type::nonNull(Type::id()),
                    'description' => 'The ID of an object',
                ],
            ],
            'resolveType' => fn($value, $context, $info) => $globalVariables->get('container')->get("overblog_graphql.test.resolver.global")->typeResolver($value),
        ];
        
        parent::__construct($configProcessor->process($config));
    }
}

Anyways if the InterfaceType was the reason, then this issues would also appear before 14.5.0. Moreover all other tests in the GraphQLBundle, which work with interfaces, pass without errors.

@Kingdutch
Copy link
Contributor

Okay I see! Sorry, not familiar with your library, so trying to understand what's happening :D

Given that your simplified version does work with the graphql-php library (when not using anything else) that suggests that the error (or the cause of the error in graphql-php) is within the difference. One thing I note is that in your latest example for NodeInterfaceType you're passing parent::__construct($configProcessor->process($config));. Could you inspect the result of $configProcessor->process($config) and confirm that it's what you expect? From your example that's the difference between the minimal working form you posted at the start of this issue and the more elaborate generated form that's not working.

The reason that I ask is that if I look at what the processor does it seems to expect a plural config array (i.e. $configProcessor->process([$config])). I can see this for example in the RelayProcessor it loops over the configs. In the singular form provided to it, it appears that the processors don't do anything/may break something.

@murtukov
Copy link
Contributor Author

@Kingdutch I can assure you that config processor is not the reason and it works as expected.

I would rather draw your attention to the following. Here is what I see from Xdebug:
image
This is the generated UserType type:

final class UserType extends ObjectType
{   
    public function __construct($typeRegistry)
    {
        $config = [
            'name' => 'User',
            'fields' => fn() => [
                'id' => [
                    'type' => Type::nonNull(Type::id()),
                    'resolve' => function ($value, $args, $context, $info) {
                        // ...
                    },
                ],
                'name' => Type::string(),
            ],
            'interfaces' => fn() => [
                $typeRegistry->get('NodeInterface'),
            ],
        ];
        
        parent::__construct($config);
    }
}

As you can see the *GraphQL\Type\Definition\ObjectType*interfaces property is set to null, but if I look into the config property it does have the interface:
image

This is after all types are generated and the config processors are executed. Hope this helps.

@murtukov
Copy link
Contributor Author

murtukov commented Jan 25, 2021

@Kingdutch ok, I found the problem. The implementing types were invisible, as described here.

Don't know why the types weren't invisible before the update tho. Sorry for your time. I'm closing this issue now.

@Kingdutch
Copy link
Contributor

Glad you found the issue!

@spawnia
Copy link
Collaborator

spawnia commented Jan 25, 2021

I think the old implementation maybe loaded your types overly eager - which costs performance whenever they are not needed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants