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 <marc12info@gmail.com>

* Update packages/core/admin/ee/server/services/review-workflows/review-workflows.js

Co-authored-by: Marc <marc12info@gmail.com>

* chore: remove unused import

* test(review-workflows): update unit tests

* test: disable review workflows

---------

Co-authored-by: Marc <marc12info@gmail.com>

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)
+    });
+  });
 });

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',

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(),
+  get: jest.fn((container) => {
-  extend: jest.fn(),
+    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(contentTypesContainer.extend).toHaveBeenCalledTimes(1);
-      expect(containerMock.extend).not.toHaveBeenCalledWith('test1', expect.any(Function));
+      expect(contentTypesContainer.extend).not.toHaveBeenCalledWith('test1', expect.any(Function));
-      expect(containerMock.extend).toHaveBeenCalledWith('test2', 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',

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)])(
+    const joinTablesToPersist = pipe([
-      contentTypes
+      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');

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
+  // Pick only content-types visible in the content-manager and option is not false
-  pickBy(get('options.reviewWorkflows')),
+  pickBy(
-  // Pick only content-types visible in the content-manager
+    (value) =>
-  // pickBy(getOr(true, 'pluginOptions.content-manager.visible')),
+      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,
 };

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
+        const model = strapi.getModel(uid);
-        return getVisibleContentTypesUID({ [uid]: strapi.getModel(uid) }).includes(uid);
+
+        // It's not a valid content type if it doesn't have the stage attribute
+        return hasStageAttribute(model);
       },
     })
 );