Skip to content

fix(graphql): use default nested query operations #5174

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

alanpoulain
Copy link
Member

Instead of relying on a resource metadata factory to retrieve a "dynamic" nested query operation, this PR adds default nested query operations if the resource does not have one.

It simplifies a lot of things and catching the OperationNotFound exception is not needed anymore.

Since operations are "nested", they do not appear as top-level queries.

This PR also improves the XML/YAML compatibility.

@alanpoulain alanpoulain requested a review from soyuka November 9, 2022 15:45
@alanpoulain alanpoulain force-pushed the fix/graphql-default-nested-query-operations branch 2 times, most recently from 356a6f5 to 10389cc Compare November 9, 2022 17:00
Comment on lines +337 to +345
$collection = $this->phpize($operation, 'collection', 'bool', false);
if (Query::class === $class && $collection) {
$class = QueryCollection::class;
}

$delete = $this->phpize($operation, 'delete', 'bool', false);
if (Mutation::class === $class && $delete) {
$class = DeleteMutation::class;
}
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 seems to have been forgotten.
cc @vincentchalamon.

@@ -15,7 +15,7 @@
</uriVariables>

<operations>
<operation class="ApiPlatform\Metadata\GetCollection" name="custom_operation_name"/>
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 needed since the IRI converter uses the operation name:

return $this->router->generate($operation->getName(), $identifiers, $operation->getUrlGenerationStrategy() ?? $referenceType);

In the previous version of the ExtractorResourceMetadataCollectionFactory, the name was overridden. The AttributesResourceMetadataCollectionFactory does not do that.
Does it mean we cannot customize the operation name @soyuka?

Copy link
Member

Choose a reason for hiding this comment

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

yes we can change it, it should not be overriden

Copy link
Member

Choose a reason for hiding this comment

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

but usually the name of the operation is used to define the one inside the router therefore this should work as welll

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!

}

if (!$hasQueryOperation) {
$queryOperation = (new Query())->withNested(true);
Copy link
Member

Choose a reason for hiding this comment

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

mb comment on Nested or here that this operation is then use when we fetch a query through another query

Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't the comment on the method sufficient?

@soyuka
Copy link
Member

soyuka commented Nov 10, 2022

This is way better! Minor changes but then we can merge this!

@alanpoulain alanpoulain force-pushed the fix/graphql-default-nested-query-operations branch 3 times, most recently from 35b266b to 908ad4d Compare November 10, 2022 17:44
Instead of relying on a resource metadata factory to retrieve a "dynamic" nested query operation, this PR adds default nested query operations if the resource does not have one.
It simplifies a lot of things and catching the OperationNotFound exception is not needed anymore.
Since operations are "nested", they do not appear as top-level queries.
This PR also improves the XML/YAML compatibility.
@alanpoulain alanpoulain force-pushed the fix/graphql-default-nested-query-operations branch from 908ad4d to c1032e9 Compare November 10, 2022 18:05
foreach ($resource->getOperations() ?? $this->getDefaultHttpOperations($resource) as $i => $operation) {
[$key, $operation] = $this->getOperationWithDefaults($resource, $operation, $resource->getOperations() ? false : true);
$operations[$key] = $operation;
if (null === $resource->getOperations()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This was causing issues with the XML/YAML extractor (operations added twice), I think this modification improves the code (getOperationWithDefaults never called twice!).

@soyuka soyuka merged commit 5bc84ce into api-platform:3.0 Nov 10, 2022
@soyuka
Copy link
Member

soyuka commented Nov 10, 2022

thanks @alanpoulain !

@develth
Copy link
Contributor

develth commented Nov 15, 2022

i currently have the issue, that i query a single item with an id with nested Items.
As soon as i want to query the nested item, i get an Error from api-platform/core/src/Doctrine/Orm/State/ItemProvider.php:73 saying

More than one result was found for query although one row or none was expected.

Example Query:

query parentWithElements($parent: ID!) {
    parent(id: $parent) {
        id
        name
        elements{
            edges{
                node{
                    id
                    children{
                        edges{
                            node{
                                id
                                children{
                                    edges{
                                        node{
                                            id
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}

Is that related or a new issue?

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