From 8a3dc4b68b36217b97110e2e3dabaaf20be59648 Mon Sep 17 00:00:00 2001 From: ian Date: Wed, 23 Nov 2022 03:57:17 +0800 Subject: [PATCH] Propagate mutation options for schema apply (#16562) * Propagate mutation options for schema apply * Fix unit test * Add bypassCache flag and remove flushCaches calls * remove accountability option from getSchema Co-authored-by: Azri Kahar <42867097+azrikahar@users.noreply.github.com> --- api/src/cli/commands/schema/apply.ts | 3 -- api/src/cli/commands/schema/snapshot.ts | 3 -- api/src/middleware/schema.ts | 2 +- api/src/services/collections.ts | 14 +++--- api/src/services/fields.ts | 59 +++++++++++++++------- api/src/services/relations.ts | 48 ++++++++++++------ api/src/utils/apply-snapshot.test.ts | 11 ++-- api/src/utils/apply-snapshot.ts | 67 ++++++++++++++++++------- api/src/utils/get-schema.ts | 11 ++-- api/src/utils/get-snapshot.ts | 2 +- 10 files changed, 145 insertions(+), 75 deletions(-) diff --git a/api/src/cli/commands/schema/apply.ts b/api/src/cli/commands/schema/apply.ts index d8169d78fe..e5e281dde5 100644 --- a/api/src/cli/commands/schema/apply.ts +++ b/api/src/cli/commands/schema/apply.ts @@ -4,7 +4,6 @@ import { promises as fs } from 'fs'; import inquirer from 'inquirer'; import { load as loadYaml } from 'js-yaml'; import path from 'path'; -import { flushCaches } from '../../../cache'; import getDatabase, { isInstalled, validateDatabaseConnection } from '../../../database'; import logger from '../../../logger'; import { Snapshot } from '../../../types'; @@ -19,8 +18,6 @@ export async function apply(snapshotPath: string, options?: { yes: boolean; dryR await validateDatabaseConnection(database); - await flushCaches(); - if ((await isInstalled()) === false) { logger.error(`Directus isn't installed on this database. Please run "directus bootstrap" first.`); database.destroy(); diff --git a/api/src/cli/commands/schema/snapshot.ts b/api/src/cli/commands/schema/snapshot.ts index dfe5fd9d95..14899681a3 100644 --- a/api/src/cli/commands/schema/snapshot.ts +++ b/api/src/cli/commands/schema/snapshot.ts @@ -5,14 +5,11 @@ import { constants as fsConstants, promises as fs } from 'fs'; import path from 'path'; import inquirer from 'inquirer'; import { dump as toYaml } from 'js-yaml'; -import { flushCaches } from '../../../cache'; export async function snapshot( snapshotPath?: string, options?: { yes: boolean; format: 'json' | 'yaml' } ): Promise { - await flushCaches(); - const database = getDatabase(); try { diff --git a/api/src/middleware/schema.ts b/api/src/middleware/schema.ts index b4aef7d72e..ee272a4655 100644 --- a/api/src/middleware/schema.ts +++ b/api/src/middleware/schema.ts @@ -3,7 +3,7 @@ import asyncHandler from '../utils/async-handler'; import { getSchema } from '../utils/get-schema'; const schema: RequestHandler = asyncHandler(async (req, res, next) => { - req.schema = await getSchema({ accountability: req.accountability }); + req.schema = await getSchema(); return next(); }); diff --git a/api/src/services/collections.ts b/api/src/services/collections.ts index 366696890e..dfdae1418e 100644 --- a/api/src/services/collections.ts +++ b/api/src/services/collections.ts @@ -173,7 +173,7 @@ export class CollectionsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; @@ -222,7 +222,7 @@ export class CollectionsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; @@ -405,7 +405,7 @@ export class CollectionsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; @@ -461,7 +461,7 @@ export class CollectionsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; @@ -511,7 +511,7 @@ export class CollectionsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; @@ -640,7 +640,7 @@ export class CollectionsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; @@ -688,7 +688,7 @@ export class CollectionsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; diff --git a/api/src/services/fields.ts b/api/src/services/fields.ts index 5fdb162e6a..dbd5552d8e 100644 --- a/api/src/services/fields.ts +++ b/api/src/services/fields.ts @@ -242,7 +242,8 @@ export class FieldsService { async createField( collection: string, field: Partial & { field: string; type: Type | null }, - table?: Knex.CreateTableBuilder // allows collection creation to + table?: Knex.CreateTableBuilder, // allows collection creation to + opts?: MutationOptions ): Promise { if (this.accountability && this.accountability.admin !== true) { throw new ForbiddenException(); @@ -310,7 +311,7 @@ export class FieldsService { ); } - nestedActionEvents.push({ + const actionEvent = { event: 'fields.create', meta: { payload: hookAdjustedField, @@ -322,29 +323,39 @@ export class FieldsService { schema: this.schema, accountability: this.accountability, }, - }); + }; + + if (opts?.bypassEmitAction) { + opts.bypassEmitAction(actionEvent); + } else { + nestedActionEvents.push(actionEvent); + } }); } finally { if (runPostColumnChange) { await this.helpers.schema.postColumnChange(); } - if (this.cache && env.CACHE_AUTO_PURGE) { + if (this.cache && env.CACHE_AUTO_PURGE && opts?.autoPurgeCache !== false) { await this.cache.clear(); } - await clearSystemCache(); + if (opts?.autoPurgeSystemCache !== false) { + await clearSystemCache(); + } - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { + const updatedSchema = await getSchema(); - for (const nestedActionEvent of nestedActionEvents) { - nestedActionEvent.context.schema = updatedSchema; - emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + for (const nestedActionEvent of nestedActionEvents) { + nestedActionEvent.context.schema = updatedSchema; + emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + } } } } - async updateField(collection: string, field: RawField): Promise { + async updateField(collection: string, field: RawField, opts?: MutationOptions): Promise { if (this.accountability && this.accountability.admin !== true) { throw new ForbiddenException(); } @@ -418,7 +429,7 @@ export class FieldsService { } } - nestedActionEvents.push({ + const actionEvent = { event: 'fields.update', meta: { payload: hookAdjustedField, @@ -430,7 +441,13 @@ export class FieldsService { schema: this.schema, accountability: this.accountability, }, - }); + }; + + if (opts?.bypassEmitAction) { + opts.bypassEmitAction(actionEvent); + } else { + nestedActionEvents.push(actionEvent); + } return field.field; } finally { @@ -438,17 +455,21 @@ export class FieldsService { await this.helpers.schema.postColumnChange(); } - if (this.cache && env.CACHE_AUTO_PURGE) { + if (this.cache && env.CACHE_AUTO_PURGE && opts?.autoPurgeCache !== false) { await this.cache.clear(); } - await clearSystemCache(); + if (opts?.autoPurgeSystemCache !== false) { + await clearSystemCache(); + } - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { + const updatedSchema = await getSchema(); - for (const nestedActionEvent of nestedActionEvents) { - nestedActionEvent.context.schema = updatedSchema; - emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + for (const nestedActionEvent of nestedActionEvents) { + nestedActionEvent.context.schema = updatedSchema; + emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + } } } } @@ -609,7 +630,7 @@ export class FieldsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; diff --git a/api/src/services/relations.ts b/api/src/services/relations.ts index 0fdde09ecb..dca529be2e 100644 --- a/api/src/services/relations.ts +++ b/api/src/services/relations.ts @@ -125,7 +125,7 @@ export class RelationsService { /** * Create a new relationship / foreign key constraint */ - async createOne(relation: Partial): Promise { + async createOne(relation: Partial, opts?: MutationOptions): Promise { if (this.accountability && this.accountability.admin !== true) { throw new ForbiddenException(); } @@ -208,7 +208,8 @@ export class RelationsService { }); await relationsItemService.createOne(metaRow, { - bypassEmitAction: (params) => nestedActionEvents.push(params), + bypassEmitAction: (params) => + opts?.bypassEmitAction ? opts.bypassEmitAction(params) : nestedActionEvents.push(params), }); }); } finally { @@ -216,13 +217,17 @@ export class RelationsService { await this.helpers.schema.postColumnChange(); } - await clearSystemCache(); + if (opts?.autoPurgeSystemCache !== false) { + await clearSystemCache(); + } - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { + const updatedSchema = await getSchema(); - for (const nestedActionEvent of nestedActionEvents) { - nestedActionEvent.context.schema = updatedSchema; - emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + for (const nestedActionEvent of nestedActionEvents) { + nestedActionEvent.context.schema = updatedSchema; + emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + } } } } @@ -232,7 +237,12 @@ export class RelationsService { * * Note: You can update anything under meta, but only the `on_delete` trigger under schema */ - async updateOne(collection: string, field: string, relation: Partial): Promise { + async updateOne( + collection: string, + field: string, + relation: Partial, + opts?: MutationOptions + ): Promise { if (this.accountability && this.accountability.admin !== true) { throw new ForbiddenException(); } @@ -298,7 +308,8 @@ export class RelationsService { if (relation.meta) { if (existingRelation?.meta) { await relationsItemService.updateOne(existingRelation.meta.id, relation.meta, { - bypassEmitAction: (params) => nestedActionEvents.push(params), + bypassEmitAction: (params) => + opts?.bypassEmitAction ? opts.bypassEmitAction(params) : nestedActionEvents.push(params), }); } else { await relationsItemService.createOne( @@ -309,7 +320,8 @@ export class RelationsService { one_collection: existingRelation.related_collection || null, }, { - bypassEmitAction: (params) => nestedActionEvents.push(params), + bypassEmitAction: (params) => + opts?.bypassEmitAction ? opts.bypassEmitAction(params) : nestedActionEvents.push(params), } ); } @@ -320,13 +332,17 @@ export class RelationsService { await this.helpers.schema.postColumnChange(); } - await clearSystemCache(); + if (opts?.autoPurgeSystemCache !== false) { + await clearSystemCache(); + } - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { + const updatedSchema = await getSchema(); - for (const nestedActionEvent of nestedActionEvents) { - nestedActionEvent.context.schema = updatedSchema; - emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + for (const nestedActionEvent of nestedActionEvents) { + nestedActionEvent.context.schema = updatedSchema; + emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + } } } } @@ -405,7 +421,7 @@ export class RelationsService { } if (opts?.emitEvents !== false && nestedActionEvents.length > 0) { - const updatedSchema = await getSchema({ accountability: this.accountability || undefined }); + const updatedSchema = await getSchema(); for (const nestedActionEvent of nestedActionEvents) { nestedActionEvent.context.schema = updatedSchema; diff --git a/api/src/utils/apply-snapshot.test.ts b/api/src/utils/apply-snapshot.test.ts index 81b1c2bab8..cea3b1e358 100644 --- a/api/src/utils/apply-snapshot.test.ts +++ b/api/src/utils/apply-snapshot.test.ts @@ -20,6 +20,11 @@ describe('applySnapshot', () => { let db: MockedFunction; let tracker: Tracker; + const mutationOptions = { + autoPurgeSystemCache: false, + bypassEmitAction: expect.any(Function), + }; + beforeEach(() => { db = vi.mocked(knex({ client: Client_PG })); tracker = getTracker(); @@ -112,7 +117,7 @@ describe('applySnapshot', () => { }); expect(createOneCollectionSpy).toHaveBeenCalledTimes(1); - expect(createOneCollectionSpy).toHaveBeenCalledWith(expected); + expect(createOneCollectionSpy).toHaveBeenCalledWith(expected, mutationOptions); // There should be no fields left to create // they will get filtered in createCollections @@ -263,8 +268,8 @@ describe('applySnapshot', () => { }); expect(createOneCollectionSpy).toHaveBeenCalledTimes(2); - expect(createOneCollectionSpy).toHaveBeenCalledWith(expected); - expect(createOneCollectionSpy).toHaveBeenCalledWith(expected2); + expect(createOneCollectionSpy).toHaveBeenCalledWith(expected, mutationOptions); + expect(createOneCollectionSpy).toHaveBeenCalledWith(expected2, mutationOptions); // There should be no fields left to create // they will get filtered in createCollections diff --git a/api/src/utils/apply-snapshot.ts b/api/src/utils/apply-snapshot.ts index 618d9ccdd8..b6356bc484 100644 --- a/api/src/utils/apply-snapshot.ts +++ b/api/src/utils/apply-snapshot.ts @@ -5,11 +5,12 @@ import { merge, set } from 'lodash'; import getDatabase from '../database'; import logger from '../logger'; import { CollectionsService, FieldsService, RelationsService } from '../services'; -import { Collection, Snapshot, SnapshotDiff, SnapshotField } from '../types'; +import { ActionEventParams, Collection, MutationOptions, Snapshot, SnapshotDiff, SnapshotField } from '../types'; import { getSchema } from './get-schema'; import { getSnapshot } from './get-snapshot'; import { getSnapshotDiff } from './get-snapshot-diff'; import { getCache } from '../cache'; +import emitter from '../emitter'; type CollectionDelta = { collection: string; @@ -21,12 +22,18 @@ export async function applySnapshot( options?: { database?: Knex; schema?: SchemaOverview; current?: Snapshot; diff?: SnapshotDiff } ): Promise { const database = options?.database ?? getDatabase(); - const schema = options?.schema ?? (await getSchema({ database })); + const schema = options?.schema ?? (await getSchema({ database, bypassCache: true })); const { systemCache } = getCache(); const current = options?.current ?? (await getSnapshot({ database, schema })); const snapshotDiff = options?.diff ?? getSnapshotDiff(current, snapshot); + const nestedActionEvents: ActionEventParams[] = []; + const mutationOptions: MutationOptions = { + autoPurgeSystemCache: false, + bypassEmitAction: (params) => nestedActionEvents.push(params), + }; + await database.transaction(async (trx) => { const collectionsService = new CollectionsService({ knex: trx, schema }); @@ -64,10 +71,13 @@ export async function applySnapshot( }); try { - await collectionsService.createOne({ - ...diff[0].rhs, - fields, - }); + await collectionsService.createOne( + { + ...diff[0].rhs, + fields, + }, + mutationOptions + ); } catch (err: any) { logger.error(`Failed to create collection "${collection}"`); throw err; @@ -93,7 +103,7 @@ export async function applySnapshot( for (const relation of relations) { try { - await relationsService.deleteOne(relation.collection, relation.field); + await relationsService.deleteOne(relation.collection, relation.field, mutationOptions); } catch (err) { logger.error( `Failed to delete collection "${collection}" due to relation "${relation.collection}.${relation.field}"` @@ -111,7 +121,7 @@ export async function applySnapshot( await deleteCollections(getNestedCollectionsToDelete(collection)); try { - await collectionsService.deleteOne(collection); + await collectionsService.deleteOne(collection, mutationOptions); } catch (err) { logger.error(`Failed to delete collection "${collection}"`); throw err; @@ -168,7 +178,7 @@ export async function applySnapshot( if (newValues) { try { - await collectionsService.updateOne(collection, newValues); + await collectionsService.updateOne(collection, newValues, mutationOptions); } catch (err) { logger.error(`Failed to update collection "${collection}"`); throw err; @@ -177,12 +187,15 @@ export async function applySnapshot( } } - const fieldsService = new FieldsService({ knex: trx, schema: await getSchema({ database: trx }) }); + const fieldsService = new FieldsService({ + knex: trx, + schema: await getSchema({ database: trx, bypassCache: true }), + }); for (const { collection, field, diff } of snapshotDiff.fields) { if (diff?.[0].kind === 'N' && !isNestedMetaUpdate(diff?.[0])) { try { - await fieldsService.createField(collection, (diff[0] as DiffNew).rhs); + await fieldsService.createField(collection, (diff[0] as DiffNew).rhs, undefined, mutationOptions); } catch (err) { logger.error(`Failed to create field "${collection}.${field}"`); throw err; @@ -196,9 +209,13 @@ export async function applySnapshot( if (newValues) { try { - await fieldsService.updateField(collection, { - ...newValues, - }); + await fieldsService.updateField( + collection, + { + ...newValues, + }, + mutationOptions + ); } catch (err) { logger.error(`Failed to update field "${collection}.${field}"`); throw err; @@ -208,7 +225,7 @@ export async function applySnapshot( if (diff?.[0].kind === 'D' && !isNestedMetaUpdate(diff?.[0])) { try { - await fieldsService.deleteField(collection, field); + await fieldsService.deleteField(collection, field, mutationOptions); } catch (err) { logger.error(`Failed to delete field "${collection}.${field}"`); throw err; @@ -222,7 +239,10 @@ export async function applySnapshot( } } - const relationsService = new RelationsService({ knex: trx, schema: await getSchema({ database: trx }) }); + const relationsService = new RelationsService({ + knex: trx, + schema: await getSchema({ database: trx, bypassCache: true }), + }); for (const { collection, field, diff } of snapshotDiff.relations) { const structure = {}; @@ -233,7 +253,7 @@ export async function applySnapshot( if (diff?.[0].kind === 'N') { try { - await relationsService.createOne((diff[0] as DiffNew).rhs); + await relationsService.createOne((diff[0] as DiffNew).rhs, mutationOptions); } catch (err) { logger.error(`Failed to create relation "${collection}.${field}"`); throw err; @@ -247,7 +267,7 @@ export async function applySnapshot( if (newValues) { try { - await relationsService.updateOne(collection, field, newValues); + await relationsService.updateOne(collection, field, newValues, mutationOptions); } catch (err) { logger.error(`Failed to update relation "${collection}.${field}"`); throw err; @@ -257,7 +277,7 @@ export async function applySnapshot( if (diff?.[0].kind === 'D') { try { - await relationsService.deleteOne(collection, field); + await relationsService.deleteOne(collection, field, mutationOptions); } catch (err) { logger.error(`Failed to delete relation "${collection}.${field}"`); throw err; @@ -267,6 +287,15 @@ export async function applySnapshot( }); await systemCache?.clear(); + + if (nestedActionEvents.length > 0) { + const updatedSchema = await getSchema({ database, bypassCache: true }); + + for (const nestedActionEvent of nestedActionEvents) { + nestedActionEvent.context.schema = updatedSchema; + emitter.emitAction(nestedActionEvent.event, nestedActionEvent.meta, nestedActionEvent.context); + } + } } export function isNestedMetaUpdate(diff: Diff): boolean { diff --git a/api/src/utils/get-schema.ts b/api/src/utils/get-schema.ts index 4111b040b6..0b7d837d24 100644 --- a/api/src/utils/get-schema.ts +++ b/api/src/utils/get-schema.ts @@ -1,5 +1,5 @@ import SchemaInspector from '@directus/schema'; -import { Accountability, Filter, SchemaOverview } from '@directus/shared/types'; +import { Filter, SchemaOverview } from '@directus/shared/types'; import { parseJSON, toArray } from '@directus/shared/utils'; import { Knex } from 'knex'; import { mapValues } from 'lodash'; @@ -15,15 +15,20 @@ import getDefaultValue from './get-default-value'; import getLocalType from './get-local-type'; export async function getSchema(options?: { - accountability?: Accountability; database?: Knex; + + /** + * To bypass any cached schema if bypassCache is enabled. + * Used to ensure schema snapshot/apply is not using outdated schema + */ + bypassCache?: boolean; }): Promise { const database = options?.database || getDatabase(); const schemaInspector = SchemaInspector(database); let result: SchemaOverview; - if (env.CACHE_SCHEMA !== false) { + if (!options?.bypassCache && env.CACHE_SCHEMA !== false) { let cachedSchema; try { diff --git a/api/src/utils/get-snapshot.ts b/api/src/utils/get-snapshot.ts index c1217c656c..c54aa83f22 100644 --- a/api/src/utils/get-snapshot.ts +++ b/api/src/utils/get-snapshot.ts @@ -9,7 +9,7 @@ import { SchemaOverview } from '@directus/shared/types'; export async function getSnapshot(options?: { database?: Knex; schema?: SchemaOverview }): Promise { const database = options?.database ?? getDatabase(); - const schema = options?.schema ?? (await getSchema({ database })); + const schema = options?.schema ?? (await getSchema({ database, bypassCache: true })); const collectionsService = new CollectionsService({ knex: database, schema }); const fieldsService = new FieldsService({ knex: database, schema });