Next-34765 - Disallow empty id filters in entity search #3696

Benedikt-Brunner · 2024-05-03T14:15:49Z

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 the ids-filter disables the page and limit 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:
```
{
   ...
   limit: 1,
   ids: [],
   ...
}
```
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

I have rebased my changes to remove merge conflicts
I have written tests and verified that they fail without my change
I have created a changelog file with all necessary information about my changes
I have written or adjusted the documentation according to my changes
This change has comments for package types, values, functions, and non-obvious lines of code
I have read the contribution requirements and fulfil them.

copilot:summary

copilot:walkthrough

Benedikt-Brunner · 2024-05-03T14:53:53Z

Note

At first i wanted to implement this check directly in the criteria class, however it turned out that at least in tests the ids-filter is set to an empty list quite often. If desired we could also refactor this PR to enforce this check directly in the criteria class.

AydinHassan · 2024-05-06T07:17:33Z

@Benedikt-Brunner yeah can you please move the check in the Criteria class to the setIds method and then just call setIds in the constructor please :)

AydinHassan · 2024-05-08T09:09:59Z

@Benedikt-Brunner thanks for updating! I guess we can remove the check from the constructor now as it's duplicated?

Benedikt-Brunner · 2024-05-08T09:32:10Z

@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 🙏 .

AydinHassan · 2024-05-08T09:34:21Z

@Benedikt-Brunner running now :)

src/Core/Framework/DataAbstractionLayer/Search/Criteria.php

AydinHassan · 2024-05-14T08:03:02Z

@Benedikt-Brunner looks like we need another empty check in \Shopware\Core\Framework\DataAbstractionLayer\Dbal\EntityReader::loadManyToManyWithCriteria where it calls setIds()

Benedikt-Brunner · 2024-05-14T12:00:08Z

@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.

AydinHassan · 2024-05-14T12:10:55Z

Nice thanks @Benedikt-Brunner I will import it now :)

AydinHassan · 2024-05-14T12:28:35Z

@Benedikt-Brunner can you maybe rebase and squash the commits? Our importer is not able to process the PR atm.

CLAassistant · 2024-05-14T13:20:03Z

All committers have signed the CLA.

Move empty ids check into criteria object fix entity reader id access refactor empty id initialisations

Benedikt-Brunner · 2024-05-14T13:52:45Z

@AydinHassan should be fixed now 🤞

shopware-github-importer · 2024-05-14T13:54:51Z

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/NEXT-34765

Please use this issue to track the state of your pull request.

AydinHassan · 2024-05-14T13:56:24Z

@Benedikt-Brunner many thanks! Now it's imported. I will let you know when it's merged 💙

mitelg · 2024-05-14T13:57:11Z

src/Storefront/Pagelet/Wishlist/GuestWishlistPageletLoader.php

