Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

feat: entity type customization, type loader support #33

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

Merged
merged 4 commits into from
Apr 5, 2023

Conversation

jpasquers
Copy link
Contributor

@jpasquers jpasquers commented 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 jpasquers requested a review from a team April 4, 2023 21:18
ramsey
ramsey previously approved these changes Apr 4, 2023
@@ -149,28 +181,12 @@ private function getQueryTypeServiceFieldConfig(): array
/** @var array */
private function getQueryTypeEntitiesFieldConfig(?array $config): array
{
if (!$this->hasEntityTypes()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reference you can link here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.md Outdated Show resolved Hide resolved
@@ -4,17 +4,17 @@

namespace Apollo\Federation;

use Apollo\Federation\Types\AnyType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import ordering, this should be caught by linting

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

protected ServiceDefinitionType $serviceDefinitionType;
protected EntityUnionType $entityUnionType;
protected AnyType $anyType;

public function __construct($config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

public static function getTypeLoader(): callable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to 👎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
Comment on lines +22 to +24
'types' => is_callable($entityTypes)
? fn () => array_values($entityTypes())
: array_values($entityTypes)
Copy link
Contributor

@tcarrio tcarrio Apr 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jpasquers jpasquers merged commit aec3604 into main Apr 5, 2023
@jpasquers jpasquers deleted the feat/expose-builtin-types branch April 5, 2023 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.