thecodingmachine / graphqlite

Use PHP Attributes/Annotations to declare your GraphQL API
https://graphqlite.thecodingmachine.io
MIT License
557 stars 98 forks source link

Input: Cached type in registry is not the type returned by type mapper #531

Open NormySan opened 1 year ago

NormySan commented 1 year ago

Hey,

I'm having a weird issue with input types where I get the exception in the title "Cached type in the registry is not the type returned by type mapper".

For some reason, the type stored in the cache is not the same as the one from the type mapper even if the input class they both reference is the same.

The mutation that I try to run only fails when I use variables to run the mutation, If I run the mutation without variables and provide the values directly it all works but this is not possible to do in the frontend since everything is automatically generated and requires the type to be provided.

Failing mutation

mutation CreateFacility($input: FacilityInput!) {
  createFacility(input: $input) {
    id
  }
}

Working mutation

mutation CreateFacility {
  createFacility(input: {
    name: "Facility"
    subdomain: "facility"
  }) {
    id
  }
}

Input type

#[Input]
class FacilityInput
{
    #[Field]
    public string $name;

    #[Field]
    public string $subdomain;
}

Controller/Resolver

class FacilityResolver
{
    public function __construct(
        private readonly CommandBus $commandBus,
    ) {
    }

    #[Mutation]
    #[Logged]
    public function createFacility(
        #[InjectUser] Employee $employee,
        FacilityInput $input
    ): Facility {
        /** @var Facility $facility */
        $facility = $this->commandBus->dispatch(new CreateFacilityCommand(
            (string) $employee->organizationId,
            $input->name,
            $input->subdomain,
        ));

        return $facility;
    }
}

The cached type

object(TheCodingMachine\GraphQLite\Types\TypeAnnotatedObjectType)[782]
  public 'name' => string 'FacilityInput' (length=13)
  public 'description' => null
  public 'astNode' => null
  public 'config' => 
    array (size=3)
      'name' => string 'FacilityInput' (length=13)
      'fields' => 
        object(Closure)[775]
          virtual 'closure' 'TheCodingMachine\GraphQLite\Types\TypeAnnotatedObjectType::TheCodingMachine\GraphQLite\Types\{closure}'
          public 'static' => 
            array (size=5)
              ...
      'interfaces' => 
        object(Closure)[842]
          virtual 'closure' 'TheCodingMachine\GraphQLite\Types\TypeAnnotatedObjectType::TheCodingMachine\GraphQLite\Types\{closure}'
          public 'static' => 
            array (size=3)
              ...
  public 'extensionASTNodes' => 
    array (size=0)
      empty
  private 'fields' (GraphQL\Type\Definition\TypeWithFields) => null
  public 'resolveFieldFn' => null
  private 'interfaces' (GraphQL\Type\Definition\ObjectType) => null
  private 'interfaceMap' (GraphQL\Type\Definition\ObjectType) => null
  private ?string 'status' (TheCodingMachine\GraphQLite\Types\MutableObjectType) => string 'frozen' (length=6)
  private array 'fieldsCallables' (TheCodingMachine\GraphQLite\Types\MutableObjectType) => 
    array (size=0)
      empty
  private ?array 'fields' (TheCodingMachine\GraphQLite\Types\MutableObjectType) => null
  private ?string 'className' (TheCodingMachine\GraphQLite\Types\MutableObjectType) => string 'App\API\Dashboard\Input\FacilityInput' (length=37)

The type from the type mapper

object(TheCodingMachine\GraphQLite\Types\InputType)[1057]
  public 'name' => string 'FacilityInput' (length=13)
  public 'description' => null
  public 'astNode' => null
  public 'config' => 
    array (size=3)
      'name' => string 'FacilityInput' (length=13)
      'description' => null
      'fields' => 
        object(Closure)[1063]
          virtual 'closure' '$this->TheCodingMachine\GraphQLite\Types\{closure}'
          public 'static' => 
            array (size=4)
              ...
          public 'this' => 
            &object(TheCodingMachine\GraphQLite\Types\InputType)[1057]
  public 'extensionASTNodes' => null
  private 'fields' (GraphQL\Type\Definition\InputObjectType) => null
  protected string 'status' => string 'pending' (length=7)
  protected array 'fieldsCallables' => 
    array (size=0)
      empty
  protected ?array 'finalFields' => null
  private array 'constructorInputFields' => 
    array (size=0)
      empty
  private array 'inputFields' => 
    array (size=0)
      empty
  private string 'className' => string 'App\API\Dashboard\Input\FacilityInput' (length=37)
  private ?TheCodingMachine\GraphQLite\Types\InputTypeValidatorInterface 'inputTypeValidator' => null
oojacoboo commented 1 year ago

@NormySan what cache adapter are you using? I'm not familiar with this error. It looks like you're doing things correctly.

NormySan commented 1 year ago

@oojacoboo I'm using a cache pool from Symfony. Only inputs causing this problem so far, all other types seems to be working properly.

oojacoboo commented 1 year ago

