From 1c2f2708ff02b720ca07b50f0374080c2af88ad8 Mon Sep 17 00:00:00 2001 From: markkaylor Date: Thu, 21 Mar 2024 15:20:57 +0100 Subject: [PATCH] feat(history): diff schemas to get unknown attributes (#19849) --- .../server/src/history/constants.ts | 10 + .../services/__tests__/history.test.ts | 223 +++++++++--------- .../history/services/__tests__/utils.test.ts | 63 +++++ .../server/src/history/services/history.ts | 33 +-- .../server/src/history/services/utils.ts | 44 ++++ .../shared/contracts/history-versions.ts | 6 + 6 files changed, 255 insertions(+), 124 deletions(-) create mode 100644 packages/core/content-manager/server/src/history/services/__tests__/utils.test.ts create mode 100644 packages/core/content-manager/server/src/history/services/utils.ts diff --git a/packages/core/content-manager/server/src/history/constants.ts b/packages/core/content-manager/server/src/history/constants.ts index eda6aa83c2..1050511a99 100644 --- a/packages/core/content-manager/server/src/history/constants.ts +++ b/packages/core/content-manager/server/src/history/constants.ts @@ -1 +1,11 @@ export const HISTORY_VERSION_UID = 'plugin::content-manager.history-version'; +export const FIELDS_TO_IGNORE = [ + 'createdAt', + 'updatedAt', + 'publishedAt', + 'createdBy', + 'updatedBy', + 'locale', + 'strapi_stage', + 'strapi_assignee', +]; diff --git a/packages/core/content-manager/server/src/history/services/__tests__/history.test.ts b/packages/core/content-manager/server/src/history/services/__tests__/history.test.ts index 2c42f68ed5..a9a5f63a8f 100644 --- a/packages/core/content-manager/server/src/history/services/__tests__/history.test.ts +++ b/packages/core/content-manager/server/src/history/services/__tests__/history.test.ts @@ -35,6 +35,7 @@ const mockStrapi = { i18n: { service: jest.fn(() => ({ getDefaultLocale: jest.fn().mockReturnValue('en'), + find: jest.fn(), })), }, }, @@ -95,136 +96,140 @@ describe('history-version service', () => { jest.useRealTimers(); }); - it('inits service only once', () => { - historyService.bootstrap(); - historyService.bootstrap(); - // @ts-expect-error - ignore - expect(mockStrapi.documents.use).toHaveBeenCalledTimes(1); - }); + describe('boostrap', () => { + it('inits service only once', () => { + historyService.bootstrap(); + historyService.bootstrap(); + // @ts-expect-error - ignore + expect(mockStrapi.documents.use).toHaveBeenCalledTimes(1); + }); - it('saves relevant document actions in history', async () => { - const context = { - action: 'create', - contentType: { - uid: 'api::article.article', - }, - args: [ - { - locale: 'fr', + it('saves relevant document actions in history', async () => { + const context = { + action: 'create', + contentType: { + uid: 'api::article.article', }, - ], - }; + args: [ + { + locale: 'fr', + }, + ], + }; - const next = jest.fn((context) => ({ ...context, documentId: 'document-id' })); - await historyService.bootstrap(); - // @ts-expect-error - ignore - const historyMiddlewareFunction = mockStrapi.documents.use.mock.calls[0][0]; + const next = jest.fn((context) => ({ ...context, documentId: 'document-id' })); + await historyService.bootstrap(); + // @ts-expect-error - ignore + const historyMiddlewareFunction = mockStrapi.documents.use.mock.calls[0][0]; - // Check that we don't break the middleware chain - await historyMiddlewareFunction(context, next); - expect(next).toHaveBeenCalledWith(context); + // Check that we don't break the middleware chain + await historyMiddlewareFunction(context, next); + expect(next).toHaveBeenCalledWith(context); - // Create and update actions should be saved in history - expect(createMock).toHaveBeenCalled(); - context.action = 'update'; - await historyMiddlewareFunction(context, next); - expect(createMock).toHaveBeenCalledTimes(2); + // Create and update actions should be saved in history + expect(createMock).toHaveBeenCalled(); + context.action = 'update'; + await historyMiddlewareFunction(context, next); + expect(createMock).toHaveBeenCalledTimes(2); - // Publish and unpublish actions should be saved in history - createMock.mockClear(); - await historyMiddlewareFunction(context, next); - context.action = 'unpublish'; - await historyMiddlewareFunction(context, next); - expect(createMock).toHaveBeenCalledTimes(2); + // Publish and unpublish actions should be saved in history + createMock.mockClear(); + await historyMiddlewareFunction(context, next); + context.action = 'unpublish'; + await historyMiddlewareFunction(context, next); + expect(createMock).toHaveBeenCalledTimes(2); - // Other actions should be ignored - createMock.mockClear(); - context.action = 'findOne'; - await historyMiddlewareFunction(context, next); - context.action = 'delete'; - await historyMiddlewareFunction(context, next); - expect(createMock).toHaveBeenCalledTimes(0); + // Other actions should be ignored + createMock.mockClear(); + context.action = 'findOne'; + await historyMiddlewareFunction(context, next); + context.action = 'delete'; + await historyMiddlewareFunction(context, next); + expect(createMock).toHaveBeenCalledTimes(0); - // Non-api content types should be ignored - createMock.mockClear(); - context.contentType.uid = 'plugin::upload.file'; - context.action = 'create'; - await historyMiddlewareFunction(context, next); - expect(createMock).toHaveBeenCalledTimes(0); + // Non-api content types should be ignored + createMock.mockClear(); + context.contentType.uid = 'plugin::upload.file'; + context.action = 'create'; + await historyMiddlewareFunction(context, next); + expect(createMock).toHaveBeenCalledTimes(0); - // Don't break middleware chain even if we don't save the action in history - next.mockClear(); - await historyMiddlewareFunction(context, next); - expect(next).toHaveBeenCalledWith(context); - }); + // Don't break middleware chain even if we don't save the action in history + next.mockClear(); + await historyMiddlewareFunction(context, next); + expect(next).toHaveBeenCalledWith(context); + }); - it('creates a history version with the author', async () => { - jest.useFakeTimers().setSystemTime(fakeDate); + it('should create a cron job that runs once a day', async () => { + // @ts-expect-error - this is a mock + const mockScheduleJob = scheduleJob.mockImplementationOnce( + jest.fn((rule, callback) => callback()) + ); - const historyVersionData = { - contentType: 'api::article.article' as UID.ContentType, - data: { - title: 'My article', - }, - locale: 'en', - relatedDocumentId: 'randomid', - schema: { - title: { - type: 'string', - }, - }, - status: 'draft' as const, - }; + await historyService.bootstrap(); - await historyService.createVersion(historyVersionData); - expect(createMock).toHaveBeenCalledWith({ - data: { - ...historyVersionData, - createdBy: userId, - createdAt: fakeDate, - }, + expect(mockScheduleJob).toHaveBeenCalledTimes(1); + expect(mockScheduleJob).toHaveBeenCalledWith('0 0 * * *', expect.any(Function)); }); }); - it('creates a history version without any author', async () => { - jest.useFakeTimers().setSystemTime(fakeDate); + describe('createVersion', () => { + it('creates a history version with the author', async () => { + jest.useFakeTimers().setSystemTime(fakeDate); - const historyVersionData = { - contentType: 'api::article.article' as UID.ContentType, - data: { - title: 'My article', - }, - locale: 'en', - relatedDocumentId: 'randomid', - schema: { - title: { - type: 'string', + const historyVersionData = { + contentType: 'api::article.article' as UID.ContentType, + data: { + title: 'My article', }, - }, - status: null, - }; + locale: 'en', + relatedDocumentId: 'randomid', + schema: { + title: { + type: 'string', + }, + }, + status: 'draft' as const, + }; - mockGetRequestContext.mockReturnValueOnce(null as any); - - await historyService.createVersion(historyVersionData); - expect(createMock).toHaveBeenCalledWith({ - data: { - ...historyVersionData, - createdBy: undefined, - createdAt: fakeDate, - }, + await historyService.createVersion(historyVersionData); + expect(createMock).toHaveBeenCalledWith({ + data: { + ...historyVersionData, + createdBy: userId, + createdAt: fakeDate, + }, + }); }); - }); - it('should create a cron job that runs once a day', async () => { - // @ts-expect-error - this is a mock - const mockScheduleJob = scheduleJob.mockImplementationOnce( - jest.fn((rule, callback) => callback()) - ); + it('creates a history version without any author', async () => { + jest.useFakeTimers().setSystemTime(fakeDate); - await historyService.bootstrap(); + const historyVersionData = { + contentType: 'api::article.article' as UID.ContentType, + data: { + title: 'My article', + }, + locale: 'en', + relatedDocumentId: 'randomid', + schema: { + title: { + type: 'string', + }, + }, + status: null, + }; - expect(mockScheduleJob).toHaveBeenCalledTimes(1); - expect(mockScheduleJob).toHaveBeenCalledWith('0 0 * * *', expect.any(Function)); + mockGetRequestContext.mockReturnValueOnce(null as any); + + await historyService.createVersion(historyVersionData); + expect(createMock).toHaveBeenCalledWith({ + data: { + ...historyVersionData, + createdBy: undefined, + createdAt: fakeDate, + }, + }); + }); }); }); diff --git a/packages/core/content-manager/server/src/history/services/__tests__/utils.test.ts b/packages/core/content-manager/server/src/history/services/__tests__/utils.test.ts new file mode 100644 index 0000000000..e95d97b13a --- /dev/null +++ b/packages/core/content-manager/server/src/history/services/__tests__/utils.test.ts @@ -0,0 +1,63 @@ +import { getSchemaAttributesDiff } from '../utils'; + +describe('history-version service utils', () => { + describe('getSchemaAttributesDiff', () => { + it('should return a diff', () => { + const versionSchema = { + title: { + type: 'string', + }, + someOtherField: { + type: 'string', + }, + }; + const contentTypeSchema = { + renamed: { + type: 'string', + }, + newField: { + type: 'string', + }, + someOtherField: { + type: 'string', + }, + }; + + // @ts-expect-error ignore + const { added, removed } = getSchemaAttributesDiff(versionSchema, contentTypeSchema); + + expect(added).toEqual({ + renamed: { + type: 'string', + }, + newField: { + type: 'string', + }, + }); + expect(removed).toEqual({ + title: { + type: 'string', + }, + }); + }); + + it('should not return a diff', () => { + const versionSchema = { + title: { + type: 'string', + }, + }; + const contentTypeSchema = { + title: { + type: 'string', + }, + }; + + // @ts-expect-error ignore + const { added, removed } = getSchemaAttributesDiff(versionSchema, contentTypeSchema); + + expect(added).toEqual({}); + expect(removed).toEqual({}); + }); + }); +}); diff --git a/packages/core/content-manager/server/src/history/services/history.ts b/packages/core/content-manager/server/src/history/services/history.ts index 7587399dcb..a5376948cb 100644 --- a/packages/core/content-manager/server/src/history/services/history.ts +++ b/packages/core/content-manager/server/src/history/services/history.ts @@ -3,8 +3,9 @@ import { omit, pick } from 'lodash/fp'; import { scheduleJob } from 'node-schedule'; -import { HISTORY_VERSION_UID } from '../constants'; +import { FIELDS_TO_IGNORE, HISTORY_VERSION_UID } from '../constants'; import type { HistoryVersions } from '../../../../shared/contracts'; +import { getSchemaAttributesDiff } from './utils'; const DEFAULT_RETENTION_DAYS = 90; @@ -104,24 +105,13 @@ const createHistoryService = ({ strapi }: { strapi: Core.LoadedStrapi }) => { }); const status = await getVersionStatus(contentTypeUid, document); - const fieldsToIgnore = [ - 'createdAt', - 'updatedAt', - 'publishedAt', - 'createdBy', - 'updatedBy', - 'locale', - 'strapi_stage', - 'strapi_assignee', - ]; - // Prevent creating a history version for an action that wasn't actually executed await strapi.db.transaction(async ({ onCommit }) => { onCommit(() => { this.createVersion({ contentType: contentTypeUid, - data: omit(fieldsToIgnore, document), - schema: omit(fieldsToIgnore, strapi.contentType(contentTypeUid).attributes), + data: omit(FIELDS_TO_IGNORE, document), + schema: omit(FIELDS_TO_IGNORE, strapi.contentType(contentTypeUid).attributes), relatedDocumentId: documentContext.documentId, locale, status, @@ -183,7 +173,20 @@ const createHistoryService = ({ strapi }: { strapi: Core.LoadedStrapi }) => { getLocaleDictionary(), ]); - const sanitizedResults = results.map((result) => ({ + const versionsWithMeta = results.map((version) => { + const { added, removed } = getSchemaAttributesDiff( + version.schema, + strapi.getModel(params.contentType).attributes + ); + const hasSchemaDiff = Object.keys(added).length > 0 || Object.keys(removed).length > 0; + + return { + ...version, + ...(hasSchemaDiff ? { meta: { unknownAttributes: { added, removed } } } : {}), + }; + }); + + const sanitizedResults = versionsWithMeta.map((result) => ({ ...result, locale: result.locale ? localeDictionary[result.locale] : null, createdBy: result.createdBy diff --git a/packages/core/content-manager/server/src/history/services/utils.ts b/packages/core/content-manager/server/src/history/services/utils.ts new file mode 100644 index 0000000000..77861c9d34 --- /dev/null +++ b/packages/core/content-manager/server/src/history/services/utils.ts @@ -0,0 +1,44 @@ +import { difference, omit } from 'lodash/fp'; +import { Schema } from '@strapi/types'; +import { CreateHistoryVersion } from '../../../../shared/contracts/history-versions'; +import { FIELDS_TO_IGNORE } from '../constants'; + +/** + * Get the difference between the version schema and the content type schema + * Returns the attributes with their original shape + */ +export const getSchemaAttributesDiff = ( + versionSchemaAttributes: CreateHistoryVersion['schema'], + contentTypeSchemaAttributes: Schema.Attributes +) => { + // Omit the same fields that were omitted when creating a history version + const sanitizedContentTypeSchemaAttributes = omit( + FIELDS_TO_IGNORE, + contentTypeSchemaAttributes + ) as CreateHistoryVersion['schema']; + + const reduceDifferenceToAttributesObject = ( + diffKeys: string[], + source: CreateHistoryVersion['schema'] + ) => { + return diffKeys.reduce((previousAttributesObject, diffKey) => { + previousAttributesObject[diffKey] = source[diffKey]; + + return previousAttributesObject; + }, {}); + }; + + const versionSchemaKeys = Object.keys(versionSchemaAttributes); + const contentTypeSchemaAttributesKeys = Object.keys(sanitizedContentTypeSchemaAttributes); + // The attribute is new if it's on the content type schema but not on the version schema + const uniqueToContentType = difference(contentTypeSchemaAttributesKeys, versionSchemaKeys); + const added = reduceDifferenceToAttributesObject( + uniqueToContentType, + sanitizedContentTypeSchemaAttributes + ); + // The attribute was removed or renamed if it's on the version schema but not on the content type schema + const uniqueToVersion = difference(versionSchemaKeys, contentTypeSchemaAttributesKeys); + const removed = reduceDifferenceToAttributesObject(uniqueToVersion, versionSchemaAttributes); + + return { added, removed }; +}; diff --git a/packages/core/content-manager/shared/contracts/history-versions.ts b/packages/core/content-manager/shared/contracts/history-versions.ts index 7091eae176..f922506fae 100644 --- a/packages/core/content-manager/shared/contracts/history-versions.ts +++ b/packages/core/content-manager/shared/contracts/history-versions.ts @@ -31,6 +31,12 @@ export interface HistoryVersionDataResponse extends Omit; + removed: Record; + }; + }; } // Export to prevent the TS "cannot be named" error in the history service