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

Optimise filtering ID: foreignIDFilter in relation lists #10887

Closed
4 tasks
GuySartorelli opened this issue Jul 25, 2023 · 2 comments
Closed
4 tasks

Optimise filtering ID: foreignIDFilter in relation lists #10887

GuySartorelli opened this issue Jul 25, 2023 · 2 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Jul 25, 2023

Related to #10860

The foreignIDFilter() method in HasManyList and ManyManyList explicitly uses parameters in its WHERE IN clause, which has been found to be relatively slow.

We should modify these clauses to not use parameters, which will be faster. We will of course still have to validate that the ID values we're filtering against are ints to be safe.

We should also double check what ManyManyThroughList is doing and apply this optimisation there if feasible/necessary.

Acceptance criteria

  • HasManyList, ManyManyList, and ManyManyThroughList don't use parameters in the WHERE IN clause of their unfiltered SELECT queries (unless any of the ID values isn't a int)
  • You can opt-out of this behaviour using the same mechanism introduced by Optimise filtering ID columns #10860
    • Note that it may be appropriate to move the configuration property for that to somewhere more central, assuming this issue is done pre CMS5.1
  • Validate that there's a performance improvements
  • There's a short explanation on this card reiterating why there's no security concern with doing this.

PRs

@GuySartorelli GuySartorelli added this to the Silverstripe CMS 5.1 milestone Jul 31, 2023
@GuySartorelli GuySartorelli changed the title Optimise filtering ID foreignIDFilter in relation lists Optimise filtering ID: foreignIDFilter in relation lists Aug 6, 2023
@emteknetnz emteknetnz self-assigned this Aug 7, 2023
@emteknetnz
Copy link
Member

emteknetnz commented Aug 8, 2023

There's a short explanation on this card reiterating why there's no security concern with doing this.

a) Before querying we're always validating that all ID's are integers, so there's no scope for SQL injection
b) We've only added this to code which is called via the methods forForiegnIDFilter() which are intended only for foreign ID's, similar to the intention in #10861 which still used placeholders for non primary or foreign key fields.
c) We've provide opt-out configuration for sites that still desire placeholders

@GuySartorelli
Copy link
Member Author

Nice optimisation. PRs merged. Will be released in CMS 5.1.0

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

No branches or pull requests

2 participants