Replace static variables and statically referenced types with instance based TypeRegistry #1424

ruudk · 2023-08-08T06:49:34Z

First of all, we ❤️ this amazing library. We've been using it since early 2017 and have 2 large schema's consisting over 1000+ types.

The library sometimes uses static variables to store objects that it creates. This way, the object is instantiated once, and then returned from cache.

graphql-php/src/Utils/Utils.php

Lines 13 to 15 in 86d5a65

    
           static $undefined; 
        
           return $undefined ??= new \stdClass();

graphql-php/src/Language/Visitor.php

Lines 344 to 346 in 86d5a65

    
           static $skipNode; 
        
           return $skipNode ??= new VisitorSkipNode();

graphql-php/src/Language/Printer.php

Lines 76 to 77 in 86d5a65

    
           static $instance; 
        
           $instance ??= new static();

The above examples are fine, because these objects don't hold state that can ever change.

But it becomes a problem when this happens:

graphql-php/src/Type/Definition/Type.php

Lines 114 to 119 in 86d5a65

    
           static $builtInTypes; 
        
           return $builtInTypes ??= \array_merge( 
        
               Introspection::getTypes(), 
        
               self::getStandardTypes() 
        
           );

Here, the Introspection types (several objects, enums, and referenced built in types) and the standard types are stored statically.

Same here:
https://github.com/webonyx/graphql-php/blob/86d5a659445225c8cbf410ac596090e3ebb0752c/src/Type/Introspection.php#L51C1-L52

And here:

graphql-php/src/Executor/ReferenceExecutor.php

Lines 664 to 667 in 86d5a65

    
           static $schemaMetaFieldDef, $typeMetaFieldDef, $typeNameMetaFieldDef; 
        
           $schemaMetaFieldDef ??= Introspection::schemaMetaFieldDef(); 
        
           $typeMetaFieldDef ??= Introspection::typeMetaFieldDef(); 
        
           $typeNameMetaFieldDef ??= Introspection::typeNameMetaFieldDef();

Let's say you have multiple independent schema's. One of the schema's uses a Type::overrideStandardTypes to replace the standard String
type with a custom one. This now becomes a problem depending when the second schema loads. Because it uses statically cached types, they reference to a different String type, resulting in:

Found duplicate type in schema at Viewer.__typename: String. Ensure the type loader returns the same instance. See https://webonyx.github.io/graphql-php/type-definitions/#type-registry. in /Volumes/CS/www/cosmos/vendor/webonyx/graphql-php/src/Executor/ReferenceExecutor.php:878
 
 Caused by
 AssertionError: Found duplicate type in schema at Viewer.__typename: String. Ensure the type loader returns the same instance. See https://webonyx.github.io/graphql-php/type-definitions/#type-registry.

This problem pops up when running our test suite. When I run a single test, everything is fine. But when I run multiple tests together, every time the Schema loads, it gets a new instance of our standard String type. Even if I would also statically cache this default type, the problem with also happen when I run 2 tests that both operate on a different schema. Therefore I think the static references are not the way to go.

I tried to create a PR to fix this, but it's a bit hard for me. Some code directly references methods on Introspection that return the cached definitions.

What I would like to see for a next version of this library, is that instead of statically referenced types we switch to an instance based type registry.

For example:

interface TypeRegistry {
  public function (protected callable $typeLoader);
  public function get($typeName) : Type&NamedType;
  public function nonNull($wrapped) : NonNullType;
  public function listOf($wrapped) : ListOfType;
  public function string() : ScalarType;
  public function int() : ScalarType;
  // ... etc
}

class DefaultTypeRegistry implements TypeRegistry {
  protected $resolvedTypes = [];  
  // ...
}

$typeRegistry = new DefaultTypeRegistry(
    typeLoader: function(string $typeName): Type&NamedType {
        return match ($typeName) {
            'Boolean' => new BooleanType(),
            'Float' => new FloatType(),
            'ID' => new IDType(),
            'Int' => new IntType(),
            'String' => new StringType(),
            'Query' => new QueryType(),
            'HelloWorld' => new HelloWorldType(),
        }
    }
);

$schema = new Schema([
    'typeRegistry' => $typeRegistry
]);

Instead of referencing Type::string() to get a StringType, you use $typeRegistry->get('String') that returns that instance.

If needed, we can create specific methods for standard types like $typeRegistry->string().
The Type::listOf and Type::nonNull can also move to this registry: $typeRegistry->listOf($typeRegistry->nonNull($typeRegistry->string())).

The Introspection class becomes an instance within the Schema. The Schema can instantiate it like $this->introspection = new Introspection($this->typeRegistry);.

The statically called Directive also switched to an instance created inside the Schema.

Since every Schema has their own registry, there are no collisions when multiple schema's coexist.

