From b8ca9a01ce2d6d3b22d01802c8b6d5e8f91cd8d9 Mon Sep 17 00:00:00 2001 From: Nathan Pichon Date: Thu, 1 Jun 2023 10:17:00 +0200 Subject: [PATCH] feat(review workflow): Prevent too long table name for rw join table attributes (#16827) * feat(review-workflow): prevent activating on content-type with a too long name * chore(review-workflow): update warning message * test(review-workflows): add a test for too long name content type * test(review-workflows): fix describe condition * chore(review-workflows): rename variable and constant * chore(review-workflows): rename constants * test(review-workflows): update test content type name length * feat(review-workflows): validate that the content type have rw activated * feat(review-workflows): simplify the validation step * Update packages/core/admin/ee/server/services/review-workflows/review-workflows.js Co-authored-by: Marc * Update packages/core/admin/ee/server/services/review-workflows/review-workflows.js Co-authored-by: Marc * chore: remove unused import * test(review-workflows): update unit tests * test: disable review workflows --------- Co-authored-by: Marc --- ...review-workflows-content-types.test.api.js | 29 ++++++++++++++-- .../core/content-manager/search.test.api.js | 3 ++ .../__tests__/review-workflows.test.js | 29 +++++++++++----- .../review-workflows/review-workflows.js | 33 +++++++++++++++---- .../admin/ee/server/utils/review-workflows.js | 16 ++++++--- .../ee/server/validation/review-workflows.js | 8 +++-- 6 files changed, 93 insertions(+), 25 deletions(-) diff --git a/api-tests/core/admin/ee/review-workflows-content-types.test.api.js b/api-tests/core/admin/ee/review-workflows-content-types.test.api.js index fde65ef9b9..2555e95797 100644 --- a/api-tests/core/admin/ee/review-workflows-content-types.test.api.js +++ b/api-tests/core/admin/ee/review-workflows-content-types.test.api.js @@ -30,8 +30,20 @@ const productModel = { type: 'string', }, }, - options: { - reviewWorkflows: true, +}; +const longCTUID = + 'api::thatsanabsurdreallyreallylongcontenttypename.thatsanabsurdreallyreallylongcontenttypename'; +const longCTModel = { + draftAndPublish: true, + pluginOptions: {}, + singularName: 'thatsanabsurdreallyreallylongcontenttypename', + pluralName: 'thatsanabsurdreallyreallylongcontenttypenamewithans', + displayName: 'Thats an absurd really really long content type name', + kind: 'collectionType', + attributes: { + name: { + type: 'string', + }, }, }; @@ -90,7 +102,7 @@ describeOnCondition(edition === 'EE')('Review workflows', () => { }; beforeAll(async () => { - await builder.addContentTypes([productModel]).build(); + await builder.addContentTypes([productModel, longCTModel]).build(); strapi = await createStrapiInstance(); requests.admin = await createAuthRequest({ strapi }); @@ -98,6 +110,8 @@ describeOnCondition(edition === 'EE')('Review workflows', () => { await Promise.all([ createEntry(productUID, { name: 'Product 1' }), createEntry(productUID, { name: 'Product 2' }), + createEntry(longCTUID, { name: 'Product 1' }), + createEntry(longCTUID, { name: 'Product 2' }), ]); }); @@ -331,4 +345,13 @@ describeOnCondition(edition === 'EE')('Review workflows', () => { }); }); }); + + describe('With long content type names', () => { + test('Should not load Review Workflow on too long content-type name', async () => { + const contentType = strapi.contentTypes[longCTUID]; + + expect(contentType.attributes[ENTITY_STAGE_ATTRIBUTE]).toBeUndefined(); + // Cannot test the log as it happens during the Strapi instance creation (in the beforeAll) + }); + }); }); diff --git a/api-tests/core/content-manager/search.test.api.js b/api-tests/core/content-manager/search.test.api.js index bab9ee8db4..b0f08ea02d 100644 --- a/api-tests/core/content-manager/search.test.api.js +++ b/api-tests/core/content-manager/search.test.api.js @@ -20,6 +20,9 @@ const bedModel = { singularName: 'bed', pluralName: 'beds', kind: 'collectionType', + options: { + reviewWorkflows: false, + }, attributes: { name: { type: 'string', diff --git a/packages/core/admin/ee/server/services/__tests__/review-workflows.test.js b/packages/core/admin/ee/server/services/__tests__/review-workflows.test.js index f0e3e4e95c..e64983aefc 100644 --- a/packages/core/admin/ee/server/services/__tests__/review-workflows.test.js +++ b/packages/core/admin/ee/server/services/__tests__/review-workflows.test.js @@ -43,16 +43,28 @@ const contentTypesMock = { visible: false, }, }, + collectionName: 'test1', }, test2: { - options: { reviewWorkflows: true }, attributes: {}, + collectionName: 'test2', }, }; +const contentTypesContainer = { + get: jest.fn((uid) => contentTypesMock[uid]), + extend: jest.fn((uid, callback) => callback(contentTypesMock[uid])), +}; + const containerMock = { - get: jest.fn().mockReturnThis(), - extend: jest.fn(), + get: jest.fn((container) => { + switch (container) { + case 'content-types': + return contentTypesContainer; + default: + return null; + } + }), }; const hookMock = jest.fn().mockReturnValue({ register: jest.fn() }); @@ -114,13 +126,14 @@ describe('Review workflows service', () => { describe('register', () => { test('Content types with review workflows options should have a new attribute', async () => { await reviewWorkflowsService.register(); - expect(containerMock.extend).toHaveBeenCalledTimes(1); - expect(containerMock.extend).not.toHaveBeenCalledWith('test1', expect.any(Function)); - expect(containerMock.extend).toHaveBeenCalledWith('test2', expect.any(Function)); + expect(contentTypesContainer.extend).toHaveBeenCalledTimes(1); + expect(contentTypesContainer.extend).not.toHaveBeenCalledWith('test1', expect.any(Function)); + expect(contentTypesContainer.extend).toHaveBeenCalledWith('test2', expect.any(Function)); - const extendFunc = containerMock.extend.mock.calls[0][1]; + const extendFunc = contentTypesContainer.extend.mock.calls[0][1]; - expect(extendFunc({})).toEqual({ + expect(extendFunc({ collectionName: 'toto' })).toEqual({ + collectionName: 'toto', attributes: { [ENTITY_STAGE_ATTRIBUTE]: expect.objectContaining({ relation: 'oneToOne', diff --git a/packages/core/admin/ee/server/services/review-workflows/review-workflows.js b/packages/core/admin/ee/server/services/review-workflows/review-workflows.js index 8b6133b054..6cf76bb56e 100644 --- a/packages/core/admin/ee/server/services/review-workflows/review-workflows.js +++ b/packages/core/admin/ee/server/services/review-workflows/review-workflows.js @@ -1,8 +1,8 @@ 'use strict'; -const { set, forEach, pipe, map } = require('lodash/fp'); +const { filter, set, forEach, pipe, map, stubTrue, cond } = require('lodash/fp'); const { getService } = require('../../utils'); -const { getVisibleContentTypesUID } = require('../../utils/review-workflows'); +const { getVisibleContentTypesUID, hasStageAttribute } = require('../../utils/review-workflows'); const defaultStages = require('../../constants/default-stages.json'); const defaultWorkflow = require('../../constants/default-workflow.json'); @@ -10,6 +10,12 @@ const { ENTITY_STAGE_ATTRIBUTE } = require('../../constants/workflows'); const { persistTables, removePersistedTablesWithSuffix } = require('../../utils/persisted-tables'); +const MAX_DB_TABLE_NAME_LEN = 63; // Postgres limit +// The longest index name that Strapi can create is prefixed with '_strapi_reviewWorkflow_stage_links_inv_fk', so the content type name should be no longer than this. +const MAX_JOIN_TABLE_NAME_SUFFIX = + 1 /* _ */ + ENTITY_STAGE_ATTRIBUTE.length + '_links_inv_fk'.length; +const MAX_CONTENT_TYPE_NAME_LEN = MAX_DB_TABLE_NAME_LEN - MAX_JOIN_TABLE_NAME_SUFFIX; + async function initDefaultWorkflow({ workflowsService, stagesService }) { const wfCount = await workflowsService.count(); const stagesCount = await stagesService.count(); @@ -28,6 +34,14 @@ async function initDefaultWorkflow({ workflowsService, stagesService }) { function extendReviewWorkflowContentTypes({ strapi }) { const extendContentType = (contentTypeUID) => { + const assertContentTypeCompatibility = (contentType) => + contentType.collectionName.length <= MAX_CONTENT_TYPE_NAME_LEN; + const incompatibleContentTypeAlert = (contentType) => { + strapi.log.warn( + `Review Workflow cannot be activated for the content type with the name '${contentType.info.displayName}' because the name exceeds the maximum length of ${MAX_CONTENT_TYPE_NAME_LEN} characters.` + ); + return contentType; + }; const setStageAttribute = set(`attributes.${ENTITY_STAGE_ATTRIBUTE}`, { writable: true, private: false, @@ -38,7 +52,12 @@ function extendReviewWorkflowContentTypes({ strapi }) { relation: 'oneToOne', target: 'admin::workflow-stage', }); - strapi.container.get('content-types').extend(contentTypeUID, setStageAttribute); + + const extendContentTypeIfCompatible = cond([ + [assertContentTypeCompatibility, setStageAttribute], + [stubTrue, incompatibleContentTypeAlert], + ]); + strapi.container.get('content-types').extend(contentTypeUID, extendContentTypeIfCompatible); }; pipe([ @@ -57,9 +76,11 @@ function persistStagesJoinTables({ strapi }) { return { name: joinTableName, dependsOn: [{ name: tableName }] }; }; - const joinTablesToPersist = pipe([getVisibleContentTypesUID, map(getStageTableToPersist)])( - contentTypes - ); + const joinTablesToPersist = pipe([ + getVisibleContentTypesUID, + filter(hasStageAttribute), + map(getStageTableToPersist), + ])(contentTypes); // TODO: Instead of removing all the tables, we should only remove the ones that are not in the joinTablesToPersist await removePersistedTablesWithSuffix('_strapi_review_workflows_stage_links'); diff --git a/packages/core/admin/ee/server/utils/review-workflows.js b/packages/core/admin/ee/server/utils/review-workflows.js index c850c18910..0d6ab6c80a 100644 --- a/packages/core/admin/ee/server/utils/review-workflows.js +++ b/packages/core/admin/ee/server/utils/review-workflows.js @@ -1,16 +1,22 @@ 'use strict'; -const { get, keys, pickBy, pipe } = require('lodash/fp'); +const { getOr, keys, pickBy, pipe, has } = require('lodash/fp'); +const { ENTITY_STAGE_ATTRIBUTE } = require('../constants/workflows'); const getVisibleContentTypesUID = pipe([ - // FIXME: Swap with the commented line below when figure out how to shorten strapi_reviewWorkflows_stage - pickBy(get('options.reviewWorkflows')), - // Pick only content-types visible in the content-manager - // pickBy(getOr(true, 'pluginOptions.content-manager.visible')), + // Pick only content-types visible in the content-manager and option is not false + pickBy( + (value) => + getOr(true, 'pluginOptions.content-manager.visible', value) && + getOr(true, 'options.reviewWorkflows', value) !== false + ), // Get UIDs keys, ]); +const hasStageAttribute = has(['attributes', ENTITY_STAGE_ATTRIBUTE]); + module.exports = { getVisibleContentTypesUID, + hasStageAttribute, }; diff --git a/packages/core/admin/ee/server/validation/review-workflows.js b/packages/core/admin/ee/server/validation/review-workflows.js index 0bf60ce99d..49bd8e56d5 100644 --- a/packages/core/admin/ee/server/validation/review-workflows.js +++ b/packages/core/admin/ee/server/validation/review-workflows.js @@ -1,7 +1,7 @@ 'use strict'; const { yup, validateYupSchema } = require('@strapi/utils'); -const { getVisibleContentTypesUID } = require('../utils/review-workflows'); +const { hasStageAttribute } = require('../utils/review-workflows'); const stageObject = yup.object().shape({ id: yup.number().integer().min(1), @@ -33,8 +33,10 @@ const validateContentTypes = yup.array().of( message: (value) => `Content type ${value.originalValue} does not have review workflow enabled`, test(uid) { - // It's not a valid content type if it's not visible in the content manager - return getVisibleContentTypesUID({ [uid]: strapi.getModel(uid) }).includes(uid); + const model = strapi.getModel(uid); + + // It's not a valid content type if it doesn't have the stage attribute + return hasStageAttribute(model); }, }) );