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

Fix huge autocomplete queries issue #10404

Merged
merged 2 commits into from Jun 13, 2019

Conversation

@bitbager
Copy link
Contributor

commented May 23, 2019

In case you have a large amount of data (i.e. 100k products), the autocomplete query fetches all matching results which brokes down the whole web server. This simple fix limits the results to 25 as in most cases the end user does not need more than this amount of suggested hits.
Could be improved by adding the limit parameter to the method signature.

Q A
Branch? 1.3
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
License MIT

@bitbager bitbager requested a review from Sylius/core-team as a code owner May 23, 2019

@bitbager bitbager changed the title Fix huge queries issue Fix huge autocomplete queries issue May 23, 2019

@@ -43,6 +43,7 @@ public function findByNamePart(string $phrase, string $locale): array
->andWhere('translation.name LIKE :name')
->setParameter('name', '%' . $phrase . '%')
->setParameter('locale', $locale)
->setMaxResults(25)

This comment has been minimized.

Copy link
@vvasiloi

vvasiloi May 23, 2019

Contributor

Maybe make it an optional argument with 25 as default value?

This comment has been minimized.

Copy link
@vvasiloi

vvasiloi May 23, 2019

Contributor

Nevermind, this is an interface method.

This comment has been minimized.

Copy link
@pamil

pamil May 24, 2019

Member

My idea would be to add an optional parameter to the method definition:

public function findByNamePart(string $phrase, string $locale, ?int $limit = null): array

This way, the behaviour wouldn't change, but we'd allow for specifying the max results amount.

The second step to actually make use of it would be to modify the routing of the action which provides autocompletion (sylius_admin_ajax_product_by_name_phrase) and limit the query to 25 there.

It would be best if we could change it for both ProductRepositoryInterface and TaxonRepositoryInterface, they both have the same method with the same usage.

What do you think?

This comment has been minimized.

Copy link
@vvasiloi

vvasiloi May 24, 2019

Contributor

@pamil That's what I thought of too, but wouldn't it be a BC break?

This comment has been minimized.

Copy link
@bitbager

bitbager May 24, 2019

Author Contributor

Yeap, as I said - this could be improved by adding the default limit to the signature. However, this would be a BC if someone customized the AJAX route in his local environment. As this would be a bugfix, I think that it makes more sense to add the limit in the default repository interface like this:
public function findByNamePart(string $phrase, string $locale, ?int $limit = 25): array

This comment has been minimized.

Copy link
@pamil

pamil Jun 13, 2019

Member

Even if the route got overwritten, it should then default to not including the limit - tested this on Sylius-Standard.

@@ -43,6 +43,7 @@ public function findByNamePart(string $phrase, string $locale): array
->andWhere('translation.name LIKE :name')
->setParameter('name', '%' . $phrase . '%')
->setParameter('locale', $locale)
->setMaxResults(25)

This comment has been minimized.

Copy link
@pamil

pamil May 24, 2019

Member

My idea would be to add an optional parameter to the method definition:

public function findByNamePart(string $phrase, string $locale, ?int $limit = null): array

This way, the behaviour wouldn't change, but we'd allow for specifying the max results amount.

The second step to actually make use of it would be to modify the routing of the action which provides autocompletion (sylius_admin_ajax_product_by_name_phrase) and limit the query to 25 there.

It would be best if we could change it for both ProductRepositoryInterface and TaxonRepositoryInterface, they both have the same method with the same usage.

What do you think?

@pamil
pamil approved these changes Jun 13, 2019

@pamil pamil force-pushed the bitbager:patch-2 branch from 0807783 to 30b53d0 Jun 13, 2019

@pamil pamil changed the base branch from master to 1.4 Jun 13, 2019

@pamil

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "patch-2" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@pamil pamil closed this Jun 13, 2019

@pamil pamil reopened this Jun 13, 2019

@pamil pamil force-pushed the bitbager:patch-2 branch from 30b53d0 to 95d841e Jun 13, 2019

Fix huge queries issue
In case you have a large amount of data (i.e. 100k products), the autocomplete query fetches all matching results which brokes down the whole web server. This simple fix limits the results to 25 as in most cases the end user does not need more than this amount of suggested hits. 
Could be improved by adding the limit parameter to the method signature.

@pamil pamil force-pushed the bitbager:patch-2 branch from bd7ca27 to 6c1a703 Jun 13, 2019

@GSadee
GSadee approved these changes Jun 13, 2019

@pamil pamil merged commit 3dca62d into Sylius:1.4 Jun 13, 2019

2 checks passed

WIP Ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pamil

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Thank you, Mikołaj! 🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.