Fix bulk delete when entity has empty components #14600

Marc-Roig · 2022-10-11T14:25:08Z

What does it do?

Why is it needed?

Deleting an entity with empty components was throwing an error.
deleteComponents function expects one of the two:

An entity with loaded components.
An entity with an id.

When components were empty, the first case was not met, so the function was expecting the entity with an id, which we were not passing. So 💥

How to test it?

Delete or bulk delete an entity and you should not see any error in the console.

Related issue(s)/PR(s)

#14384

codecov · 2022-10-11T14:43:51Z

Codecov Report

Base: 58.71% // Head: 50.24% // Decreases project coverage by -8.47% ⚠️

Coverage data is based on head (f367352) compared to base (d905044).
Patch coverage: 26.66% of modified lines in pull request are covered.

❗ Current head f367352 differs from pull request most recent head d58963e. Consider uploading reports for the commit d58963e to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #14600      +/-   ##
==========================================
- Coverage   58.71%   50.24%   -8.48%     
==========================================
  Files        1321      279    -1042     
  Lines       31981     9687   -22294     
  Branches     5960     2109    -3851     
==========================================
- Hits        18777     4867   -13910     
+ Misses      11344     3977    -7367     
+ Partials     1860      843    -1017

Flag	Coverage Δ
front	`?`
unit	`50.24% <26.66%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...e/strapi/lib/services/entity-service/components.js	`22.29% <8.33%> (+0.58%)`	⬆️
...s/core/strapi/lib/services/entity-service/index.js	`63.26% <100.00%> (+0.25%)`	⬆️
...c/components/EditAssetDialog/ReplaceMediaButton.js
...plicationInfosPage/components/LogoInput/reducer.js
...issions/utils/formatContentTypesPermissionToAPI.js
...ore/admin/admin/src/components/GuidedTour/index.js
...in/src/components/BulkMoveDialog/BulkMoveDialog.js
...sPage/pages/Users/utils/validations/users/index.js
...min/src/content-manager/utils/createDefaultForm.js
...ckages/core/upload/admin/src/utils/urlYupSchema.js
... and 1034 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

petersg83 · 2022-10-11T15:33:05Z

packages/core/strapi/lib/services/entity-service/index.js

@@ -217,7 +217,7 @@ const createDefaultImplementation = ({ strapi, db, eventHub, entityValidator })
    const componentsToDelete = await getComponents(uid, entityToDelete);

    await db.query(uid).delete({ where: { id: entityToDelete.id } });
-    await deleteComponents(uid, { ...entityToDelete, ...componentsToDelete });
+    await deleteComponents(uid, componentsToDelete);


In entityToDelete the id was not there?

yes! But it wasn't for the bulk delete.

I though it was going to complicate things doing this in the bulkDelete, when you have multiple entities.
So my proposal is to return the entity id in getComponents instead

After discussing it with you I made the changes!
It didn't make sense to return the entity ID in getComponents. Now we specify if we want to loadComponentss when calling deleteComponents 👍

jhoward1994

Nice!

In my testing single deletion works but bulkDelete is causing an error. I think it is caused by packages/core/strapi/lib/services/entity-service/components.js#L361 not passing the third argument to deleteComponents

p.s. do you know how to leave a PR comment on an unmodified line?

Marc-Roig · 2022-10-13T11:25:48Z

Nice!

In my testing single deletion works but bulkDelete is causing an error. I think it is caused by packages/core/strapi/lib/services/entity-service/components.js#L361 not passing the third argument to deleteComponents

p.s. do you know how to leave a PR comment on an unmodified line?

good call.
I had to add a default {} value for for the loadComponents parameter

In the deleteComponents function the desired behaviour is to load the components (which is the default when not passing anything)

And no idea how to leave a comment on an unmodified line :/

jhoward1994 · 2022-10-13T11:52:07Z

Nice!
In my testing single deletion works but bulkDelete is causing an error. I think it is caused by packages/core/strapi/lib/services/entity-service/components.js#L361 not passing the third argument to deleteComponents
p.s. do you know how to leave a PR comment on an unmodified line?

good call. I had to add a default {} value for for the loadComponents parameter

In the deleteComponents function the desired behaviour is to load the components (which is the default when not passing anything)

And no idea how to leave a comment on an unmodified line :/

Nice, it works well for me now 🙂

petersg83 · 2022-10-17T13:16:45Z

packages/core/strapi/lib/services/entity-service/components.js

-        (await strapi.query(uid).load(entityToDelete, attributeName));
+      if (!value && loadComponents) {
+        value = await strapi.query(uid).load(entityToDelete, attributeName);
+      }


In a non repeatable component case, if loadComponents is true and there is no relations, we would load twice no? Line 286 and there.

you are right. This could be removed.

it would load it twice for repeatable components

dkryvi · 2022-10-25T14:41:48Z

Hello, any updates in this pull request? It's a critical issue for our project

Marc-Roig · 2022-10-25T14:47:09Z

Hello, any updates in this pull request? It's a critical issue for our project

Hello, I will try to push this so it is ready for next release

dkryvi · 2022-10-25T15:14:20Z

Hello, any updates in this pull request? It's a critical issue for our project

Hello, I will try to push this so it is ready for next release

Thank you. We'are waiting :)

petersg83

LGTM :) well done

Marc-Roig added 5 commits October 11, 2022 15:37

filter empty components when deleting entity

cf67b12

test with empty component

be2a256

add entity id in list of components

d5ae2f9

use entity id when deleting components

38ed957

remove unnecessary isEmpty

cee7eb1

Marc-Roig added source: core:strapi Source is core/strapi package pr: fix This PR is fixing a bug labels Oct 11, 2022

Marc-Roig added this to the 4.4.4 milestone Oct 11, 2022

Marc-Roig requested a review from petersg83 October 11, 2022 14:25

Marc-Roig self-assigned this Oct 11, 2022

Marc-Roig changed the title ~~filter empty components when deleting entity~~ Fix bulk delete when entity has empty components Oct 11, 2022

Marc-Roig requested a review from jhoward1994 October 11, 2022 14:29

add await to load components

6a6701c

petersg83 reviewed Oct 11, 2022

View reviewed changes

Marc-Roig added 2 commits October 11, 2022 18:21

add load components parameter

842c38b

refactor deleteComponents

a1537f3

Marc-Roig requested a review from petersg83 October 11, 2022 16:30

only load value if specified

a99d68d

jhoward1994 requested changes Oct 13, 2022

View reviewed changes

load components with default value

69fa150

jhoward1994 self-requested a review October 13, 2022 11:51

jhoward1994 previously approved these changes Oct 13, 2022

View reviewed changes

petersg83 suggested changes Oct 17, 2022

View reviewed changes

remove duplicated load

b6e6fc3

it would load it twice for repeatable components

Marc-Roig dismissed jhoward1994’s stale review via b6e6fc3 October 17, 2022 13:40

Merge branch 'main' into fix/bulk-delete-error-no-compos

d58963e

Marc-Roig requested a review from petersg83 October 18, 2022 07:57

Convly modified the milestones: 4.4.4, 4.4.5, 4.4.6 Oct 19, 2022

petersg83 approved these changes Oct 25, 2022

View reviewed changes

Marc-Roig merged commit 191b64d into main Oct 25, 2022

Marc-Roig deleted the fix/bulk-delete-error-no-compos branch October 25, 2022 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bulk delete when entity has empty components #14600

Fix bulk delete when entity has empty components #14600

Marc-Roig commented Oct 11, 2022

codecov bot commented Oct 11, 2022 •

edited

petersg83 Oct 11, 2022

Marc-Roig Oct 11, 2022

Marc-Roig Oct 11, 2022

jhoward1994 left a comment

Marc-Roig commented Oct 13, 2022

jhoward1994 commented Oct 13, 2022

petersg83 Oct 17, 2022

Marc-Roig Oct 17, 2022

dkryvi commented Oct 25, 2022

Marc-Roig commented Oct 25, 2022

dkryvi commented Oct 25, 2022

petersg83 left a comment

Fix bulk delete when entity has empty components #14600

Fix bulk delete when entity has empty components #14600

Conversation

Marc-Roig commented Oct 11, 2022

What does it do?

Why is it needed?

How to test it?

Related issue(s)/PR(s)

codecov bot commented Oct 11, 2022 • edited

Codecov Report

petersg83 Oct 11, 2022

Choose a reason for hiding this comment

Marc-Roig Oct 11, 2022

Choose a reason for hiding this comment

Marc-Roig Oct 11, 2022

Choose a reason for hiding this comment

jhoward1994 left a comment

Choose a reason for hiding this comment

Marc-Roig commented Oct 13, 2022

jhoward1994 commented Oct 13, 2022

petersg83 Oct 17, 2022

Choose a reason for hiding this comment

Marc-Roig Oct 17, 2022

Choose a reason for hiding this comment

dkryvi commented Oct 25, 2022

Marc-Roig commented Oct 25, 2022

dkryvi commented Oct 25, 2022

petersg83 left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2022 •

edited