+        try {
+            $criteria = $this->createCriteria($request, $context);
+            $response = $this->productListRoute->load($criteria, $context);
+        } catch (DataAbstractionLayerException $e) {


why did you changed it like this? the logic is different now.

we should probably fix the throwing of this exception instead

@mitelg I think it's the same. Because now setIds() on criteria throws an exception whens ids is empty.

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.

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

ah yeah, thanks. I will take a look.

In that case the event probably needs to be dispatched in both the try and catch closures.

it's a bit tricky because we no longer have a valid Criteria object to dispatch GuestWishListPageletProductCriteriaEvent with

Should i do something here or will you handle it?

@Benedikt-Brunner I made the changes and its internal review now. Will ping you when it's merged :)

AydinHassan · 2024-05-17T11:30:05Z

@Benedikt-Brunner thanks for your contribution, it's merged now!

github-actions bot added the Triage required label May 3, 2024

Benedikt-Brunner changed the title ~~disallow empty id filters in entity search~~ Next-34765 - Disallow empty id filters in entity search May 3, 2024

AydinHassan added the Area: Core label May 6, 2024

AydinHassan self-assigned this May 8, 2024

AydinHassan added the Incomplete label May 10, 2024

mitelg reviewed May 13, 2024

View reviewed changes

src/Core/Framework/DataAbstractionLayer/Search/Criteria.php Outdated Show resolved Hide resolved

AydinHassan removed Incomplete Triage required labels May 14, 2024

Benedikt-Brunner changed the base branch from trunk to 6.4.15.2 May 14, 2024 13:21

Benedikt-Brunner changed the base branch from 6.4.15.2 to trunk May 14, 2024 13:22

maximilianruesch force-pushed the NEXT-34765/disallow-empty-id-filters-in-entity-search branch from 55ae77d to 55b75fe Compare May 14, 2024 13:45

Disallow empty ID filters in entity search

60eab29

Move empty ids check into criteria object fix entity reader id access refactor empty id initialisations

maximilianruesch force-pushed the NEXT-34765/disallow-empty-id-filters-in-entity-search branch from 55b75fe to 60eab29 Compare May 14, 2024 13:46

shopware-github-importer bot added the Scheduled label May 14, 2024

mitelg reviewed May 14, 2024

View reviewed changes

shopwareBot closed this in 8facc78 May 17, 2024

AydinHassan removed the Scheduled label May 17, 2024

AydinHassan added the Accepted label May 17, 2024

Benedikt-Brunner deleted the NEXT-34765/disallow-empty-id-filters-in-entity-search branch May 17, 2024 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Next-34765 - Disallow empty id filters in entity search #3696

Next-34765 - Disallow empty id filters in entity search #3696

Benedikt-Brunner commented May 3, 2024 •

edited

Benedikt-Brunner commented May 3, 2024

AydinHassan commented May 6, 2024

AydinHassan commented May 8, 2024

Benedikt-Brunner commented May 8, 2024

AydinHassan commented May 8, 2024

AydinHassan commented May 14, 2024

Benedikt-Brunner commented May 14, 2024

AydinHassan commented May 14, 2024

AydinHassan commented May 14, 2024

CLAassistant commented May 14, 2024 •

edited

Benedikt-Brunner commented May 14, 2024

shopware-github-importer bot commented May 14, 2024

AydinHassan commented May 14, 2024

mitelg May 14, 2024

AydinHassan May 14, 2024 •

edited

Benedikt-Brunner May 14, 2024

mitelg May 14, 2024

AydinHassan May 14, 2024

Benedikt-Brunner May 14, 2024

AydinHassan May 14, 2024

Benedikt-Brunner May 14, 2024

AydinHassan May 15, 2024

AydinHassan commented May 17, 2024

Next-34765 - Disallow empty id filters in entity search #3696

Next-34765 - Disallow empty id filters in entity search #3696

Conversation

Benedikt-Brunner commented May 3, 2024 • edited

1. Why is this change necessary?

2. What does this change do, exactly?

3. Describe each step to reproduce the issue or behaviour.

4. Please link to the relevant issues (if any).

5. Checklist

Benedikt-Brunner commented May 3, 2024

AydinHassan commented May 6, 2024

AydinHassan commented May 8, 2024

Benedikt-Brunner commented May 8, 2024

AydinHassan commented May 8, 2024

AydinHassan commented May 14, 2024

Benedikt-Brunner commented May 14, 2024

AydinHassan commented May 14, 2024

AydinHassan commented May 14, 2024

CLAassistant commented May 14, 2024 • edited

Benedikt-Brunner commented May 14, 2024

shopware-github-importer bot commented May 14, 2024

AydinHassan commented May 14, 2024

Choose a reason for hiding this comment

AydinHassan May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AydinHassan commented May 17, 2024

Benedikt-Brunner commented May 3, 2024 •

edited

CLAassistant commented May 14, 2024 •

edited

AydinHassan May 14, 2024 •

edited