From the cached type:

private ?string 'className' (TheCodingMachine\GraphQLite\Types\MutableObjectType) => string 'App\API\Dashboard\Input\FacilityInput' (length=37)

From the type mapper:

private string 'className' => string 'App\API\Dashboard\Input\FacilityInput' (length=37)

What's the stack-trace look like? How is the comparison being made?

NormySan commented 1 year ago

@oojacoboo This is the stack trace:

https://gist.github.com/NormySan/d505993a1817843f7c23aa2f5a39acc0

The exception is thrown here in the recursive type mapper:

https://github.com/thecodingmachine/graphqlite/blob/fed9ffe7d6f2e7df81d2cdf7f62de4115e87e292/src/Mappers/RecursiveTypeMapper.php#L406

oojacoboo commented 1 year ago

@NormySan do you use this same input type in multiple mutations? Can you confirm that the cached type and the type from the type mapper are effectively the same object? I think the issue here is that it's using !== instead of !=. Can you test with != and see if that resolves the issue for you?

NormySan commented 1 year ago

@oojacoboo Yeah, I'm using the input type in multiple mutations, one is for updating and one is for creating. I tried changing the comparison to just using != and I'm getting the same exception.

oojacoboo commented 1 year ago

@NormySan if you're certain in your testing that it was changed and executed (not cached), and that comparison is failing, can you identify what's different about the two objects and what's causing it to fail that comparison?

NormySan commented 1 year ago

@oojacoboo I think it's failing because they are entirely different objects, I'm going to set up a basic reproduction of my codebase, maybe that will help with identifying the underlying issue.

NormySan commented 1 year ago

Here is a simple reproduction of the exception in a simple Symfony application.

https://github.com/NormySan/graphqlite-input-error

It does not use the Symfony bundle but instead uses a custom implementation. The GraphQL request is handled by the App\GraphQL\RequestHandler class, this class is created through the App\GraphQL\RequestHandlerFactory.

NormySan commented 1 year ago

So far I've narrowed it down to the following:

  1. An attempt is made to load the FacilityInput type.
  2. All type mappers are called where it ends up in the RecursiveTypeMapper in the mapNameToType method.
  3. This method then calls the mapNameToType method on the type mapper.
  4. This results in a TypeAnnotatedObjectType being returned and registered in the type registry.
  5. Later on a call is made to the RecursiveTypeMapper to the mapClassToInputType method to get the input type.
  6. This results in an InputType object being created instead by the type mapper which is not the same type of object that was cached earlier on.
  7. Because the objects are not the same it crashes with the exception "Cached type in the registry is not the type returned by type mapper".

I'm not sure what the expected type in the cache should be but I'm guessing that it should be the InputType object and not the TypeAnnotatedObjectType object?

oojacoboo commented 1 year ago

@NormySan so, I think the issue here is that you're using the same Input type for multiple operations. I'm not going to assume this is an incorrect design decision on your part, although it very well could be. It's mostly recommended to have a unique Input type per operation. I know this is how our API is designed and likely the reason we haven't run into this error - same for most others.

That said, looking at the code, it seems clear that it's doing a direct comparison of a cached version of the Input type and the one currently being processed by the type mapper.

I don't have enough insight or feedback on the overall use of this. But, it seems like a fresh look at this caching logic needs to be had.

I don't have any answers to these questions, but it seems this is the path forward. Then we can assess a refactor of the design and PR.

NormySan commented 1 year ago

Tried using two different input types and I'm still getting the same error so must be something else. I did try to set up a very minimal test based on the no-framework instructions and having the same name for the input type worked well in this case so it must be something else in Symfony or my setup causing the problem.

If I can locate the underlying cause and if it's in this package I will try to make a PR.

pvlg commented 1 year ago

Similar issue in EnumTypeMapper.php. Need to add $enumClass = ltrim($enumClass, '\\'); in mapByClassName(). Because of the slash does not find the class in the cache.

Lappihuan commented 1 year ago

@NormySan did you try ommiting the Input suffix on the PHP Class? Its automatically added and i do remember having issues if i added it by accident.

NormySan commented 1 year ago

@Lappihuan That solved it, I would really like my inputs to be named like that though since it creates a lot of ambiguity if the input is named the same as the entities. Going to have a look at what part of the library that might be causing that issue.

NormySan commented 1 year ago

I've been trying to find a cause as to why some inputs are created as TypeAnnotatedObjectType while others are created as InputType but have yet to find a clear reason for this. The code jumps so much back and forth and the mappers are calling each other back and forth so it's very hard to get a feel for what the code is doing.

I've also been trying to find out why the name of the input might be causing issues but I can't find any reason for this either. The naming strategy looks to be working fine, this was the only place I could find where a name is being decided.

oojacoboo commented 1 year ago

@NormySan I agree it can be confusing. Have a look at this doc page for some insight into the type mapper

https://graphqlite.thecodingmachine.io/docs/internals

Also, regarding the Input name, have you tried using the following?

#[Input(name: 'FacilityInput')]
class FacilityInput
NormySan commented 1 year ago

