From 23912e523215037b1dc8efd427b172ea185aefb2 Mon Sep 17 00:00:00 2001 From: Ian Pirro Date: Thu, 21 Jul 2022 00:29:12 -0700 Subject: [PATCH] Fix: Error applying schemas with nested collection(s) (#13949) * Fix nested collection creation when parent exists * tests * desc | comments * typo * minor tweak Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com> --- api/src/utils/apply-snapshot.ts | 38 ++- api/tests/__test-utils__/schemas.ts | 30 ++ api/tests/__test-utils__/snapshots.ts | 415 +++++++++++++++++++++++++ api/tests/utils/apply-snapshot.test.ts | 270 ++++++++++++++++ 4 files changed, 747 insertions(+), 6 deletions(-) create mode 100644 api/tests/__test-utils__/snapshots.ts create mode 100644 api/tests/utils/apply-snapshot.test.ts diff --git a/api/src/utils/apply-snapshot.ts b/api/src/utils/apply-snapshot.ts index 9220052974..4c92dcb353 100644 --- a/api/src/utils/apply-snapshot.ts +++ b/api/src/utils/apply-snapshot.ts @@ -96,12 +96,38 @@ export async function applySnapshot( } }; - // create top level collections (no group) first, then continue with nested collections recursively - await createCollections( - snapshotDiff.collections.filter( - ({ diff }) => diff[0].kind === 'N' && (diff[0] as DiffNew).rhs.meta?.group === null - ) - ); + // Finds all collections that need to be created + const filterCollectionsForCreation = ({ diff }: { collection: string; diff: Diff[] }) => { + // Check new collections only + const isNewCollection = diff[0].kind === 'N'; + if (!isNewCollection) return false; + + // Create now if no group + const groupName = (diff[0] as DiffNew).rhs.meta?.group; + if (!groupName) return true; + + // Check if parent collection already exists in schema + const parentExists = current.collections.find((c) => c.collection === groupName) !== undefined; + // If this is a new collection and the parent collection doesn't exist in current schema -> + // Check if the parent collection will be created as part of applying this snapshot -> + // If yes -> this collection will be created recursively + // If not -> create now + // (ex.) + // TopLevelCollection - I exist in current schema + // NestedCollection - I exist in snapshotDiff as a new collection + // TheCurrentCollectionInIteration - I exist in snapshotDiff as a new collection but will be created as part of NestedCollection + const parentWillBeCreatedInThisApply = + snapshotDiff.collections.filter(({ collection, diff }) => diff[0].kind === 'N' && collection === groupName) + .length > 0; + // Has group, but parent is not new, parent is also not being created in this snapshot apply + if (parentExists && !parentWillBeCreatedInThisApply) return true; + + return false; + }; + + // Create top level collections (no group, or highest level in existing group) first, + // then continue with nested collections recursively + await createCollections(snapshotDiff.collections.filter(filterCollectionsForCreation)); // delete top level collections (no group) first, then continue with nested collections recursively await deleteCollections( diff --git a/api/tests/__test-utils__/schemas.ts b/api/tests/__test-utils__/schemas.ts index ab9709e899..591c8bf0bb 100644 --- a/api/tests/__test-utils__/schemas.ts +++ b/api/tests/__test-utils__/schemas.ts @@ -273,3 +273,33 @@ export const userSchema = { }, ] as Relation[], }; + +export const snapshotApplyTestSchema = { + collections: { + test_table: { + collection: 'test_table', + primary: 'id', + singleton: false, + note: 'test_table', + sortField: null, + accountability: 'all', + fields: { + id: { + field: 'id', + defaultValue: null, + nullable: false, + generated: false, + type: 'uuid', + dbType: 'uuid', + precision: null, + scale: null, + special: [], + note: null, + alias: false, + validation: null, + }, + }, + }, + } as CollectionsOverview, + relations: [] as Relation[], +}; diff --git a/api/tests/__test-utils__/snapshots.ts b/api/tests/__test-utils__/snapshots.ts new file mode 100644 index 0000000000..f0e3963649 --- /dev/null +++ b/api/tests/__test-utils__/snapshots.ts @@ -0,0 +1,415 @@ +import { Snapshot, SnapshotField } from '../../src/types'; + +export const testSnapshotBefore: Snapshot = { + version: 1, + directus: '0.0.0', + collections: [ + { + collection: 'test_table', + meta: { + accountability: 'all', + collection: 'test_table', + group: null, + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { + comment: null, + name: 'test_table', + schema: 'public', + }, + }, + ], + fields: [ + { + collection: 'test_table', + field: 'id', + meta: { + collection: 'test_table', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table', + }, + type: 'uuid', + } as SnapshotField, + ], + relations: [], +}; + +export const testSnapshotToApply: Snapshot = { + version: 1, + directus: '0.0.0', + collections: [ + { + collection: 'test_table', + meta: { + accountability: 'all', + collection: 'test_table', + group: null, + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { + comment: null, + name: 'test_table', + schema: 'public', + }, + }, + { + collection: 'test_table_2', + meta: { + accountability: 'all', + collection: 'test_table_2', + group: 'test_table', + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { + comment: null, + name: 'test_table_2', + schema: 'public', + }, + }, + { + collection: 'test_table_3', + meta: { + accountability: 'all', + collection: 'test_table_3', + group: 'test_table_2', + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { + comment: null, + name: 'test_table_3', + schema: 'public', + }, + }, + ], + fields: [ + { + collection: 'test_table', + field: 'id', + meta: { + collection: 'test_table', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table', + }, + type: 'uuid', + } as SnapshotField, + { + collection: 'test_table_2', + field: 'id', + meta: { + collection: 'test_table_2', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table_2', + }, + type: 'uuid', + } as SnapshotField, + { + collection: 'test_table_3', + field: 'id', + meta: { + collection: 'test_table_3', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table_3', + }, + type: 'uuid', + } as SnapshotField, + ], + relations: [], +}; + +export const testSnapshotToApplyNotNested: Snapshot = { + version: 1, + directus: '0.0.0', + collections: [ + { + collection: 'test_table', + meta: { + accountability: 'all', + collection: 'test_table', + group: null, + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { + comment: null, + name: 'test_table', + schema: 'public', + }, + }, + { + collection: 'test_table_2', + meta: { + accountability: 'all', + collection: 'test_table_2', + group: null, + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { + comment: null, + name: 'test_table_2', + schema: 'public', + }, + }, + ], + fields: [ + { + collection: 'test_table', + field: 'id', + meta: { + collection: 'test_table', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table', + }, + type: 'uuid', + } as SnapshotField, + { + collection: 'test_table_2', + field: 'id', + meta: { + collection: 'test_table_2', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table_2', + }, + type: 'uuid', + } as SnapshotField, + ], + relations: [], +}; diff --git a/api/tests/utils/apply-snapshot.test.ts b/api/tests/utils/apply-snapshot.test.ts new file mode 100644 index 0000000000..e1f2f8742d --- /dev/null +++ b/api/tests/utils/apply-snapshot.test.ts @@ -0,0 +1,270 @@ +import knex, { Knex } from 'knex'; +import { getTracker, MockClient, Tracker } from 'knex-mock-client'; +import { snapshotApplyTestSchema } from '../__test-utils__/schemas'; + +import { applySnapshot } from '../../src/utils/apply-snapshot'; +import { testSnapshotToApply, testSnapshotBefore, testSnapshotToApplyNotNested } from '../__test-utils__/snapshots'; +import { CollectionsService, FieldsService } from '../../src/services'; +import * as getSchema from '../../src/utils/get-schema'; + +jest.mock('../../src/database/index', () => { + return { + getDatabaseClient: jest.fn().mockReturnValue('postgres'), + }; +}); +jest.requireMock('../../src/database/index'); + +class Client_PG extends MockClient {} + +describe('applySnapshot', () => { + let db: jest.Mocked; + let tracker: Tracker; + + beforeEach(() => { + db = knex({ client: Client_PG }) as jest.Mocked; + tracker = getTracker(); + }); + + afterEach(() => { + tracker.reset(); + jest.clearAllMocks(); + }); + + describe('Creating new collection(s)', () => { + it('Creates new top-level collection(s)', async () => { + const expected = { + collection: 'test_table_2', + meta: { + accountability: 'all', + collection: 'test_table_2', + group: null, + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { comment: null, name: 'test_table_2', schema: 'public' }, + fields: [ + { + collection: 'test_table_2', + field: 'id', + meta: { + collection: 'test_table_2', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table_2', + }, + type: 'uuid', + }, + ], + }; + + // Stop call to db later on in apply-snapshot + jest.spyOn(getSchema, 'getSchema').mockReturnValue(Promise.resolve(snapshotApplyTestSchema)); + // We are not actually testing that createOne works, just that is is called correctly + const createOneCollectionSpy = jest + .spyOn(CollectionsService.prototype, 'createOne') + .mockImplementation(jest.fn()); + const createFieldSpy = jest.spyOn(FieldsService.prototype, 'createField').mockImplementation(jest.fn()); + + await applySnapshot(testSnapshotToApplyNotNested, { + database: db, + current: testSnapshotBefore, + schema: snapshotApplyTestSchema, + }); + + expect(createOneCollectionSpy).toHaveBeenCalledTimes(1); + expect(createOneCollectionSpy).toHaveBeenCalledWith(expected); + + // There should be no fields left to create + // they will get filtered in createCollections + expect(createFieldSpy).toHaveBeenCalledTimes(0); + }); + + it('Creates the highest-level nested collection(s) with existing parents and any children', async () => { + const expected = { + collection: 'test_table_2', + meta: { + accountability: 'all', + collection: 'test_table_2', + group: 'test_table', + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { comment: null, name: 'test_table_2', schema: 'public' }, + fields: [ + { + collection: 'test_table_2', + field: 'id', + meta: { + collection: 'test_table_2', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table_2', + }, + type: 'uuid', + }, + ], + }; + + const expected2 = { + collection: 'test_table_3', + fields: [ + { + collection: 'test_table_3', + field: 'id', + meta: { + collection: 'test_table_3', + conditions: null, + display: null, + display_options: null, + field: 'id', + group: null, + hidden: true, + interface: null, + note: null, + options: null, + readonly: false, + required: false, + sort: null, + special: null, + translations: {}, + validation: null, + validation_message: null, + width: 'full', + }, + schema: { + comment: null, + data_type: 'uuid', + default_value: null, + foreign_key_column: null, + foreign_key_schema: null, + foreign_key_table: null, + generation_expression: null, + has_auto_increment: false, + is_generated: false, + is_nullable: false, + is_primary_key: true, + is_unique: true, + max_length: null, + name: 'id', + numeric_precision: null, + numeric_scale: null, + schema: 'public', + table: 'test_table_3', + }, + type: 'uuid', + }, + ], + meta: { + accountability: 'all', + collection: 'test_table_3', + group: 'test_table_2', + hidden: true, + icon: 'import_export', + item_duplication_fields: null, + note: null, + singleton: false, + translations: {}, + }, + schema: { comment: null, name: 'test_table_3', schema: 'public' }, + }; + + // Stop call to db later on in apply-snapshot + jest.spyOn(getSchema, 'getSchema').mockReturnValue(Promise.resolve(snapshotApplyTestSchema)); + // We are not actually testing that createOne works, just that is is called correctly + const createOneCollectionSpy = jest + .spyOn(CollectionsService.prototype, 'createOne') + .mockImplementation(jest.fn()); + const createFieldSpy = jest.spyOn(FieldsService.prototype, 'createField').mockImplementation(jest.fn()); + + await applySnapshot(testSnapshotToApply, { + database: db, + current: testSnapshotBefore, + schema: snapshotApplyTestSchema, + }); + + expect(createOneCollectionSpy).toHaveBeenCalledTimes(2); + expect(createOneCollectionSpy).toHaveBeenCalledWith(expected); + expect(createOneCollectionSpy).toHaveBeenCalledWith(expected2); + + // There should be no fields left to create + // they will get filtered in createCollections + expect(createFieldSpy).toHaveBeenCalledTimes(0); + }); + }); +});