Fix diff validation and apply (#18048)

Co-authored-by: Rijk van Zanten <rijkvanzanten@me.com>
Co-authored-by: ian <licitdev@gmail.com>

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;

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<void> {
 	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) {

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
  *

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

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(),

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

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<string, { id: any; m2o_id: any; o2m_id: any }> = {};
+					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();