8000 [DoctrineBridge] [Validator] UniqueEntityValidator should limit the retrieved entities to `2` to avoid potential memory issues with `broad` queries · Issue #43254 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DoctrineBridge] [Validator] UniqueEntityValidator should limit the retrieved entities to 2 to avoid potential memory issues with broad queries #43254

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
Closed
AndreasA opened this issue Sep 30, 2021 · 8 comments · Fixed by #43611
Closed

[DoctrineBridge] [Validator] UniqueEntityValidator should limit the retrieved entities to 2 to avoid potential memory issues with broad queries #43254

AndreasA opened this issue Sep 30, 2021 · 8 comments · Fixed by #43611
Labels
Bug DoctrineBridge Status: Needs Review Validator

Comments

@AndreasA
Copy link
Contributor

Symfony version(s) affected: 4.4.* and 5.3.*

Description
It isn't that huge an issue because the validator isn't supposed to be use in this way but in theory it would be possible to check uniqueness against a property that isn't strictly unique, e.g. due to old imported data.

This means that the validator could retrieve all those entities. However, in order to determine uniqueness it is enough to retrieve at max 2 as there are only these scenarios possible:

  • Count > 1: Definitely not unique
  • Count < 1 (= 0): Definitely unique
  • Count = 1: Check if fetched entity is equivalent to current.

So there is no reason to retrieve more than 2 and as it is a simple change, I think it should be added.
@AndreasA AndreasA added the Bug label Sep 30, 2021
@xabbuh
Copy link
Member
xabbuh commented Oct 1, 2021

Sounds reasonable to me. As this looks like a performance improvement to me a PR should be opened against the 5.4 branch.

@AndreasA Up to give it a try?

@AndreasA
Copy link
Contributor Author
AndreasA commented Oct 1, 2021 •
edited
Loading

@xabbuh I can't guarantee that I will find the time but I will try to create a PR next week. It should be rather straight forward.
Testing the performance/memory improvement might be more tricky though. However, I guess as long as the existing tests still work as they do currently, it should be fine?

@fabpot
Copy link
Member
fabpot commented Oct 2, 2021

No need to test the performance change for this change IMHO.

@AndreasA
Copy link
Contributor Author
AndreasA commented Oct 18, 2021 •
edited
Loading

OK. Great. I hope I will find time this week but can't guarantee it. Sadly, I didn't find time the last two weeks.

8000
@AndreasA
Copy link
Contributor Author

@fabpot @xabbuh

One remaining question though. How dynamic does it need to be? because supporting it for the findBy method is simple but if one uses a custom method, it will be more tricky as we do not know which parameter is the limit parameter.

I could only add the limit parameter, if the default method findBy is used and if another is used, they should implement it accordingly? Or I could add an option: additionalParameters that are added to the function call ?

@fabpot
Copy link
Member
fabpot commented Oct 18, 2021

Let's optimize for the simple and default findBy method. If someone uses a custom method, then they should optimize the query themselves (we might add a note in the docs about it though).

@xabbuh
Copy link
Member
xabbuh commented Oct 19, 2021

I agree with @fabpot.

@AndreasA AndreasA mentioned this issue Oct 20, 2021
Merged
@AndreasA
Copy link
Contributor Author

Created a PR now (for docs and symfony). Hopefully I did everything correctly, first symfony PR. However, one integration test is failing but it shouldn't be related to my PR at all (MemcacheAdapter)?

@fabpot fabpot closed this as completed Oct 26, 2021
fabpot added a commit that referenced this issue Oct 26, 2021
@fabpot
minor #43611 UniqueEntityValidator: Retrieve at max two entities to c…
ed3660a 
…heck uniqueness. (AndreasA)

This PR was merged into the 5.4 branch.

Discussion
----------

UniqueEntityValidator: Retrieve at max two entities to check uniqueness.

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #43254
| License       | MIT
| Doc PR        | symfony/symfony-docs#15965

It is enough to retrieve two entities to check for uniqueness.

- No entity returned, it is definitely unique.
- Two or more entities returned and it cannot be unique.
- One entity returned, it depends on the entity if it is unique or not.

By providing a limit it avoids potential performance and memory issues in case the entity uniqueness check is not done on a unique field.

Commits
-------

f55efc3 UniqueEntityValidator: Retrieve at max two entities for unique entity check.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug DoctrineBridge Status: Needs Review Validator
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

UniqueEntityValidator: Retrieve at max two entities to check uniqueness.
4 participants
@fabpot @AndreasA @xabbuh @carsonbot
0