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

Remove root level nested params #20172

Open
wants to merge 5 commits into
base: v5/main
Choose a base branch
from

Conversation

Bassel17
Copy link
Member

What does it do?

It makes sure that only Fragments can be used with morph data structures,
updating the types and throwing errors incase the type checks were bypassed

How to test it?

This is an example on how things should work based on the getstarted app

strapi.documents('api::restaurant.restaurant').findMany({
      populate: {
        address: {
          populate: {
            cover: {
              fields: '*',
            },
          },
        },
        images: {
          fields: '*',
        },
        dz: {
          on: {
            'default.dish': true,
          },
        },
      },
    });

if you try to pass unsupported nested params to Dynamic Zone for example, typescript should show an error.
if typescript was ignored a clear error should be thrown

Co-authored-by: Jean-Sébastien Herbaux <Convly@users.noreply.github.com>
@Bassel17 Bassel17 added source: core:content-manager Source is core/content-manager package source: core:utils Source is core/utils or utils packages source: typescript Source is related to TypeScript (typings, tooling, ...) labels Apr 22, 2024
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.

Would it be possible to add unit tests to the convert-query-params? Dow e already have some?

@innerdvations innerdvations added the flag: 💥 Breaking change This PR contains breaking changes and should not be merged label Apr 22, 2024
@Bassel17
Copy link
Member Author

Would it be possible to add unit tests to the convert-query-params? Dow e already have some?

yeah, there are some not a lot, there are also some test.todos, so I could attempt to create those test cases

Copy link

vercel bot commented Apr 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
contributor-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 6, 2024 8:30pm

@Convly
Copy link
Member

Convly commented Apr 24, 2024

Would it be possible to add unit tests to the convert-query-params? Dow e already have some?

yeah, there are some not a lot, there are also some test.todos, so I could attempt to create those test cases

Would be awesome!

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.

You'll also need to remove the support for the deprecated syntax in the query-populate DZ handler.
https://github.com/strapi/strapi/blob/v5/main/packages/core/utils/src/traverse/query-populate.ts

// Handle legacy DZ params
        let newProperties: unknown = omit('on', value);

        for (const componentUID of components) {
          const componentSchema = getModel(componentUID);

          const properties = await recurse(
            visitor,
            { schema: componentSchema, path, getModel },
            value
          );
          newProperties = merge(newProperties, properties);
        }

        Object.assign(newValue, newProperties);

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.

Reviewed and nothing to add on the code that hasn't already been said.

There's a linting error, and it looks like some API tests need to be updated maybe because they were using the deprecated format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: 💥 Breaking change This PR contains breaking changes and should not be merged source: core:content-manager Source is core/content-manager package source: core:utils Source is core/utils or utils packages source: typescript Source is related to TypeScript (typings, tooling, ...)
Projects
Status: To Review
Development

Successfully merging this pull request may close these issues.

None yet

3 participants