-
-
Notifications
You must be signed in to change notification settings - Fork 921
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
Conversation
@@ -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', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ItemNormalizer::FORMAT
src/Graphql/Type/SchemaBuilder.php
Outdated
} | ||
|
||
return [ | ||
'type' => $graphqlType, | ||
'description' => $fieldDescription, | ||
'args' => $args, | ||
'resolve' => $resolve, | ||
'context' => ['foo' => 'bar'], |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad rebase?
dd0c719
to
b1b9213
Compare
173ef37
to
c4fa41d
Compare
* | ||
* @return object|null | ||
*/ | ||
private function getSubresource(string $rootClass, array $rootResolvedFields, array $rootIdentifiers, string $rootProperty, string $subresourceClass, bool $isCollection) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
core/src/Bridge/Symfony/Routing/ApiLoader.php
Line 112 in 091865f
'_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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\
not needed
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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]], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
\
not needed
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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.
attributes
feature of the Symfony serializer (will allow to reduce the number of SQL queries in a following PR)TODO: