Skip to content
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

Do not add JOINs for Filters without a value #3703

Merged
merged 4 commits into from
Mar 1, 2021

Conversation

WybrenKoelmans
Copy link
Contributor

This potentially improves performance significantly with queries that use many filters on nested properties.

Q A
Bug fix? yes
New feature? no
BC breaks? potentially
Deprecations? no
Tickets n/a
License MIT
Doc PR n/a

Please note that this change has been poorly tested and this PR must be considered more of a discussion than an actual mergable PR.

In our app we have an Doctrine Entity with multiple associated ManyToOne and ManyToMany Collections. We have also defined SearchFilters for properties of these Collections in the Annotation for this entity.

While using the GraphQL API, we define the query with all these properties. When the user does not use the filter it will still be defined as nestedPropertyName: []. This causes the JOIN to be added through (old) lines 83-84. However these JOINs serve no function (?) because (old) line 89 exits the function due to a lack of values.

In our test joining all these relations even though they are not used makes a very significant performance difference (think 4000ms query vs 90ms in our specific case). Additionally these JOIN's filter out any results that do not have any items in that relation (even if the filter has no value), however I'm not sure that is a feature or a bug.

Regarding not setting the filter in the query in the first place: we aim to have our queries as static as possible only passing in different parameters, it seems that is already supported judging by (old) lines 88-90.

I would love to hear your opinion on this.

@alanpoulain
Copy link
Member

Seems OK to me, but it would be great to have a unit test to see what it does.
And could you also do it for the MongoDB part (it should be more or less the same thing)?

@WybrenKoelmans
Copy link
Contributor Author

Some more research reveals that using nestedPropertyName: null will not add the JOINs, because it exits at line 76. So there is a potential solution, however it would be a pain for us to convert each empty array to null before we can do the request (other parts of the interface rely on it being an array). I also feel like the [] and null case should at least be handled the same way (either both have JOINs or neither has the JOINs).

This potentially improves performance significantly with queries that use many filters on nested properties.
Also do not add lookups for MongoDB
@alanpoulain alanpoulain merged commit 7c6f96f into api-platform:2.6 Mar 1, 2021
@alanpoulain
Copy link
Member

Thanks @WybrenKoelmans.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants