Skip to content

Use IRIs as GraphQL's ids and improve perf #1569

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 7 commits into from
Dec 20, 2017

Conversation

dunglas
Copy link
Member

@dunglas dunglas commented Dec 18, 2017

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a
  • Refactor (simplify) the GraphQL subsystem
  • Use IRIs as id (Relay compatibility, better compat with REST endpoints - I'll blog about this -, easier handling of composed identifiers)
  • Leverage the attributes feature of the Symfony serializer (will allow to reduce the number of SQL queries in a following PR)

TODO:

  • Add some more unit tests

@@ -72,8 +72,8 @@ public function createItemMutationResolver(string $resourceClass, string $mutati

return $this->normalizer->normalize(
$item,
null,
['graphql' => true] + $resourceMetadata->getGraphqlAttribute($mutationName, 'normalization_context', [], true)
'graphql',
Copy link
Member

Choose a reason for hiding this comment

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

ItemNormalizer::FORMAT

}

return [
'type' => $graphqlType,
'description' => $fieldDescription,
'args' => $args,
'resolve' => $resolve,
'context' => ['foo' => 'bar'],
Copy link
Member

Choose a reason for hiding this comment

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

What is this?

private $vary;
private $public;

public function __construct(bool $etag = false, int $maxAge = null, int $sharedMaxAge = null, array $vary = null, bool $public = null)
{
$this->etag = $etag;
$this->maxAge = $maxAge;
$this->sharedMaxage = $sharedMaxAge;
$this->sharedMaxAge = $sharedMaxAge;
Copy link
Member

Choose a reason for hiding this comment

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

Bad rebase?

*
* @return object|null
*/
private function getSubresource(string $rootClass, array $rootResolvedFields, array $rootIdentifiers, string $rootProperty, string $subresourceClass, bool $isCollection)
Copy link
Member

Choose a reason for hiding this comment

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

this code is also done in the SubresourceOperationFactory, maybe we should use the service instead to prevent side effects in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you point me to the duplicated snippet? And I didn't manage to find it.

Copy link
Member

Choose a reason for hiding this comment

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

If I get that correctly you want to find the subresources identifiers etc (ie: everything you need for the subresourceDataprovider->getSubresource context).

I'd recommend the use of this:

public function create(string $resourceClass): array

It's cached, and we use it in ApiLoader and DocumentationNormalizer already. In ApiLoader we build a per-operation context:

'_api_subresource_context' => [

@@ -146,7 +144,7 @@ private function getResourceFieldConfiguration(string $fieldDescription = null,
try {
$graphqlType = $this->convertType($type, $isInput, $isMutation, $mutationName);
$graphqlWrappedType = $graphqlType instanceof WrappingType ? $graphqlType->getWrappedType() : $graphqlType;
$isInternalGraphqlType = in_array($graphqlWrappedType, GraphQLType::getInternalTypes(), true);
$isInternalGraphqlType = \in_array($graphqlWrappedType, GraphQLType::getInternalTypes(), true);
Copy link
Member

Choose a reason for hiding this comment

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

\ not needed

Copy link
Member Author

@dunglas dunglas Dec 19, 2017

Choose a reason for hiding this comment

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

It is, it trigger an opcode optimization (PHP EA Inspection reports it), see also #1572.

Copy link
Member

Choose a reason for hiding this comment

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

Okay but we discussed it before and if we add it here shouldn't we put this everywhere? Oh okay so we actually came to the decision that we will haha nvm.

Copy link
Member

Choose a reason for hiding this comment

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

Why only for the in_array function then?

Copy link
Member

Choose a reason for hiding this comment

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

'identifiers' => [['rootIdentifier', 'rootClass']],
$subresourceDataProviderProphecy->getSubresource(RelatedDummy::class, $identifiers, [
'property' => 'relatedDummies',
'identifiers' => [['id', Dummy::class]],
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes id isn't called id but is another property. Maybe we should add such test to prevent failing cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

id is mandatory according to the Relay spec.

if (!is_array($collection)) {
$normalizerProphecy->normalize($collection->current(), Argument::cetera())->willReturn('normalized'.$collection->current());
} else {
if (\is_array($collection)) {
Copy link
Member

Choose a reason for hiding this comment

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

\ not needed

Copy link
Member Author

Choose a reason for hiding this comment

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

It is.

$resolveInfoProphecy->fieldName = 'rootProperty';

$this->assertNull($resolver(null, ['id' => 3], null, $resolveInfoProphecy->reveal()));
$this->assertNull($resolver(null, ['id' => '/related_dummies/3'], null, $resolveInfo));
Copy link
Member

Choose a reason for hiding this comment

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

Should I assume that ['id' => 3] isn't valid anymore (ie: not using IRIs?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. IRIs are now mandatory (it's valid according to both the GraphQL and the Relay spec).


$resolver = new ResourceFieldResolver($iriConverterProphecy->reveal());
$this->assertEquals('bar', $resolver(['foo' => 'bar'], [], [], $resolveInfo));
$this->assertEquals('bar', $resolver((object) ['foo' => 'bar'], [], [], $resolveInfo));
Copy link
Member

Choose a reason for hiding this comment

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

Okay in fact I think that most of my questions are answered here abouts ids.

@dunglas dunglas merged commit 0ed9a02 into api-platform:master Dec 20, 2017
@dunglas dunglas deleted the graphql-use-iri branch December 20, 2017 09:01
hoangnd25 pushed a commit to hoangnd25/core that referenced this pull request Feb 23, 2018
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.

4 participants