From a92a78cc9e93924c431bb0183be500db650889d9 Mon Sep 17 00:00:00 2001 From: chenos Date: Sat, 11 Jun 2022 20:46:30 +0800 Subject: [PATCH] fix: rollback when field creation fails (#498) * fix: rollback when field creation fails * fix: missing transaction --- packages/core/actions/src/actions/create.ts | 1 - packages/core/database/src/collection.ts | 4 + packages/core/database/src/database.ts | 2 + packages/core/database/src/fields/field.ts | 4 + .../relation-repository.ts | 1 + .../src/__tests__/through.test.ts | 119 ++++----- .../src/models/collection.ts | 36 ++- .../collection-manager/src/models/field.ts | 28 +- .../plugins/collection-manager/src/plugin.ts | 244 +++++++++--------- 9 files changed, 249 insertions(+), 190 deletions(-) diff --git a/packages/core/actions/src/actions/create.ts b/packages/core/actions/src/actions/create.ts index 2a08776de..34c00da40 100644 --- a/packages/core/actions/src/actions/create.ts +++ b/packages/core/actions/src/actions/create.ts @@ -12,7 +12,6 @@ export async function create(ctx: Context, next) { updateAssociationValues, context: ctx, }); - ctx.body = instance; await next(); diff --git a/packages/core/database/src/collection.ts b/packages/core/database/src/collection.ts index 922591d5b..163ec1351 100644 --- a/packages/core/database/src/collection.ts +++ b/packages/core/database/src/collection.ts @@ -186,6 +186,10 @@ export class Collection< } } + remove() { + this.context.database.removeCollection(this.name); + } + removeField(name) { if (!this.fields.has(name)) { return; diff --git a/packages/core/database/src/database.ts b/packages/core/database/src/database.ts index 656d621df..0e41538b5 100644 --- a/packages/core/database/src/database.ts +++ b/packages/core/database/src/database.ts @@ -198,6 +198,8 @@ export class Database extends EventEmitter implements AsyncEmitter { if (result) { this.emit('afterRemoveCollection', collection); } + + return result; } getModel(name: string) { diff --git a/packages/core/database/src/fields/field.ts b/packages/core/database/src/fields/field.ts index 256a89373..895d4ea9a 100644 --- a/packages/core/database/src/fields/field.ts +++ b/packages/core/database/src/fields/field.ts @@ -75,6 +75,10 @@ export abstract class Field { return this.options[name]; } + remove() { + return this.collection.removeField(this.name); + } + merge(obj: any) { Object.assign(this.options, obj); } diff --git a/packages/core/database/src/relation-repository/relation-repository.ts b/packages/core/database/src/relation-repository/relation-repository.ts index 51d63314c..84f593e3d 100644 --- a/packages/core/database/src/relation-repository/relation-repository.ts +++ b/packages/core/database/src/relation-repository/relation-repository.ts @@ -48,6 +48,7 @@ export abstract class RelationRepository { return (this.association).accessors; } + @transaction() async create(options?: CreateOptions): Promise { const createAccessor = this.accessors().create; diff --git a/packages/plugins/collection-manager/src/__tests__/through.test.ts b/packages/plugins/collection-manager/src/__tests__/through.test.ts index d702ddf36..33981d1d8 100644 --- a/packages/plugins/collection-manager/src/__tests__/through.test.ts +++ b/packages/plugins/collection-manager/src/__tests__/through.test.ts @@ -1,36 +1,24 @@ -import { mockServer, MockServer } from '@nocobase/test'; +import { mockServer } from '@nocobase/test'; import CollectionManagerPlugin from '../plugin'; describe('collections repository', () => { - let app: MockServer; - - beforeEach(async () => { - app = mockServer({ + it('case 1', async () => { + const app1 = mockServer({ database: { tablePrefix: 'through_', }, }); - await app.cleanDb(); - app.plugin(CollectionManagerPlugin); - await app.load(); - await app.install({ clean: true }); - await app.start(); - }); - - afterEach(async () => { - await app.destroy(); - }); - - it('case 1', async () => { - await app + await app1.cleanDb(); + app1.plugin(CollectionManagerPlugin); + await app1.load(); + await app1.install({ clean: true }); + await app1.start(); + await app1 .agent() .resource('collections') .create({ values: { name: 'resumes', - createdBy: true, - updatedBy: true, - sortable: true, fields: [ { name: 'id', @@ -38,23 +26,17 @@ describe('collections repository', () => { autoIncrement: true, primaryKey: true, allowNull: false, - uiSchema: { type: 'number', title: '{{t("ID")}}', 'x-component': 'InputNumber', 'x-read-pretty': true }, - interface: 'id', }, ], - title: '简历', }, }); - await app + await app1 .agent() .resource('collections') .create({ values: { name: 'jobs', - createdBy: true, - updatedBy: true, - sortable: true, fields: [ { name: 'id', @@ -62,23 +44,17 @@ describe('collections repository', () => { autoIncrement: true, primaryKey: true, allowNull: false, - uiSchema: { type: 'number', title: '{{t("ID")}}', 'x-component': 'InputNumber', 'x-read-pretty': true }, - interface: 'id', }, ], - title: '职位', }, }); - await app + await app1 .agent() .resource('collections') .create({ values: { name: 'matches', - createdBy: true, - updatedBy: true, - sortable: true, fields: [ { name: 'id', @@ -86,47 +62,57 @@ describe('collections repository', () => { autoIncrement: true, primaryKey: true, allowNull: false, - uiSchema: { type: 'number', title: '{{t("ID")}}', 'x-component': 'InputNumber', 'x-read-pretty': true }, - interface: 'id', }, ], - title: '匹配', }, }); - await app + await app1 .agent() .resource('collections.fields', 'resumes') .create({ values: { name: 'jobs', type: 'belongsToMany', - uiSchema: { - 'x-component': 'RecordPicker', - 'x-component-props': { multiple: true, fieldNames: { label: 'id', value: 'id' } }, - title: '职位', - }, + foreignKey: 'rid', + otherKey: 'jid', reverseField: { - interface: 'linkTo', type: 'belongsToMany', - uiSchema: { - 'x-component': 'RecordPicker', - 'x-component-props': { multiple: true, fieldNames: { label: 'id', value: 'id' } }, - title: '简历', - }, + name: 'resumes', }, - interface: 'linkTo', target: 'jobs', through: 'matches', }, }); - const matchesCollection = app.db.getCollection('matches'); + await app1 + .agent() + .resource('collections.fields', 'resumes') + .create({ + values: { + name: 'matches2', + type: 'hasMany', + target: 'matches', + foreignKey: 'rid', + reverseField: { + name: 'resume', + }, + }, + }); - const matchesFields = [...matchesCollection.fields.entries()]; - const matchJobField = matchesFields.find((item) => item[1].options.target == 'jobs'); - - expect(matchesCollection.model.rawAttributes[matchJobField[1].options.foreignKey].primaryKey).not.toBeTruthy(); + const job1 = await app1.db.getRepository('jobs').create({}); + await app1.db.getRepository('resumes').create({ + values: { + jobs: [job1.get('id')], + }, + }); + const match1 = await app1.db.getRepository('matches').findOne(); + expect(match1.toJSON()).toMatchObject({ + id: 1, + rid: 1, + jid: 1, + }); + await app1.destroy(); const app2 = mockServer({ database: { @@ -137,12 +123,21 @@ describe('collections repository', () => { await app2.load(); await app2.start(); - await app2.db.sync(); - - expect( - app.db.getCollection('matches').model.rawAttributes[matchJobField[1].options.foreignKey].primaryKey, - ).not.toBeTruthy(); - + await app2.db.sync({ + force: true, + }); + const job = await app2.db.getRepository('jobs').create({}); + await app2.db.getRepository('resumes').create({ + values: { + jobs: [job.get('id')], + }, + }); + const match = await app2.db.getRepository('matches').findOne(); + expect(match.toJSON()).toMatchObject({ + id: 1, + rid: 1, + jid: 1, + }); await app2.destroy(); }); }); diff --git a/packages/plugins/collection-manager/src/models/collection.ts b/packages/plugins/collection-manager/src/models/collection.ts index b1cc696ec..7e3662a97 100644 --- a/packages/plugins/collection-manager/src/models/collection.ts +++ b/packages/plugins/collection-manager/src/models/collection.ts @@ -1,5 +1,5 @@ -import { SyncOptions, Transactionable } from 'sequelize'; import Database, { Collection, MagicAttributeModel } from '@nocobase/database'; +import { SyncOptions, Transactionable } from 'sequelize'; import { FieldModel } from './field'; interface LoadOptions extends Transactionable { @@ -50,16 +50,36 @@ export class CollectionModel extends MagicAttributeModel { } } + /** + * TODO: drop table from the database + * + * @param options + * @returns + */ + async remove(options?: any) { + const name = this.get('name'); + // delete from memory + const result = this.db.removeCollection(name); + // TODO: drop table from the database + // this.db.sequelize.getQueryInterface().dropTable(this.get('name')); + return result; + } + async migrate(options?: SyncOptions & Transactionable) { const collection = await this.load({ transaction: options?.transaction, }); - await collection.sync({ - force: false, - alter: { - drop: false, - }, - ...options, - }); + try { + await collection.sync({ + force: false, + alter: { + drop: false, + }, + ...options, + }); + } catch (error) { + const name = this.get('name'); + this.db.removeCollection(name); + } } } diff --git a/packages/plugins/collection-manager/src/models/field.ts b/packages/plugins/collection-manager/src/models/field.ts index f602b35e9..f0dd6f485 100644 --- a/packages/plugins/collection-manager/src/models/field.ts +++ b/packages/plugins/collection-manager/src/models/field.ts @@ -41,6 +41,32 @@ export class FieldModel extends MagicAttributeModel { if (!field) { return; } - await field.sync(options); + try { + await field.sync(options); + } catch (error) { + // field sync failed, delete from memory + field.remove(); + throw error; + } + } + + /** + * TODO: drop column from the database + * + * @param options + * @returns + */ + async remove(options?: any) { + const collectionName = this.get('collectionName'); + const fieldName = this.get('name'); + if (!this.db.hasCollection(collectionName)) { + return; + } + const collection = this.db.getCollection(collectionName); + // delete from memory + const result = collection.removeField(this.get('name')); + // TODO: drop column from the database + // this.db.sequelize.getQueryInterface().removeColumn(collectionName, fieldName); + return result; } } diff --git a/packages/plugins/collection-manager/src/plugin.ts b/packages/plugins/collection-manager/src/plugin.ts index 212c4a6da..6980b8538 100644 --- a/packages/plugins/collection-manager/src/plugin.ts +++ b/packages/plugins/collection-manager/src/plugin.ts @@ -1,6 +1,5 @@ import { Collection } from '@nocobase/database'; import { Plugin } from '@nocobase/server'; -import { uid } from '@nocobase/utils'; import lodash from 'lodash'; import path from 'path'; import { CollectionRepository } from '.'; @@ -92,123 +91,132 @@ export class CollectionManagerPlugin extends Plugin { } }); - this.app.db.on('fields.afterCreateWithAssociations', async (model, { context, transaction }) => { - if (!context) { - return; - } - if (!model.get('through')) { - return; - } - const [throughName, sourceName, targetName] = [ - model.get('through'), - model.get('collectionName'), - model.get('target'), - ]; - const db = this.app.db; - const through = await db.getRepository('collections').findOne({ - filter: { - name: throughName, - }, - transaction, - }); - if (!through) { - return; - } - const repository = db.getRepository('collections.fields', throughName); - await repository.create({ - transaction, - values: { - name: `f_${uid()}`, - type: 'belongsTo', - target: sourceName, - targetKey: model.get('sourceKey'), - foreignKey: model.get('foreignKey'), - interface: 'linkTo', - reverseField: { - interface: 'subTable', - uiSchema: { - type: 'void', - title: through.get('title'), - 'x-component': 'TableField', - 'x-component-props': {}, - }, - // uiSchema: { - // title: through.get('title'), - // 'x-component': 'RecordPicker', - // 'x-component-props': { - // // mode: 'tags', - // multiple: true, - // fieldNames: { - // label: 'id', - // value: 'id', - // }, - // }, - // }, - }, - uiSchema: { - title: db.getCollection(sourceName)?.options?.title || sourceName, - 'x-component': 'RecordPicker', - 'x-component-props': { - // mode: 'tags', - multiple: false, - fieldNames: { - label: 'id', - value: 'id', - }, - }, - }, - }, - }); - await repository.create({ - transaction, - values: { - name: `f_${uid()}`, - type: 'belongsTo', - target: targetName, - targetKey: model.get('targetKey'), - foreignKey: model.get('otherKey'), - interface: 'linkTo', - reverseField: { - interface: 'subTable', - uiSchema: { - type: 'void', - title: through.get('title'), - 'x-component': 'TableField', - 'x-component-props': {}, - }, - // interface: 'linkTo', - // uiSchema: { - // title: through.get('title'), - // 'x-component': 'RecordPicker', - // 'x-component-props': { - // // mode: 'tags', - // multiple: true, - // fieldNames: { - // label: 'id', - // value: 'id', - // }, - // }, - // }, - }, - uiSchema: { - title: db.getCollection(targetName)?.options?.title || targetName, - 'x-component': 'RecordPicker', - 'x-component-props': { - // mode: 'tags', - multiple: false, - fieldNames: { - label: 'id', - value: 'id', - }, - }, - }, - }, - }); - await db.getRepository('collections').load({ - filter: { - 'name.$in': [throughName, sourceName, targetName], - }, - }); + // this.app.db.on('fields.afterCreateWithAssociations', async (model, { context, transaction }) => { + // return; + // if (!context) { + // return; + // } + // if (!model.get('through')) { + // return; + // } + // const [throughName, sourceName, targetName] = [ + // model.get('through'), + // model.get('collectionName'), + // model.get('target'), + // ]; + // const db = this.app.db; + // const through = await db.getRepository('collections').findOne({ + // filter: { + // name: throughName, + // }, + // transaction, + // }); + // if (!through) { + // return; + // } + // const repository = db.getRepository('collections.fields', throughName); + // await repository.create({ + // transaction, + // values: { + // name: `f_${uid()}`, + // type: 'belongsTo', + // target: sourceName, + // targetKey: model.get('sourceKey'), + // foreignKey: model.get('foreignKey'), + // interface: 'linkTo', + // reverseField: { + // interface: 'subTable', + // uiSchema: { + // type: 'void', + // title: through.get('title'), + // 'x-component': 'TableField', + // 'x-component-props': {}, + // }, + // // uiSchema: { + // // title: through.get('title'), + // // 'x-component': 'RecordPicker', + // // 'x-component-props': { + // // // mode: 'tags', + // // multiple: true, + // // fieldNames: { + // // label: 'id', + // // value: 'id', + // // }, + // // }, + // // }, + // }, + // uiSchema: { + // title: db.getCollection(sourceName)?.options?.title || sourceName, + // 'x-component': 'RecordPicker', + // 'x-component-props': { + // // mode: 'tags', + // multiple: false, + // fieldNames: { + // label: 'id', + // value: 'id', + // }, + // }, + // }, + // }, + // }); + // await repository.create({ + // transaction, + // values: { + // name: `f_${uid()}`, + // type: 'belongsTo', + // target: targetName, + // targetKey: model.get('targetKey'), + // foreignKey: model.get('otherKey'), + // interface: 'linkTo', + // reverseField: { + // interface: 'subTable', + // uiSchema: { + // type: 'void', + // title: through.get('title'), + // 'x-component': 'TableField', + // 'x-component-props': {}, + // }, + // // interface: 'linkTo', + // // uiSchema: { + // // title: through.get('title'), + // // 'x-component': 'RecordPicker', + // // 'x-component-props': { + // // // mode: 'tags', + // // multiple: true, + // // fieldNames: { + // // label: 'id', + // // value: 'id', + // // }, + // // }, + // // }, + // }, + // uiSchema: { + // title: db.getCollection(targetName)?.options?.title || targetName, + // 'x-component': 'RecordPicker', + // 'x-component-props': { + // // mode: 'tags', + // multiple: false, + // fieldNames: { + // label: 'id', + // value: 'id', + // }, + // }, + // }, + // }, + // }); + // await db.getRepository('collections').load({ + // filter: { + // 'name.$in': [throughName, sourceName, targetName], + // }, + // }); + // }); + + this.app.db.on('fields.afterDestroy', async (model, options) => { + await model.remove(options); + }); + + this.app.db.on('collections.afterDestroy', async (model, options) => { + await model.remove(options); }); this.app.on('beforeStart', async () => {