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

Better handle populating of dynamic-zones #13855

Merged
merged 1 commit into from Aug 1, 2022

Conversation

petersg83
Copy link
Contributor

@petersg83 petersg83 commented Jul 26, 2022

What does it do?

It changes the populate process for dynamic-zones.
Instead of assigning the attributes in a fake schema, it does the populate for each component of the dz and then depp merge the results together.
It is better but still not perfect as there is no granularity on which component to populate exactly (ie if 2 components have the same field names, we cannot populate only one, the 2 would be populated). That seems however reasonable to not address this issue for the moment

Why is it needed?

To fix this issue : #11955

How to test it?

A test was added

Related issue(s)/PR(s)

fix #11955
#11909

@petersg83 petersg83 added source: core:utils Source is core/utils or utils packages pr: fix This PR is fixing a bug labels Jul 26, 2022
@codecov
Copy link

codecov bot commented Jul 26, 2022

Codecov Report

Merging #13855 (5fa3b5d) into master (7431d8c) will increase coverage by 2.31%.
The diff coverage is 68.42%.

❗ Current head 5fa3b5d differs from pull request most recent head 952bca2. Consider uploading reports for the commit 952bca2 to get more accurate results

@@            Coverage Diff             @@
##           master   #13855      +/-   ##
==========================================
+ Coverage   54.34%   56.66%   +2.31%     
==========================================
  Files        1190     1178      -12     
  Lines       30194    28987    -1207     
  Branches     5470     5417      -53     
==========================================
+ Hits        16409    16425      +16     
+ Misses      12008    10798    -1210     
+ Partials     1777     1764      -13     
Flag Coverage Δ
front 59.86% <75.00%> (+3.31%) ⬆️
unit 48.90% <63.63%> (+0.21%) ⬆️

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

Impacted Files Coverage Δ
...ger/pages/EditSettingsView/components/ModalForm.js 30.90% <0.00%> (+0.55%) ⬆️
...es/core/admin/admin/src/pages/ProfilePage/index.js 67.69% <ø> (ø)
.../SettingsPage/pages/Users/EditPage/utils/layout.js 0.00% <ø> (ø)
...gsPage/pages/Users/components/SelectRoles/index.js 51.61% <ø> (ø)
packages/core/utils/lib/convert-query-params.js 13.45% <25.00%> (ø)
...b/sanitize/visitors/remove-restricted-relations.js 62.16% <85.71%> (+53.07%) ⬆️
...ges/Users/ListPage/DynamicTable/TableRows/index.js 83.33% <100.00%> (ø)
...ngsPage/pages/Users/ListPage/utils/tableHeaders.js 100.00% <100.00%> (ø)
...tViewDataManagerProvider/utils/getAPIInnerError.js 14.28% <0.00%> (-14.29%) ⬇️
...ackages/core/admin/admin/src/core/apis/Reducers.js 20.00% <0.00%> (-13.34%) ⬇️
... and 263 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7431d8c...952bca2. Read the comment docs.

@petersg83 petersg83 force-pushed the fix/dz-populate-same-field-name branch from 88f90a9 to 952bca2 Compare July 26, 2022 15:14
@petersg83 petersg83 marked this pull request as ready for review July 26, 2022 15:14
@petersg83 petersg83 added this to the 4.3.1 milestone Jul 26, 2022
Copy link
Member

@Convly Convly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nitintecg
Copy link

nitintecg commented Aug 2, 2022

@Convly @petersg83 We are facing one issue having 250+ Components in Dynamic Zone . Initially ,before this version find api (get content) was taking 10 ms and now its taking 30s sometimes 60s . Please Fix this Issue ASAP .

@narensgh
Copy link

narensgh commented Aug 2, 2022

I am happy for this fix as it solves once of the basic bug in Dynamic Zone, but even after the fix this feature is not yet production ready for complex use cases. We have 200+ components defined as templates and that can be selected/used by content managers to define a new page/content.

Prior to this PR, it was not returning data from all the components and once we migrated to this version(4.3.2) now its taking more that 50X time to return the response. Earlier it was taking around 600ms to return the response and now it takes more than 40 sec; this fix cannot be pushed to prod.

The real problem is https://github.com/strapi/strapi/blob/master/packages/core/utils/lib/convert-query-params.js#L173.
We are looping through all the dynamic zone components and quering the data from DB without checking if this is used in the current scope. Means suppose you have defined 500+ components in DZ and used only 5 in you content it'll loop through all 500 instead on only 5.

I am looking for a real solution to this or lets tag this feature as experimental.
Thanks!
@petersg83 @Convly

@petersg83
Copy link
Contributor Author

Hello, thank you for raising the issue @nitintejuja @narensgh!
Could you please share your schema and/or the payload that takes so much time to be loaded?
If the data is sensitive maybe you can send it to pierre.noel@strapi.io.
How many nested levels of components do you have?
If you prefer not to share anything, would you be available to talk about it on discord?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: core:utils Source is core/utils or utils packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Media/relation inside a repeatable component disapears when saving
5 participants