Yeah, I read the docs and it helped a bit to get started on where to look. Tried assigning a name to the input also but didn't work. I've come to the conclusion that none of the input types in my API are working.

Lappihuan commented 1 year ago

we use input on entities, but that might now work for you

oojacoboo commented 1 year ago

We just have our inputs separate from our models, entirely, and within our GraphQL namespace along with our operations.

NormySan commented 1 year ago

I do the same, everything related to the GraphQL API:s is separate from the rest of the code to create a clear boundary between API and "Domain".

NormySan commented 1 year ago

I think I might have found something related to why the types are not the same:

  1. The RecursiveTypeMapper makes a call to the mapNameToType method here:

https://github.com/thecodingmachine/graphqlite/blob/master/src/Mappers/RecursiveTypeMapper.php#L487

  1. The result from the call is a TypeAnnotatedObjectType object that is created by the TypeGenerator when the mapAnnotatedObject method is called. This method is called from the AbstractTypeMapper in the mapNameToType method. It looks like the input is considered a regular type by this code so it never continues to the input part later on.

https://github.com/thecodingmachine/graphqlite/blob/master/src/TypeGenerator.php#L43 https://github.com/thecodingmachine/graphqlite/blob/master/src/Mappers/AbstractTypeMapper.php#L323

  1. The TypeAnnotatedObjectType has now been created and returned to the RecursiveTypeMapper. Since the type has not been cached in the type registry the type is then cached by calling the registerType method on the TypeRegistry class.

  2. Later in the code the mapClassToInputType method on the RecursiveTypeMapper is called. Here it attempts to find the type in the classToInputTypeCache but it's not there so a call is made to the mapClassToInputType method on the type mapper. This call results in an InputType object being created.

https://github.com/thecodingmachine/graphqlite/blob/master/src/Mappers/RecursiveTypeMapper.php#L393

  1. The InputType type is this time created by the mapClassToInputType method in the AbstractTypeMapper class.

https://github.com/thecodingmachine/graphqlite/blob/master/src/Mappers/AbstractTypeMapper.php#L296

  1. The RecursiveTypeMapper continues and checks if the type exists in the type registry which it does but the object that has been registered here is the TypeAnnotatedObjectType that was done previously when the mapNameToType method was called in the same class. But this time around an InputType is created instead so when a comparison is made on these objects they are not the same and the runtime exception is thrown.

My question here then is why do the mapNameToType and mapClassToInputType not generate the same end result. My expectations would be that they both return an InputType and that calling either of the methods result in an InputType being cached in the type registry.

NormySan commented 1 year ago

A change to the mapNameToType method in the AbstractTypeMapper class where it attempts to map a factory or input first solves the issue for me. No tests are failing with this change, maybe it would be good to add some regression tests to make sure this never happens again.

https://github.com/NormySan/graphqlite/commit/4a3b9ce020d869fe1a7cd134338c253caec13826

Edit: Looks like there is a failing test, will have a look at that.

oojacoboo commented 1 year ago

@NormySan instead of re-ordering the operations. I think we should focus on the checks that determine which type is being mapped where.

Are you using the Type attribute on your Input type class? Or just the Input attribute?

NormySan commented 1 year ago

@oojacoboo I'm using the Input attribute.

Here is another example repo that uses no framework that also has the same problem with the inputs as the one I linked to previously.

https://github.com/NormySan/graphqlite-no-framework

Lappihuan commented 1 year ago

@NormySan iirc there is semthing odd in nested input types i recently encountered which required me to "manualy" set inputType propery on Field/Input/Type attribute.

I did not have time to investigate the issue in depth but i suspect there is inconsitent name conversion for PHP->GQL.

NormySan commented 1 year ago

@Lappihuan Might be, I don't really have any more time to investigate the issue either, unfortunately.

mdoelker commented 1 year ago

Encountered this as well. And the issue was also mentioned in #248.

mdoelker commented 1 year ago

I did some digging and the issue was introduced as part of #458 since now the AnnotationReader::getTypeAnnotation method returns classes annotated with #[Input] as if they were #[Type] classes. Then, when mapping the type, it will end up as an output type, because that conversions happens before #[Factory] and #[Input]. I'm not sure what the change intended to solve exactly, but maybe you can shed some light on this @oojacoboo. Thanks @NormySan for the excellent and well-described minimal test repo. It made it very easy to track down.

If we revert this change from

$type = $this->getClassAnnotation($refClass, Type::class)
  ?? $this->getClassAnnotation($refClass, Input::class);

back to

$type = $this->getClassAnnotation($refClass, Type::class);

everything works as expected.

Change: https://github.com/thecodingmachine/graphqlite/pull/458/commits/83357f0e48ce0d86f1f867fb9acb26d540476a6f#diff-b4cb271cb57cae8e3c9c55d3590bab4ea0781066f699d94ac970f89f8b7e06b4R269

oojacoboo commented 1 year ago

532 has been merged. Please confirm that this issue is now resolved.