feat: entity type customization, type loader support #33

jpasquers · Apr 4, 2023

Proposed changes

Provide support for the typeLoader config option by exposing the types added for the FederatedSchema (namely, _Service, _Entity, _Any
Also Provides two different ways that we could try to prevent the EntityUnionType from doing an entire type tree scan
- Exposes the ability to provide a pre existing list of entity types (and thus bypass the type scan
- Adjusted the fallback behavior to make the entity union type types field a callable so that theoretically the underlying entity types (and therefore the type scan) only happens on analysis/interpretation of the _Entity type.
- We will have to evaluate which effect gives more of the desired performance improvement.

How to test

Unit Tests

This PR has unit tests
This PR does not have unit tests: why?

jpasquers · Apr 5, 2023

src/FederatedSchema.php

@@ -149,28 +181,12 @@ private function getQueryTypeServiceFieldConfig(): array
    /** @var array */
    private function getQueryTypeEntitiesFieldConfig(?array $config): array
    {
-        if (!$this->hasEntityTypes()) {


This is removed because it is incompatible with the notion of an invocable set of union types. I don't believe it is needed since the _entities query is always a required part of the subgraph definition

Is there a reference you can link here?

Yep: https://www.apollographql.com/docs/federation/subgraph-spec/

There are various similar links throughout the readme, this is the one that specifies you have to include _entities

README.md

tcarrio · Apr 5, 2023

src/FederatedSchema.php

@@ -4,17 +4,17 @@

 namespace Apollo\Federation;

+use Apollo\Federation\Types\AnyType;


Import ordering, this should be caught by linting

tcarrio · Apr 5, 2023

src/FederatedSchema.php

@@ -55,18 +55,30 @@
 */
 class FederatedSchema extends Schema
 {
-    /** @var EntityObjectType[] */
+    /** @var EntityObjectType[]|callable: EntityObjectType[] */


I haven't seen the callable: ReturnType type declaration before- when was this introduced?

I actually adapted it from here: https://github.com/webonyx/graphql-php/blob/master/src/Type/Definition/UnionType.php#L18

I'm not sure when it was introduced, but I think it's apt that we're mimic'ing the union type structure.

tcarrio · Apr 5, 2023

src/FederatedSchema.php

+     */
+    private function supplementTypeLoader(array $config): array
+    {
+        if (!array_key_exists('typeLoader', $config) || !is_callable($config['typeLoader'])) {


We're using the 'typeLoader' string a lot- we should share this value in a way that we understand the type signature of the config and expose that externally as well.

The typeLoader string is a constant of the PHP graphql library, we can const it but I don't think they expose a const for it (silly random associative arrays)

tcarrio · Apr 5, 2023

src/FederatedSchema.php

@@ -55,18 +55,30 @@
 */
 class FederatedSchema extends Schema


The docstring for the class has not been updated to include this new functionality. That's not required, but would be useful

tcarrio · Apr 5, 2023

src/FederatedSchema.php

+    protected ServiceDefinitionType $serviceDefinitionType;
+    protected EntityUnionType $entityUnionType;
+    protected AnyType $anyType;
+
    public function __construct($config)


What is the signature of the config array? It didn't exist before but it certainly can now, especially as we introduce even more properties.

tcarrio · Apr 5, 2023

src/FederatedSchema.php

-            if ($type instanceof EntityObjectType) {
-                $entityTypes[$type->name] = $type;
+        return function () use ($config) {
+            $resolvedTypes = TypeInfo::extractTypes($config['query']);


More magic strings for the config, +1 for annotating that config array

tcarrio · Apr 5, 2023

test/StarWarsSchema.php

            ]);
        }
        return self::$overRiddedEpisodesSchema;
    }

+    public static function getTypeLoader(): callable {


I know it's just a test, but maybe we should consider using something like containers for our test/example implementation. Using something like an array even would let us register these and look them up in a straightforward way.

Or, we simply break out an example usage separate from the test. But it feels like the test suite is actually perfect for demonstrating an example usage in this case.

Feel free to 👎

I think as it is it's okay. I think if this keeps growing though we will need some sort of TypeRegistry you're right.

Co-authored-by: Tom Carrio <tom@carrio.dev>

tcarrio · Apr 5, 2023

src/Types/EntityUnionType.php

+            'types' => is_callable($entityTypes) 
+                ? fn () => array_values($entityTypes())
+                : array_values($entityTypes)


Starting to see this pattern often enough. What we have is essential a type like MaybeLazyEntityTypes = T | () => T. We can define a common utility for working with this.

function normalizeEntityTypes($maybeLazyEntityTypes): array { return is_callable($maybeLazyEntityTypes) ? $maybeLazyEntityTypes() : $maybeLazyEntityTypes; }

Which you can use as such:

Suggested change

'types' => is_callable($entityTypes)

? fn () => array_values($entityTypes())

: array_values($entityTypes)

'types' => array_values(normalizeEntityTypes($entityTypes)),

we need to preserve the lazy nature of the operation
So we need to wrap the operation in a new lazy operation that then updates it on invocation
Which is not the same as invoking it and operating on the result.

So alas we cannot commonize this

feat: entity type customization, type loader support

1267378

jpasquers requested a review from a team April 4, 2023 21:18

ramsey previously approved these changes Apr 4, 2023

View reviewed changes

feat: lazy union type loading

35d0f30

jpasquers dismissed ramsey’s stale review via 35d0f30 April 5, 2023 02:40

jpasquers commented Apr 5, 2023

View reviewed changes

ramsey approved these changes Apr 5, 2023

View reviewed changes

tcarrio reviewed Apr 5, 2023

View reviewed changes

Update README.md

5c4110b

Co-authored-by: Tom Carrio <tom@carrio.dev>

tcarrio reviewed Apr 5, 2023

View reviewed changes

fix: type docs updating

3836591

tcarrio approved these changes Apr 5, 2023

View reviewed changes

jpasquers merged commit aec3604 into main Apr 5, 2023

jpasquers deleted the feat/expose-builtin-types branch April 5, 2023 16:25

Search code, repositories, users, issues, pull requests...

feat: entity type customization, type loader support #33

feat: entity type customization, type loader support #33

Uh oh!

Conversation

jpasquers commented Apr 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

How to test

Unit Tests

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcarrio Apr 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jpasquers commented Apr 4, 2023 •

edited

Loading

tcarrio Apr 5, 2023 •

edited

Loading