From 4dcfe4ab407307ff7c7b5e37fad783c28e31df54 Mon Sep 17 00:00:00 2001 From: Convly Date: Fri, 4 Nov 2022 15:06:36 +0100 Subject: [PATCH 1/6] (tmp) init morph populate --- .../lib/query/helpers/populate/apply.js | 19 +++++++++-- .../core/utils/lib/convert-query-params.js | 33 +++++++++---------- 2 files changed, 32 insertions(+), 20 deletions(-) diff --git a/packages/core/database/lib/query/helpers/populate/apply.js b/packages/core/database/lib/query/helpers/populate/apply.js index 2ae346d135..130f2aa093 100644 --- a/packages/core/database/lib/query/helpers/populate/apply.js +++ b/packages/core/database/lib/query/helpers/populate/apply.js @@ -470,6 +470,8 @@ const morphToMany = async (input, ctx) => { }, {}); const map = {}; + const { on, ...typePopulate } = populateValue; + for (const type of Object.keys(idsByType)) { const ids = idsByType[type]; @@ -480,10 +482,14 @@ const morphToMany = async (input, ctx) => { continue; } + if (on && on[type]) { + Object.assign(typePopulate, on[type]); + } + const qb = db.entityManager.createQueryBuilder(type); const rows = await qb - .init(populateValue) + .init(typePopulate) .addSelect(`${qb.alias}.${idColumn.referencedColumn}`) .where({ [idColumn.referencedColumn]: ids }) .execute({ mapResults: false }); @@ -579,7 +585,16 @@ const morphToOne = async (input, ctx) => { // TODO: Omit limit & offset to avoid needing a query per result to avoid making too many queries const pickPopulateParams = (populate) => { - const fieldsToPick = ['select', 'count', 'where', 'populate', 'orderBy', 'filters', 'ordering']; + const fieldsToPick = [ + 'select', + 'count', + 'where', + 'populate', + 'orderBy', + 'filters', + 'ordering', + 'on', + ]; if (populate.count !== true) { fieldsToPick.push('limit', 'offset'); diff --git a/packages/core/utils/lib/convert-query-params.js b/packages/core/utils/lib/convert-query-params.js index 38d1af385b..4c11510375 100644 --- a/packages/core/utils/lib/convert-query-params.js +++ b/packages/core/utils/lib/convert-query-params.js @@ -6,17 +6,17 @@ * Converts the standard Strapi REST query params to a more usable format for querying * You can read more here: https://docs.strapi.io/developer-docs/latest/developer-resources/database-apis-reference/rest-api.html#filters */ + const { + isNil, + toNumber, + isInteger, has, isEmpty, isObject, isPlainObject, cloneDeep, get, - mergeAll, - isNil, - toNumber, - isInteger, } = require('lodash/fp'); const _ = require('lodash'); const parseType = require('./parse-type'); @@ -185,22 +185,19 @@ const convertPopulateObject = (populate, schema) => { return acc; } - // FIXME: This is a temporary solution for dynamic zones that should be - // fixed when we'll implement a more accurate way to query them - if (attribute.type === 'dynamiczone') { - const populates = attribute.components - .map((uid) => strapi.getModel(uid)) - .map((schema) => convertNestedPopulate(subPopulate, schema)) - .map((populate) => (populate === true ? {} : populate)) // cast boolean to empty object to avoid merging issues - .filter((populate) => populate !== false); - - if (isEmpty(populates)) { - return acc; - } - + if (subPopulate && subPopulate.on) { return { ...acc, - [key]: mergeAll(populates), + [key]: { + ...subPopulate, + on: Object.entries(subPopulate.on).reduce( + (newTypeSubPopulate, [type, typeSubPopulate]) => ({ + ...newTypeSubPopulate, + [type]: convertNestedPopulate(typeSubPopulate, strapi.getModel(type)), + }), + {} + ), + }, }; } From 8c2fb155b82e747b4ea2b63424589c213fb81e1c Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 15 Nov 2022 15:14:28 +0100 Subject: [PATCH 2/6] hanlde morphToOne populate & re-add legacy way of querying DZs --- .../lib/query/helpers/populate/apply.js | 15 ++++++----- .../core/utils/lib/convert-query-params.js | 27 ++++++++++++++++--- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/packages/core/database/lib/query/helpers/populate/apply.js b/packages/core/database/lib/query/helpers/populate/apply.js index 130f2aa093..1138bab261 100644 --- a/packages/core/database/lib/query/helpers/populate/apply.js +++ b/packages/core/database/lib/query/helpers/populate/apply.js @@ -446,6 +446,11 @@ const morphToMany = async (input, ctx) => { .where({ [joinColumn.name]: referencedValues, ...(joinTable.on || {}), + // If the populateValue contains an "on" property, + // only populate the types defined in it + ...('on' in populateValue + ? { [morphColumn.typeColumn.name]: Object.keys(populateValue.on) } + : {}), }) .orderBy([joinColumn.name, 'order']) .execute({ mapResults: false }); @@ -482,14 +487,10 @@ const morphToMany = async (input, ctx) => { continue; } - if (on && on[type]) { - Object.assign(typePopulate, on[type]); - } - const qb = db.entityManager.createQueryBuilder(type); const rows = await qb - .init(typePopulate) + .init(on && type in on ? on[type] : typePopulate) .addSelect(`${qb.alias}.${idColumn.referencedColumn}`) .where({ [idColumn.referencedColumn]: ids }) .execute({ mapResults: false }); @@ -546,6 +547,8 @@ const morphToOne = async (input, ctx) => { }, {}); const map = {}; + const { on, ...typePopulate } = populateValue; + for (const type of Object.keys(idsByType)) { const ids = idsByType[type]; @@ -558,7 +561,7 @@ const morphToOne = async (input, ctx) => { const qb = db.entityManager.createQueryBuilder(type); const rows = await qb - .init(populateValue) + .init(on && type in on ? on[type] : typePopulate) .addSelect(`${qb.alias}.${idColumn.referencedColumn}`) .where({ [idColumn.referencedColumn]: ids }) .execute({ mapResults: false }); diff --git a/packages/core/utils/lib/convert-query-params.js b/packages/core/utils/lib/convert-query-params.js index 4c11510375..a6b6cadb26 100644 --- a/packages/core/utils/lib/convert-query-params.js +++ b/packages/core/utils/lib/convert-query-params.js @@ -17,6 +17,7 @@ const { isPlainObject, cloneDeep, get, + mergeAll, } = require('lodash/fp'); const _ = require('lodash'); const parseType = require('./parse-type'); @@ -185,14 +186,13 @@ const convertPopulateObject = (populate, schema) => { return acc; } - if (subPopulate && subPopulate.on) { + if (subPopulate && 'on' in subPopulate) { return { ...acc, [key]: { - ...subPopulate, on: Object.entries(subPopulate.on).reduce( - (newTypeSubPopulate, [type, typeSubPopulate]) => ({ - ...newTypeSubPopulate, + (acc, [type, typeSubPopulate]) => ({ + ...acc, [type]: convertNestedPopulate(typeSubPopulate, strapi.getModel(type)), }), {} @@ -201,6 +201,25 @@ const convertPopulateObject = (populate, schema) => { }; } + // TODO: Deprecated way of handling dynamic zone populate queries. It's kept as is, + // as removing it could break existing user queries but should be removed in V5. + if (attribute.type === 'dynamiczone') { + const populates = attribute.components + .map((uid) => strapi.getModel(uid)) + .map((schema) => convertNestedPopulate(subPopulate, schema)) + .map((populate) => (populate === true ? {} : populate)) // cast boolean to empty object to avoid merging issues + .filter((populate) => populate !== false); + + if (isEmpty(populates)) { + return acc; + } + + return { + ...acc, + [key]: mergeAll(populates), + }; + } + // NOTE: Retrieve the target schema UID. // Only handles basic relations, medias and component since it's not possible // to populate with options for a dynamic zone or a polymorphic relation From 0c205d0f58537a52bf0b4d77f89798731a8dee48 Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 15 Nov 2022 18:31:33 +0100 Subject: [PATCH 3/6] Fix convert query params for 'true' subPopulate --- packages/core/utils/lib/convert-query-params.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/core/utils/lib/convert-query-params.js b/packages/core/utils/lib/convert-query-params.js index a6b6cadb26..cd274b97d7 100644 --- a/packages/core/utils/lib/convert-query-params.js +++ b/packages/core/utils/lib/convert-query-params.js @@ -186,7 +186,7 @@ const convertPopulateObject = (populate, schema) => { return acc; } - if (subPopulate && 'on' in subPopulate) { + if (typeof subPopulate === 'object' && 'on' in subPopulate) { return { ...acc, [key]: { From 7c1837bbe1daaa2a62bad41a59c3968b0a9e6cd6 Mon Sep 17 00:00:00 2001 From: Convly Date: Mon, 21 Nov 2022 11:02:38 +0100 Subject: [PATCH 4/6] Add test + update logic --- .../lib/query/helpers/populate/apply.js | 4 +- .../api/populate/filtering/index.test.api.js | 60 +++++++++++++++++++ .../core/utils/lib/convert-query-params.js | 14 ++++- 3 files changed, 73 insertions(+), 5 deletions(-) diff --git a/packages/core/database/lib/query/helpers/populate/apply.js b/packages/core/database/lib/query/helpers/populate/apply.js index 1138bab261..fee3feeeaa 100644 --- a/packages/core/database/lib/query/helpers/populate/apply.js +++ b/packages/core/database/lib/query/helpers/populate/apply.js @@ -490,7 +490,7 @@ const morphToMany = async (input, ctx) => { const qb = db.entityManager.createQueryBuilder(type); const rows = await qb - .init(on && type in on ? on[type] : typePopulate) + .init(on?.[type] ?? typePopulate) .addSelect(`${qb.alias}.${idColumn.referencedColumn}`) .where({ [idColumn.referencedColumn]: ids }) .execute({ mapResults: false }); @@ -561,7 +561,7 @@ const morphToOne = async (input, ctx) => { const qb = db.entityManager.createQueryBuilder(type); const rows = await qb - .init(on && type in on ? on[type] : typePopulate) + .init(on?.[type] ?? typePopulate) .addSelect(`${qb.alias}.${idColumn.referencedColumn}`) .where({ [idColumn.referencedColumn]: ids }) .execute({ mapResults: false }); diff --git a/packages/core/strapi/tests/api/populate/filtering/index.test.api.js b/packages/core/strapi/tests/api/populate/filtering/index.test.api.js index 24931e882f..44c39d34e7 100644 --- a/packages/core/strapi/tests/api/populate/filtering/index.test.api.js +++ b/packages/core/strapi/tests/api/populate/filtering/index.test.api.js @@ -315,4 +315,64 @@ describe('Populate filters', () => { expect(body.data[0].attributes.third).toBeUndefined(); }); }); + + describe('Populate a dynamic zone', () => { + test('Populate every components in the dynamic zone', async () => { + const qs = { + populate: { + dz: '*', + }, + }; + + const { status, body } = await rq.get(`/${schemas.contentTypes.b.pluralName}`, { qs }); + + expect(status).toBe(200); + expect(body.data).toHaveLength(2); + expect(body.data[0].attributes.dz).toHaveLength(3); + expect(body.data[1].attributes.dz).toHaveLength(1); + }); + + test('Populate only one component type using fragment', async () => { + const qs = { + populate: { + dz: { + on: { + 'default.foo': true, + }, + }, + }, + }; + + const { status, body } = await rq.get(`/${schemas.contentTypes.b.pluralName}`, { qs }); + + expect(status).toBe(200); + expect(body.data).toHaveLength(2); + expect(body.data[0].attributes.dz).toHaveLength(2); + expect(body.data[1].attributes.dz).toHaveLength(0); + }); + + test('Populate the dynamic zone with filters in fragments', async () => { + const qs = { + populate: { + dz: { + on: { + 'default.foo': { + filters: { number: { $lt: 2 } }, + }, + 'default.bar': { + filters: { title: { $contains: 'another' } }, + }, + }, + }, + }, + }; + + const { status, body } = await rq.get(`/${schemas.contentTypes.b.pluralName}`, { qs }); + + expect(status).toBe(200); + expect(body.data).toHaveLength(2); + expect(body.data[0].attributes.dz).toHaveLength(1); + expect(body.data[1].attributes.dz).toHaveLength(1); + }); + }); }); diff --git a/packages/core/utils/lib/convert-query-params.js b/packages/core/utils/lib/convert-query-params.js index cd274b97d7..182a1ff8bd 100644 --- a/packages/core/utils/lib/convert-query-params.js +++ b/packages/core/utils/lib/convert-query-params.js @@ -186,7 +186,15 @@ const convertPopulateObject = (populate, schema) => { return acc; } - if (typeof subPopulate === 'object' && 'on' in subPopulate) { + // Allow adding a 'on' strategy to populate queries for polymorphic relations, media and dynamic zones + const isAllowedAttributeForFragmentPopulate = + attribute.type === 'dynamiczone' || + attribute.type === 'media' || + (attribute.relation && attribute.relation.startsWith('morphTo')); + + const hasFragmentPopulateDefined = typeof subPopulate === 'object' && 'on' in subPopulate; + + if (isAllowedAttributeForFragmentPopulate && hasFragmentPopulateDefined) { return { ...acc, [key]: { @@ -201,8 +209,8 @@ const convertPopulateObject = (populate, schema) => { }; } - // TODO: Deprecated way of handling dynamic zone populate queries. It's kept as is, - // as removing it could break existing user queries but should be removed in V5. + // TODO: This is a query's populate fallback for DynamicZone and is kept for legacy purpose. + // Removing it could break existing user queries but it should be removed in V5. if (attribute.type === 'dynamiczone') { const populates = attribute.components .map((uid) => strapi.getModel(uid)) From b784135413e9b5adee7b24e6d9bba5398939b415 Mon Sep 17 00:00:00 2001 From: Convly Date: Mon, 21 Nov 2022 14:20:59 +0100 Subject: [PATCH 5/6] Fix typo --- .../core/strapi/tests/api/populate/filtering/index.test.api.js | 2 +- packages/core/utils/lib/convert-query-params.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/strapi/tests/api/populate/filtering/index.test.api.js b/packages/core/strapi/tests/api/populate/filtering/index.test.api.js index 44c39d34e7..f0c5103df9 100644 --- a/packages/core/strapi/tests/api/populate/filtering/index.test.api.js +++ b/packages/core/strapi/tests/api/populate/filtering/index.test.api.js @@ -317,7 +317,7 @@ describe('Populate filters', () => { }); describe('Populate a dynamic zone', () => { - test('Populate every components in the dynamic zone', async () => { + test('Populate every component in the dynamic zone', async () => { const qs = { populate: { dz: '*', diff --git a/packages/core/utils/lib/convert-query-params.js b/packages/core/utils/lib/convert-query-params.js index 182a1ff8bd..ec77ca23ce 100644 --- a/packages/core/utils/lib/convert-query-params.js +++ b/packages/core/utils/lib/convert-query-params.js @@ -186,7 +186,7 @@ const convertPopulateObject = (populate, schema) => { return acc; } - // Allow adding a 'on' strategy to populate queries for polymorphic relations, media and dynamic zones + // Allow adding an 'on' strategy to populate queries for polymorphic relations, media and dynamic zones const isAllowedAttributeForFragmentPopulate = attribute.type === 'dynamiczone' || attribute.type === 'media' || From c49df55a6780bf60f3fc8c7344240454398d7148 Mon Sep 17 00:00:00 2001 From: Convly Date: Tue, 22 Nov 2022 11:40:37 +0100 Subject: [PATCH 6/6] Update tests & add type util to convert query params --- .../api/populate/filtering/index.test.api.js | 51 +++++++++++++++++-- packages/core/utils/lib/content-types.js | 16 ++++-- .../core/utils/lib/convert-query-params.js | 11 ++-- 3 files changed, 65 insertions(+), 13 deletions(-) diff --git a/packages/core/strapi/tests/api/populate/filtering/index.test.api.js b/packages/core/strapi/tests/api/populate/filtering/index.test.api.js index f0c5103df9..279bedddb3 100644 --- a/packages/core/strapi/tests/api/populate/filtering/index.test.api.js +++ b/packages/core/strapi/tests/api/populate/filtering/index.test.api.js @@ -108,6 +108,11 @@ const fixtures = { number: 2, field: 'short string', }, + { + __component: 'default.foo', + number: 3, + field: 'long string', + }, { __component: 'default.bar', title: 'this is a title', @@ -328,8 +333,19 @@ describe('Populate filters', () => { expect(status).toBe(200); expect(body.data).toHaveLength(2); - expect(body.data[0].attributes.dz).toHaveLength(3); - expect(body.data[1].attributes.dz).toHaveLength(1); + + fixtures.b.forEach((fixture, i) => { + const res = body.data[i]; + const { dz } = res.attributes; + + expect(dz).toHaveLength(fixture.dz.length); + expect(dz).toMatchObject( + fixture.dz.map((component) => ({ + ...omit('field', component), + id: expect.any(Number), + })) + ); + }); }); test('Populate only one component type using fragment', async () => { @@ -347,8 +363,18 @@ describe('Populate filters', () => { expect(status).toBe(200); expect(body.data).toHaveLength(2); - expect(body.data[0].attributes.dz).toHaveLength(2); + + expect(body.data[0].attributes.dz).toHaveLength(3); expect(body.data[1].attributes.dz).toHaveLength(0); + + const expected = fixtures.b[0].dz + .filter(({ __component }) => __component === 'default.foo') + .map((component) => ({ + ...component, + id: expect.any(Number), + })); + + expect(body.data[0].attributes.dz).toMatchObject(expected); }); test('Populate the dynamic zone with filters in fragments', async () => { @@ -357,7 +383,7 @@ describe('Populate filters', () => { dz: { on: { 'default.foo': { - filters: { number: { $lt: 2 } }, + filters: { number: { $lt: 3 } }, }, 'default.bar': { filters: { title: { $contains: 'another' } }, @@ -371,8 +397,23 @@ describe('Populate filters', () => { expect(status).toBe(200); expect(body.data).toHaveLength(2); - expect(body.data[0].attributes.dz).toHaveLength(1); + expect(body.data[0].attributes.dz).toHaveLength(2); expect(body.data[1].attributes.dz).toHaveLength(1); + + const filter = (data = []) => + data + .filter(({ __component, number, title }) => { + if (__component === 'default.foo') return number < 3; + if (__component === 'default.bar') return title.includes('another'); + return false; + }) + .map((component) => ({ + ...(component.__component === 'default.foo' ? component : omit('field', component)), + id: expect.any(Number), + })); + + expect(body.data[0].attributes.dz).toMatchObject(filter(fixtures.b[0].dz)); + expect(body.data[1].attributes.dz).toMatchObject(filter(fixtures.b[1].dz)); }); }); }); diff --git a/packages/core/utils/lib/content-types.js b/packages/core/utils/lib/content-types.js index cec115eab4..0303b8d2a5 100644 --- a/packages/core/utils/lib/content-types.js +++ b/packages/core/utils/lib/content-types.js @@ -104,12 +104,16 @@ const isPrivateAttribute = (model = {}, attributeName) => { }; const isScalarAttribute = (attribute) => { - return !['media', 'component', 'relation', 'dynamiczone'].includes(attribute.type); + return !['media', 'component', 'relation', 'dynamiczone'].includes(attribute?.type); +}; +const isMediaAttribute = (attribute) => attribute?.type === 'media'; +const isRelationalAttribute = (attribute) => attribute?.type === 'relation'; +const isComponentAttribute = (attribute) => ['component', 'dynamiczone'].includes(attribute?.type); + +const isDynamicZoneAttribute = (attribute) => attribute?.type === 'dynamiczone'; +const isMorphToRelationalAttribute = (attribute) => { + return isRelationalAttribute(attribute) && attribute?.relation?.startsWith?.('morphTo'); }; -const isMediaAttribute = (attribute) => attribute && attribute.type === 'media'; -const isRelationalAttribute = (attribute) => attribute && attribute.type === 'relation'; -const isComponentAttribute = (attribute) => - attribute && ['component', 'dynamiczone'].includes(attribute.type); const getComponentAttributes = (schema) => { return _.reduce( @@ -158,6 +162,8 @@ module.exports = { isMediaAttribute, isRelationalAttribute, isComponentAttribute, + isDynamicZoneAttribute, + isMorphToRelationalAttribute, isTypedAttribute, getPrivateAttributes, isPrivateAttribute, diff --git a/packages/core/utils/lib/convert-query-params.js b/packages/core/utils/lib/convert-query-params.js index ec77ca23ce..3dea86ee7b 100644 --- a/packages/core/utils/lib/convert-query-params.js +++ b/packages/core/utils/lib/convert-query-params.js @@ -23,6 +23,11 @@ const _ = require('lodash'); const parseType = require('./parse-type'); const contentTypesUtils = require('./content-types'); const { PaginationError } = require('./errors'); +const { + isMediaAttribute, + isDynamicZoneAttribute, + isMorphToRelationalAttribute, +} = require('./content-types'); const { PUBLISHED_AT_ATTRIBUTE } = contentTypesUtils.constants; @@ -188,9 +193,9 @@ const convertPopulateObject = (populate, schema) => { // Allow adding an 'on' strategy to populate queries for polymorphic relations, media and dynamic zones const isAllowedAttributeForFragmentPopulate = - attribute.type === 'dynamiczone' || - attribute.type === 'media' || - (attribute.relation && attribute.relation.startsWith('morphTo')); + isDynamicZoneAttribute(attribute) || + isMediaAttribute(attribute) || + isMorphToRelationalAttribute(attribute); const hasFragmentPopulateDefined = typeof subPopulate === 'object' && 'on' in subPopulate;