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

[Feature] Populate fragments for polymorphic relations #14879

Merged
merged 12 commits into from Nov 29, 2022

Conversation

Convly
Copy link
Member

@Convly Convly commented Nov 15, 2022

What does it do?

Add the possibility to fragment the populate query of polymorphic relations using the populate.on property.
It's possible to use this syntax for dynamic zones and media to add more granularity & precision to the populate's queries.

Why is it needed?

Up until now, it was not possible to differentiate multiple entities attached to a polymorphic relation & apply different rules to them based on their type.
It was particularly an issue regarding dynamic zone population as it was thus impossible to apply different filters to different kinds of component structures.

How to test it?

  • Run the examples/getstarted application
  • Create a restaurant entity and add multiple (different) components to the dynamic zone value (dz attribute)
  • Go to your browser and use a query similar to
    http://localhost:1337/api/restaurants?populate[dz][on][default.dish]=true
    You can also have variant such as
    http://localhost:1337/api/restaurants?populate[dz][on][default.dish][fields]=name
  • It should only populate the components that are defined in the on property.

Related issue(s)/PR(s)

fix #13220

@Convly Convly added source: core:database Source is core/database package pr: feature This PR adds a new feature flag: documentation This PR requires a documentation update labels Nov 15, 2022
@derrickmehaffy
Copy link
Member

@Convly how would this look in QS? Based on the way this is constructed it would mean being able to populate only specific components in a DZ? Does this break the existing logic?

@codecov
Copy link

codecov bot commented Nov 15, 2022

Codecov Report

Base: 59.61% // Head: 59.59% // Decreases project coverage by -0.02% ⚠️

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #14879      +/-   ##
==========================================
- Coverage   59.61%   59.59%   -0.03%     
==========================================
  Files        1340     1340              
  Lines       32639    32655      +16     
  Branches     6225     6232       +7     
==========================================
+ Hits        19459    19461       +2     
- Misses      11316    11326      +10     
- Partials     1864     1868       +4     
Flag Coverage Δ
back 49.64% <31.81%> (-0.06%) ⬇️
front 64.13% <ø> (ø)
unit_back 49.64% <31.81%> (-0.06%) ⬇️
unit_front 64.13% <ø> (ø)

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

Impacted Files Coverage Δ
.../core/database/lib/query/helpers/populate/apply.js 3.90% <0.00%> (-0.09%) ⬇️
packages/core/utils/lib/convert-query-params.js 19.73% <12.50%> (-0.27%) ⬇️
packages/core/utils/lib/content-types.js 91.78% <85.71%> (-1.18%) ⬇️

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.

@derrickmehaffy
Copy link
Member

@Convly how would this look in QS? Based on the way this is constructed it would mean being able to populate only specific components in a DZ? Does this break the existing logic?

Based on what JS showed me in a Discord voice call:

  • Yes you can do component specific stuff
  • No it won't break existing options but will replace them in v5 (for now they are mutually exclusive, if you use on other operators are ignored, the intention is to remove the other operators and make on the default in v5 without the on)

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

Just missing some tests and a few comments but looks pretty good

packages/core/utils/lib/convert-query-params.js Outdated Show resolved Hide resolved
packages/core/database/lib/query/helpers/populate/apply.js Outdated Show resolved Hide resolved
packages/core/database/lib/query/helpers/populate/apply.js Outdated Show resolved Hide resolved
Copy link
Contributor

@innerdvations innerdvations left a comment

Choose a reason for hiding this comment

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

This looks great to me, and I don't have any thoughts beyond what others have already brought up.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

A few comments but the is there 👌

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

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

LGTM. Nice

@Convly Convly removed the flag: documentation This PR requires a documentation update label Nov 23, 2022
@Convly Convly added this to the 4.5.3 milestone Nov 29, 2022
@Convly Convly merged commit 3ebd2db into main Nov 29, 2022
@Convly Convly deleted the features/morphs-populate branch November 29, 2022 16:06
@SalahAdDin
Copy link

This also does include a pretty good documentation, right?

@Convly
Copy link
Member Author

Convly commented Jan 11, 2023

This also does include a pretty good documentation, right?

strapi/documentation#1293 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: feature This PR adds a new feature source: core:database Source is core/database package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in processPopulate if different components in a dynamic zone share the same attribute name
5 participants