-
Notifications
You must be signed in to change notification settings - Fork 970
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
Next-34765 - Disallow empty id filters in entity search #3696
Next-34765 - Disallow empty id filters in entity search #3696
Conversation
Note At first i wanted to implement this check directly in the criteria class, however it turned out that at least in tests the |
@Benedikt-Brunner yeah can you please move the check in the Criteria class to the |
@Benedikt-Brunner thanks for updating! I guess we can remove the check from the constructor now as it's duplicated? |
@AydinHassan that's right, i hadn't catched that the check was there 😅 . The move is however not complete as there are surely still failing tests. I am having a little trouble getting the tests to run correctly on my system so it would be great if we could enable the CI to run again 🙏 . |
@Benedikt-Brunner running now :) |
@Benedikt-Brunner looks like we need another empty check in |
@AydinHassan these new changes fixed all of the unit tests at least locally, however for some reason there was a migration that was constantly failing, i assume it was a problem on my machine as the migration did not use the criteria object. |
Nice thanks @Benedikt-Brunner I will import it now :) |
@Benedikt-Brunner can you maybe rebase and squash the commits? Our importer is not able to process the PR atm. |
55ae77d
to
55b75fe
Compare
Move empty ids check into criteria object fix entity reader id access refactor empty id initialisations
55b75fe
to
60eab29
Compare
@AydinHassan should be fixed now 🤞 |
Hello, thank you for creating this pull request. Please use this issue to track the state of your pull request. |
@Benedikt-Brunner many thanks! Now it's imported. I will let you know when it's merged 💙 |
try { | ||
$criteria = $this->createCriteria($request, $context); | ||
$response = $this->productListRoute->load($criteria, $context); | ||
} catch (DataAbstractionLayerException $e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you changed it like this? the logic is different now.
we should probably fix the throwing of this exception instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mitelg I think it's the same. Because now setIds()
on criteria throws an exception whens ids is empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the code specifically checked for the ids being empty, so i assumed it was expected behaviour. Therefore it seemed to be most correct to run the code behind that check when instantiating the criteria object fails.
I do however not know much about the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first the criteria is created and then the event is dispatched. that is now changed to dispatching the event after the loading of the product list. which is wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yeah, thanks. I will take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case the event probably needs to be dispatched in both the try
and catch
closures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a bit tricky because we no longer have a valid Criteria
object to dispatch GuestWishListPageletProductCriteriaEvent
with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should i do something here or will you handle it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Benedikt-Brunner I made the changes and its internal review now. Will ping you when it's merged :)
@Benedikt-Brunner thanks for your contribution, it's merged now! |
1. Why is this change necessary?
When fetching entities it is currently possible to pass the
ids
-filter as an empty list resulting in no filter being applied, therefore all entities are part of the result. Then all entities are loaded into memory, since theids
-filter disables thepage
andlimit
filters.2. What does this change do, exactly?
This adds a check to see if the
ids
-filter is an empty list and throws an exception informing the user of the api that"ids may not be empty"
.3. Describe each step to reproduce the issue or behaviour.
Send a request to
...api/search/{entity}
with roughly this body:Verify that all entities have been loaded / the program has run out of memory
4. Please link to the relevant issues (if any).
https://issues.shopware.com/issues/NEXT-34765
5. Checklist
copilot:summary
copilot:walkthrough