The Type::overrideStandardTypes can be removed, as this is now controlled in your type loader. It will first go to the type loader to load String. It can fallback to a default StringType later.

I also think that Type::getStandardTypes can be removed, as it doesn't really matter if something is standard or not. Just always go through the type loader and cache it inside the instance of the type registry.

The text was updated successfully, but these errors were encountered:

ruudk · 2023-08-10T18:06:41Z

@spawnia if you have time I'd like to hear your thoughts on this. 😊

shmax · 2023-08-11T21:57:19Z

I tried to create a PR to fix this, but it's a bit hard for me. Some code directly references methods on Introspection that return the cached definitions.

Why not post your PR as a draft? Then you can point out the bits that don't work, and maybe someone will be able to help you work out the kinks...

ruudk · 2023-08-12T06:45:00Z

@shmax I first want to know if this is a direction to be considered. Then I'd be happy to spend time on it and come up with a PR.

Currently, types are referenced and cached statically. This is problematic when using multiple schema's that have different standard types that share the same memory. For example, when running them in un-isolated unit tests or when there is a long running PHP process that serves GraphQL requests. To solve this problem, we introduce a `StandardTypeRegistry` interface with a `DefaultTypeRegistry` implementation. People are allowed to create their own registries by implementing the interface. Every Schema should be constructed with a `typeRegistry` that is an instance of `StandardTypeRegistry`. From there on, all types are queried from the registry. The registry will be responsible for caching the types to make sure subsequent calls to the same type will return the same instance. Internally, all static calls to the standard types (Type::string(), Type::int(), Type::float(), Type::boolean(), Type::id()) have been replaced with dynamic calls on the type registry. Also calls to the introspection objects and internal directives are using the type registry now. As most people probably have only one schema, we keep the static methods on the Type call. These now forward to `DefaultTypeRegistry::getInstance()`. This way, we don't break existing installations. The reason for creating a `StandardTypeRegistry` interface as opposed to just a non-final implementation is that it allows people to use composition instead of inheritance to extend the functionality of the registry. For example, in my project I'd like to have a registry that holds all types and that allows me to query any type by their name (instead of FQCN). I can then delegate the interface methods to the decorated `StandardTypeRegistry`. Resolves webonyx#1424

ruudk · 2023-08-14T08:56:33Z

@shmax @spawnia Created a PR to resolve this issue: #1426

Currently, types are referenced and cached statically. This is problematic when using multiple schema's that have different standard types that share the same memory. For example, when running them in un-isolated unit tests or when there is a long running PHP process that serves GraphQL requests. To solve this problem, we introduce a `StandardTypeRegistry` interface with a `DefaultTypeRegistry` implementation. People are allowed to create their own registries by implementing the interface. Every Schema should be constructed with a `typeRegistry` that is an instance of `StandardTypeRegistry`. From there on, all types are queried from the registry. The registry will be responsible for caching the types to make sure subsequent calls to the same type will return the same instance. Internally, all static calls to the standard types (Type::string(), Type::int(), Type::float(), Type::boolean(), Type::id()) have been replaced with dynamic calls on the type registry. Also calls to the introspection objects and internal directives are using the type registry now. As most people probably have only one schema, we keep the static methods on the Type call. These now forward to `DefaultTypeRegistry::getInstance()`. This way, we don't break existing installations. The reason for creating a `StandardTypeRegistry` interface as opposed to just a non-final implementation is that it allows people to use composition instead of inheritance to extend the functionality of the registry. For example, in my project I'd like to have a registry that holds all types and that allows me to query any type by their name (instead of FQCN). I can then delegate the interface methods to the decorated `StandardTypeRegistry`. Resolves webonyx#1424

ruudk mentioned this issue Aug 12, 2023

Do not store standard types statically but keep them on the Schema instead #1183

Closed

ruudk linked a pull request Aug 14, 2023 that will close this issue

Introduce StandardTypeRegistry #1426

Open

spawnia added the enhancement label Aug 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace static variables and statically referenced types with instance based TypeRegistry #1424

Replace static variables and statically referenced types with instance based TypeRegistry #1424

ruudk commented Aug 8, 2023 •

edited

ruudk commented Aug 10, 2023

shmax commented Aug 11, 2023

ruudk commented Aug 12, 2023

ruudk commented Aug 14, 2023

Replace static variables and statically referenced types with instance based TypeRegistry #1424

Replace static variables and statically referenced types with instance based TypeRegistry #1424

Comments

ruudk commented Aug 8, 2023 • edited

ruudk commented Aug 10, 2023

shmax commented Aug 11, 2023

ruudk commented Aug 12, 2023

ruudk commented Aug 14, 2023

ruudk commented Aug 8, 2023 •

edited