From dd3202ce38454d546741a2ec18997d438bbdd2f5 Mon Sep 17 00:00:00 2001 From: Azri Kahar <42867097+azrikahar@users.noreply.github.com> Date: Mon, 10 Apr 2023 23:53:51 +0800 Subject: [PATCH] Fix diff validation and apply (#18048) Co-authored-by: Rijk van Zanten Co-authored-by: ian --- api/src/services/fields.ts | 3 +- api/src/utils/apply-diff.ts | 8 + api/src/utils/sanitize-schema.ts | 21 ++ api/src/utils/validate-diff.test.ts | 162 +++++++++++++ api/src/utils/validate-diff.ts | 14 +- packages/schema/src/dialects/cockroachdb.ts | 2 + tests-blackbox/routes/schema/schema.test.ts | 244 ++++++++++++++++++++ 7 files changed, 448 insertions(+), 6 deletions(-) diff --git a/api/src/services/fields.ts b/api/src/services/fields.ts index f7425f1d10..4719156b61 100644 --- a/api/src/services/fields.ts +++ b/api/src/services/fields.ts @@ -21,6 +21,7 @@ import type { AbstractServiceOptions, ActionEventParams, MutationOptions } from import getDefaultValue from '../utils/get-default-value.js'; import getLocalType from '../utils/get-local-type.js'; import { getSchema } from '../utils/get-schema.js'; +import { sanitizeColumn } from '../utils/sanitize-schema.js'; import { RelationsService } from './relations.js'; export class FieldsService { @@ -395,7 +396,7 @@ export class FieldsService { if (hookAdjustedField.schema) { const existingColumn = await this.schemaInspector.columnInfo(collection, hookAdjustedField.field); - if (!isEqual(existingColumn, hookAdjustedField.schema)) { + if (!isEqual(sanitizeColumn(existingColumn), hookAdjustedField.schema)) { try { await this.knex.schema.alterTable(collection, (table) => { if (!hookAdjustedField.schema) return; diff --git a/api/src/utils/apply-diff.ts b/api/src/utils/apply-diff.ts index 6e2633334d..f1b78f1501 100644 --- a/api/src/utils/apply-diff.ts +++ b/api/src/utils/apply-diff.ts @@ -20,6 +20,7 @@ import { SnapshotField, } from '../types/index.js'; import { getSchema } from './get-schema.js'; +import { getHelpers } from '../database/helpers/index.js'; type CollectionDelta = { collection: string; @@ -32,6 +33,7 @@ export async function applyDiff( options?: { database?: Knex; schema?: SchemaOverview } ): Promise { const database = options?.database ?? getDatabase(); + const helpers = getHelpers(database); const schema = options?.schema ?? (await getSchema({ database, bypassCache: true })); const nestedActionEvents: ActionEventParams[] = []; @@ -41,6 +43,8 @@ export async function applyDiff( bypassLimits: true, }; + const runPostColumnChange = await helpers.schema.preColumnChange(); + await database.transaction(async (trx) => { const collectionsService = new CollectionsService({ knex: trx, schema }); @@ -301,6 +305,10 @@ export async function applyDiff( } }); + if (runPostColumnChange) { + await helpers.schema.postColumnChange(); + } + await clearSystemCache(); if (nestedActionEvents.length > 0) { diff --git a/api/src/utils/sanitize-schema.ts b/api/src/utils/sanitize-schema.ts index a4743865e3..707fd1d735 100644 --- a/api/src/utils/sanitize-schema.ts +++ b/api/src/utils/sanitize-schema.ts @@ -1,3 +1,4 @@ +import type { Column } from '@directus/schema'; import type { Field, Relation } from '@directus/types'; import { pick } from 'lodash-es'; import type { Collection } from '../types/index.js'; @@ -50,6 +51,26 @@ export function sanitizeField(field: Field | undefined, sanitizeAllSchema = fals return pick(field, pickedPaths); } +export function sanitizeColumn(column: Column) { + return pick(column, [ + 'name', + 'table', + 'data_type', + 'default_value', + 'max_length', + 'numeric_precision', + 'numeric_scale', + 'is_nullable', + 'is_unique', + 'is_primary_key', + 'is_generated', + 'generation_expression', + 'has_auto_increment', + 'foreign_key_table', + 'foreign_key_column', + ]); +} + /** * Pick certain database vendor specific relation properties that should be compared when performing diff * diff --git a/api/src/utils/validate-diff.test.ts b/api/src/utils/validate-diff.test.ts index 5d59f63a91..4e82dd8b54 100644 --- a/api/src/utils/validate-diff.test.ts +++ b/api/src/utils/validate-diff.test.ts @@ -131,6 +131,168 @@ describe('should throw accurate error', () => { }); }); +test('should not throw error for diffs with varying types of lhs/rhs', () => { + const diff: any = { + hash: 'abc', + diff: { + collections: [ + { + collection: 'a', + diff: [ + { + kind: 'E', + path: ['meta', 'color'], + lhs: null, + rhs: '#6644FF', + }, + ], + }, + { + collection: 'a', + diff: [ + { + kind: 'A', + path: ['meta', 'translations'], + index: 1, + item: { + kind: 'N', + rhs: { + language: 'de-DE', + translation: 'Collection A de-DE', + }, + }, + }, + ], + }, + { + collection: 'b', + diff: [ + { + kind: 'E', + path: ['meta', 'translations', 1, 'language'], + lhs: 'es-ES', + rhs: 'nl-NL', + }, + { + kind: 'E', + path: ['meta', 'translations', 1, 'translation'], + lhs: 'nombre', + rhs: 'naam', + }, + ], + }, + ], + fields: [ + { + collection: 'a', + field: 'new_field', + diff: [ + { + kind: 'N', + rhs: { + collection: 'a', + field: 'new_field', + type: 'string', + meta: {}, + schema: {}, + }, + }, + ], + }, + { + collection: 'a', + field: 'update_field', + diff: [ + { + kind: 'E', + path: ['meta', 'options'], + lhs: { + iconLeft: 'check_circle', + }, + rhs: null, + }, + ], + }, + { + collection: 'a', + field: 'delete_field', + diff: [ + { + kind: 'D', + lhs: { + collection: 'a', + field: 'delete_field', + type: 'string', + meta: {}, + schema: {}, + }, + }, + ], + }, + ], + relations: [ + { + collection: 'a', + field: 'm2o', + related_collection: 'b', + diff: [ + { + kind: 'E', + path: ['schema', 'on_delete'], + lhs: 'SET NULL', + rhs: 'CASCADE', + }, + ], + }, + ], + }, + }; + const snapshot = { hash: 'abc' } as SnapshotWithHash; + + expect(() => validateApplyDiff(diff, snapshot)).not.toThrow(); +}); + +test('should not throw error for relation diff with null related_collection (applicable for M2A junction tables)', () => { + const diff: any = { + hash: 'abc', + diff: { + collections: [], + fields: [], + relations: [ + { + collection: 'pages_blocks', + field: 'item', + related_collection: null, + diff: [ + { + kind: 'N', + rhs: { + collection: 'pages_blocks', + field: 'item', + related_collection: null, + meta: { + junction_field: 'pages_id', + many_collection: 'pages_blocks', + many_field: 'item', + one_allowed_collections: ['a', 'b'], + one_collection: null, + one_collection_field: 'collection', + one_deselect_action: 'nullify', + one_field: null, + sort_field: null, + }, + }, + }, + ], + }, + ], + }, + }; + const snapshot = { hash: 'abc' } as SnapshotWithHash; + + expect(() => validateApplyDiff(diff, snapshot)).not.toThrow(); +}); + test('should detect empty diff', () => { const diff = { hash: 'abc', diff --git a/api/src/utils/validate-diff.ts b/api/src/utils/validate-diff.ts index ac21226280..d38dd68aba 100644 --- a/api/src/utils/validate-diff.ts +++ b/api/src/utils/validate-diff.ts @@ -6,12 +6,16 @@ const deepDiffSchema = Joi.object({ kind: Joi.string() .valid(...Object.values(DiffKind)) .required(), - path: Joi.array().items(Joi.string()), - lhs: Joi.object().when('kind', { is: [DiffKind.DELETE, DiffKind.EDIT], then: Joi.required() }), - rhs: Joi.object().when('kind', { is: [DiffKind.NEW, DiffKind.EDIT], then: Joi.required() }), + path: Joi.array().items(Joi.alternatives().try(Joi.string(), Joi.number())), + lhs: Joi.any().when('kind', { is: [DiffKind.NEW, DiffKind.ARRAY], then: Joi.optional(), otherwise: Joi.required() }), + rhs: Joi.any().when('kind', { + is: [DiffKind.DELETE, DiffKind.ARRAY], + then: Joi.optional(), + otherwise: Joi.required(), + }), index: Joi.number().when('kind', { is: DiffKind.ARRAY, then: Joi.required() }), - item: Joi.link('/').when('kind', { is: DiffKind.ARRAY, then: Joi.required() }), -}); + item: Joi.link('#deepdiff').when('kind', { is: DiffKind.ARRAY, then: Joi.required() }), +}).id('deepdiff'); const applyJoiSchema = Joi.object({ hash: Joi.string().required(), diff --git a/packages/schema/src/dialects/cockroachdb.ts b/packages/schema/src/dialects/cockroachdb.ts index 58ef6b4ba2..73cbc7e741 100644 --- a/packages/schema/src/dialects/cockroachdb.ts +++ b/packages/schema/src/dialects/cockroachdb.ts @@ -47,6 +47,8 @@ export function parseDefaultValue(value: string | null) { value = value.split('::')[0] ?? null; + if (value?.trim().toLowerCase() === 'null') return null; + return stripQuotes(value); } diff --git a/tests-blackbox/routes/schema/schema.test.ts b/tests-blackbox/routes/schema/schema.test.ts index d8a4c30427..fa59ef8d55 100644 --- a/tests-blackbox/routes/schema/schema.test.ts +++ b/tests-blackbox/routes/schema/schema.test.ts @@ -25,6 +25,7 @@ import { cloneDeep } from 'lodash'; import { PrimaryKeyType, PRIMARY_KEY_TYPES } from '@common/index'; import { load as loadYaml } from 'js-yaml'; import { version as currentDirectusVersion } from '../../../api/package.json'; +import { v4 as uuid } from 'uuid'; describe('Schema Snapshots', () => { const snapshotsCacheOriginal: { @@ -399,6 +400,249 @@ describe('Schema Snapshots', () => { 300000 ); }); + + describe('applies lhs that is not an object', () => { + it.each(vendors)( + '%s', + async (vendor) => { + expect(snapshotsCacheOriginal[vendor]).toBeDefined(); + + // Setup + for (const pkType of PRIMARY_KEY_TYPES) { + await request(getUrl(vendor)) + .patch(`/collections/${collectionAll}_${pkType}`) + .send({ meta: { icon: 'abc', color: '#E35169' } }) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + } + + // Action + const responseDiff = await request(getUrl(vendor)) + .post('/schema/diff') + .send(snapshotsCacheOriginal[vendor]) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + const response = await request(getUrl(vendor)) + .post('/schema/apply') + .send(responseDiff.body.data) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + // Assert + expect(response.statusCode).toEqual(204); + }, + 300000 + ); + }); + + describe('applies Array type diffs', () => { + it.each(vendors)( + '%s', + async (vendor) => { + // Setup + for (const pkType of PRIMARY_KEY_TYPES) { + const nameField = ( + await request(getUrl(vendor)) + .get(`/fields/${collectionAll}_${pkType}/name`) + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`) + ).body.data; + + nameField.meta.translations = [ + { language: 'en-US', translation: `${pkType} name` }, + { language: 'nl-NL', translation: `${pkType} naam` }, + ]; + + await request(getUrl(vendor)) + .patch(`/fields/${collectionAll}_${pkType}/name`) + .send(nameField) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + } + + const newSnapshot = ( + await request(getUrl(vendor)) + .get('/schema/snapshot') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`) + ).body.data; + + for (const pkType of PRIMARY_KEY_TYPES) { + const nameField = ( + await request(getUrl(vendor)) + .get(`/fields/${collectionAll}_${pkType}/name`) + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`) + ).body.data; + + nameField.meta.translations = [ + { language: 'en-US', translation: `${pkType} name` }, + { language: 'es-ES', translation: `${pkType} nombre` }, + { language: 'nl-NL', translation: `${pkType} naam` }, + ]; + + await request(getUrl(vendor)) + .patch(`/fields/${collectionAll}_${pkType}/name`) + .send(nameField) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + } + + // Action + const responseDiff = await request(getUrl(vendor)) + .post('/schema/diff') + .send(newSnapshot) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + const response = await request(getUrl(vendor)) + .post('/schema/apply') + .send(responseDiff.body.data) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + // Assert + expect(response.statusCode).toEqual(204); + }, + 300000 + ); + }); + + describe('applies with field meta changes', () => { + it.each(vendors)( + '%s', + async (vendor) => { + // Setup + for (const pkType of PRIMARY_KEY_TYPES) { + const fields = ( + await request(getUrl(vendor)) + .get(`/fields/${collectionAll}_${pkType}`) + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`) + ).body.data.map((field: any) => { + return field.field; + }); + + fields.sort(); + + const payload = fields.map((field: string, index: number) => { + return { field, meta: { sort: index + 1 } }; + }); + + await request(getUrl(vendor)) + .patch(`/fields/${collectionAll}_${pkType}`) + .send(payload) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + } + + const newSnapshot = ( + await request(getUrl(vendor)) + .get('/schema/snapshot') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`) + ).body.data; + + for (const pkType of PRIMARY_KEY_TYPES) { + const fields = ( + await request(getUrl(vendor)) + .get(`/fields/${collectionAll}_${pkType}`) + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`) + ).body.data.map((field: any) => { + return field.field; + }); + + fields.sort().reverse(); + + const payload = fields.map((field: string, index: number) => { + return { field, meta: { sort: index + 1 } }; + }); + + await request(getUrl(vendor)) + .patch(`/fields/${collectionAll}_${pkType}`) + .send(payload) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + } + + // Action + const responseDiff = await request(getUrl(vendor)) + .post('/schema/diff') + .send(newSnapshot) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + const response = await request(getUrl(vendor)) + .post('/schema/apply') + .send(responseDiff.body.data) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + // Assert + expect(response.statusCode).toEqual(204); + }, + 300000 + ); + }); + + describe('confirm deletion of relational field does not nullify existing relational fields', () => { + it.each(vendors)( + '%s', + async (vendor) => { + // TODO: Fix cockroachdb requiring schema changes to be applied first when in a transaction + if (vendor === 'cockroachdb') { + expect(true).toBe(true); + return; + } + + expect(snapshotsCacheOriginal[vendor]).toBeDefined(); + + // Setup + const childrenIDs: Record = {}; + const tempRelationalField = 'temp_relational'; + for (const pkType of PRIMARY_KEY_TYPES) { + const item = await common.CreateItem(vendor, { + collection: `${collectionAll}_${pkType}`, + item: { + id: pkType === 'string' ? uuid() : undefined, + all_id: { id: pkType === 'string' ? uuid() : undefined }, + o2m: [{ id: pkType === 'string' ? uuid() : undefined }], + }, + }); + childrenIDs[pkType] = { id: item.id, m2o_id: item.all_id, o2m_id: item.o2m[0] }; + await common.CreateFieldM2O(vendor, { + collection: `${collectionAll}_${pkType}`, + field: tempRelationalField, + otherCollection: collectionSelf, + }); + } + + // Action + const responseDiff = await request(getUrl(vendor)) + .post('/schema/diff') + .send(snapshotsCacheOriginal[vendor]) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + const response = await request(getUrl(vendor)) + .post('/schema/apply') + .send(responseDiff.body.data) + .set('Content-type', 'application/json') + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`); + + // Assert + expect(response.statusCode).toEqual(204); + for (const pkType of PRIMARY_KEY_TYPES) { + const item = ( + await request(getUrl(vendor)) + .get(`/items/${collectionAll}_${pkType}/${childrenIDs[pkType].id}`) + .set('Authorization', `Bearer ${common.USER.ADMIN.TOKEN}`) + ).body.data; + + expect(item.all_id).toBe(childrenIDs[pkType].m2o_id); + expect(item.o2m).toHaveLength(1); + expect(item.o2m[0]).toBe(childrenIDs[pkType].o2m_id); + } + }, + 300000 + ); + }); }); common.ClearCaches();