Skip to content

Fix joinRelations with multiple associations. #2791

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 2 commits into from Mar 23, 2021
Merged

Fix joinRelations with multiple associations. #2791

merged 2 commits into from Mar 23, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 14, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Hello,
I propose my first contribution to the project.

I found that the final SQL query was not correct in the case where an entity had several doctrinal relationships and was asked to retrieve some properties with the PropertyFilter.

Example:

class Example {
     public $property;
     public $relation1;
     public $relation2;
}
class Relation {
     public $property1;
     public $property2;
}

uri: /api/examples?properties[relation1][]=property1&properties[relation2][]=property2

In this case the relation1 has been added but relation2 is ignored.

After searching, you can see that the bug is coming from: src/Bridge/Doctrine/Orm/Extension/EagerLoadingExtension.php line 169 or the context is overwritten.

I propose my modification to fix the bug.

Sorry I'm not good at unit testing so if someone can help me write it I'm interested.

@ghost
Copy link
Author

ghost commented Aug 1, 2019

Hello,

I come back to this pullRequest because I just noticed that it does not affect the number sql request but also the Order filter If have relation ...ToMany.

I have create an example Demo to show you.
Clone my repository and enter make for install and launch.

Test 1:
curl -X GET -H 'Accept: application/ld+json' -i 'http://localhost:8000/api/examples?properties[]=property1&properties[relation1][]=relationProperty2&properties[relation2][]=relationProperty1'

OK but 32 Database Queries

Test 2:
curl -X GET -H 'Accept: application/ld+json' -i 'http://localhost:8000/api/examples?order[relation2.relationProperty1]=asc&properties[]=property1&properties[relation1][]=relationProperty2&properties[relation2][]=relationProperty1'

NOK Status code 500
'Cannot select distinct identifiers from query with LIMIT and ORDER BY on a column from a fetch joined to-many association. Use output walkers.'

Test 3:
curl -X GET -H 'Accept: application/ld+json' -i 'http://localhost:8000/api/examples?order[relation2.relationProperty1]=asc&properties[relation2][]=relationProperty1'

OK

@ghost
Copy link
Author

ghost commented Jan 3, 2020

Hi @dunglas, is possible to valid this pull request ? because it is a problem in my production app

@soyuka
Copy link
Member

soyuka commented Jan 6, 2020

The fix looks correct, can you add your test (from your demo) to our behat test suite? See Tests/Fixtures and features/filters. Also, bug fixes should now target the 2.5 branch.

@ghost ghost changed the base branch from 2.4 to 2.5 January 7, 2020 10:13
@ghost
Copy link
Author

ghost commented Jan 7, 2020

I have change target to 2.5 but for test it is possible to help me because I'm not very experienced.
Learn Beahat and PhpUnit is on my todo list for 2020

@soyuka
Copy link
Member

soyuka commented Jan 8, 2020

We can guide you to set it up :). Copy one of the tests from the file I linked above, write your spec to reproduce the Test 3 of your first post, add some entities to the fixtures to match your example properties and you're all set!
We're not in a hurry it's open source :).

@alanpoulain alanpoulain changed the base branch from 2.5 to 2.6 March 23, 2021 14:56
@alanpoulain alanpoulain merged commit e21565d into api-platform:2.6 Mar 23, 2021
@alanpoulain
Copy link
Member

Thank you @xPolo360.

norkunas pushed a commit to norkunas/core that referenced this pull request Mar 25, 2021
norkunas pushed a commit to norkunas/core that referenced this pull request Mar 25, 2021
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.

None yet

2 participants