Skip to content

fix(graphql): use depth for nested resource class operation #5314

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.

Sign up for GitHub

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

Jump to bottom
Merged
alanpoulain merged 2 commits into from
Jan 12, 2023
Merged

fix(graphql): use depth for nested resource class operation #5314

alanpoulain merged 2 commits into from
Jan 12, 2023

Conversation

@NicoHaase
Copy link
Contributor

@NicoHaase NicoHaase commented Jan 9, 2023 •
edited
Loading

Q A
Branch? 3.0
Tickets #5310
License MIT
Doc PR api-platform/docs#...

Reproducer for the bug reported in #5310

@alanpoulain
Copy link
Member

alanpoulain commented Jan 9, 2023

Hello, thanks for this PR!
Could you use a dedicated resource though?

@NicoHaase NicoHaase force-pushed the 5310-reproducer branch 2 times, most recently from 843b723 to b726568 Compare January 9, 2023 09:55
@NicoHaase
Copy link
Contributor Author

NicoHaase commented Jan 9, 2023

Hello, thanks for this PR! Could you use a dedicated resource though?

Sorry - this still needs some work, as I'm currently not able to reproduce the problem in an isolated test fase

@NicoHaase
Copy link
Contributor Author

NicoHaase commented Jan 9, 2023

Let's see whether the CI triggers the same problem I could reproduce locally now. Also, I've found how my previous version of this reproducer differs from this one: by reordering the list of graphqloperations on the TreeDummy, I was able to fix the bug. Having QueryCollection first and then Query, no bug is thrown. This also resolves the problem in my own application.

But that looks pretty strange to me. Why should the order of these operations influence how a field is resolved?

@alanpoulain alanpoulain changed the base branch from main to 3.0 January 12, 2023 10:33
@alanpoulain alanpoulain changed the title Reproducer for bug 5310 fix(graphql): use depth for nested resource class operation Jan 12, 2023
@alanpoulain alanpoulain marked this pull request as ready for review January 12, 2023 10:34
@alanpoulain
Copy link
Member

alanpoulain commented Jan 12, 2023

Hello @NicoHaase, I've made the fix I think is appropriate. What do you think of it?
@develth, does it fix your issue too?

@develth
Copy link
Contributor

develth commented Jan 12, 2023

Hello @NicoHaase, I've made the fix I think is appropriate. What do you think of it? @develth, does it fix your issue too?

will create a diff patch & test it - thx!

@develth
Copy link
Contributor

develth commented Jan 12, 2023

Your fix was successful for me @alanpoulain !

@alanpoulain alanpoulain merged commit 3d8371a into api-platform:3.0 Jan 12, 2023
@NicoHaase NicoHaase deleted the 5310-reproducer branch January 12, 2023 14:51
@NicoHaase
Copy link
Contributor Author

NicoHaase commented Jan 12, 2023

I haven't checked this fix yet, but when the test is no longer failing, I would assume it works :) But what makes me curious is why ordering the operations makes a difference. This is not addressed in this fix

@alanpoulain
Copy link
Member

alanpoulain commented Jan 12, 2023 •
edited
Loading

It doesn't make a difference anymore since the nested operation will now always be selected depending on the type (collection or not).
Before it was not the case and the first operation on the resource was used.

You can better understand it here:

core/src/GraphQl/Type/TypeBuilder.php

Lines 51 to 54 in 902b135

public function getResourceObjectType(?string $resourceClass, ResourceMetadataCollection $resourceMetadataCollection, Operation $operation, bool $input, bool $wrapped = false, int $depth = 0): GraphQLType
{
$shortName = $operation->getShortName();
$operationName = $operation->getName();

The first operation on the resource will be used to determine the resource GraphQL type.

KDederichs pushed a commit to KDederichs/core that referenced this pull request Jan 12, 2023
@NicoHaase @alanpoulain @KDederichs
fix(graphql): use depth for nested resource class operation (api-plat…
b49768e 
…form#5314)

* test: add reproducer for bug 5310

* fix(graphql): use depth for nested resource class operation

Co-authored-by: Alan Poulain <[email protected]>
soyuka pushed a commit that referenced this pull request Jan 12, 2023
@KDederichs @NicoHaase @alanpoulain
chore(symfony): Only use ValueResolverInterface if it exists (#5324)
965332b 
* chore(deprecation): Only use ValueResolverInterface if it exists

* chore(deprecation): fix CS

* chore(deprecation): Add new CompatibleValueResolverInterface

* fix(graphql): use depth for nested resource class operation (#5314)

* test: add reproducer for bug 5310

* fix(graphql): use depth for nested resource class operation

Co-authored-by: Alan Poulain <[email protected]>

* ci: test against lowest dependencies (#5329)

Co-authored-by: Nico Haase <[email protected]>
Co-authored-by: Alan Poulain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@soyuka soyuka soyuka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@NicoHaase @alanpoulain @develth @soyuka