From 72e3f1530646653097cb73a122b0e43391154b13 Mon Sep 17 00:00:00 2001 From: chenos Date: Sat, 18 Jun 2022 00:18:12 +0800 Subject: [PATCH] fix: remove collections & fields from db (#511) * fix: remove collections & fields from db * fix: cannot read property 'removeFromDb' of undefined * test: add test cases * test: add test cases * fix: exclude non-deletable fields --- .../database/src/__tests__/collection.test.ts | 27 ++++ packages/core/database/src/collection.ts | 24 ++- packages/core/database/src/database.ts | 14 +- packages/core/database/src/fields/field.ts | 79 +++++++++- packages/core/server/src/application.ts | 4 +- packages/core/server/src/commands/install.ts | 2 +- .../collection-manager/src/__tests__/index.ts | 5 +- .../src/__tests__/remove-collection.test.ts | 2 +- .../resources/collections.fields.test.ts | 123 +++++++++++++++ .../__tests__/resources/collections.test.ts | 130 ++++++++++++++++ .../src/models/collection.ts | 32 ++-- .../collection-manager/src/models/field.ts | 19 +-- .../plugins/collection-manager/src/server.ts | 142 ------------------ 13 files changed, 423 insertions(+), 180 deletions(-) create mode 100644 packages/plugins/collection-manager/src/__tests__/resources/collections.fields.test.ts create mode 100644 packages/plugins/collection-manager/src/__tests__/resources/collections.test.ts diff --git a/packages/core/database/src/__tests__/collection.test.ts b/packages/core/database/src/__tests__/collection.test.ts index 827c73f27c..f83ccf6ee4 100644 --- a/packages/core/database/src/__tests__/collection.test.ts +++ b/packages/core/database/src/__tests__/collection.test.ts @@ -13,6 +13,33 @@ describe('collection', () => { await db.close(); }); + test('removeFromDb', async () => { + await db.clean({ drop: true }); + const collection = db.collection({ + name: 'test', + fields: [ + { + type: 'string', + name: 'name', + }, + ], + }); + await db.sync(); + + const field = collection.getField('name'); + const r1 = await field.existsInDb(); + expect(r1).toBe(true); + await field.removeFromDb(); + const r2 = await field.existsInDb(); + expect(r2).toBe(false); + + const r3 = await collection.existsInDb(); + expect(r3).toBe(true); + await collection.removeFromDb(); + const r4 = await collection.existsInDb(); + expect(r4).toBe(false); + }); + test('collection disable authGenId', async () => { const Test = db.collection({ name: 'test', diff --git a/packages/core/database/src/collection.ts b/packages/core/database/src/collection.ts index 163ec13512..6cc8ecaac6 100644 --- a/packages/core/database/src/collection.ts +++ b/packages/core/database/src/collection.ts @@ -1,7 +1,7 @@ import merge from 'deepmerge'; import { EventEmitter } from 'events'; import { default as lodash, default as _ } from 'lodash'; -import { ModelCtor, ModelOptions, SyncOptions } from 'sequelize'; +import { ModelCtor, ModelOptions, QueryInterfaceDropTableOptions, SyncOptions, Transactionable } from 'sequelize'; import { Database } from './database'; import { Field, FieldOptions } from './fields'; import { Model } from './model'; @@ -53,6 +53,10 @@ export class Collection< return this.options.name; } + get db() { + return this.context.database; + } + constructor(options: CollectionOptions, context?: CollectionContext) { super(); this.context = context; @@ -190,6 +194,22 @@ export class Collection< this.context.database.removeCollection(this.name); } + async removeFromDb(options?: QueryInterfaceDropTableOptions) { + if ( + await this.existsInDb({ + transaction: options?.transaction, + }) + ) { + const queryInterface = this.db.sequelize.getQueryInterface(); + await queryInterface.dropTable(this.model.tableName, options); + } + this.remove(); + } + + async existsInDb(options?: Transactionable) { + return this.db.collectionExistsInDb(this.name, options); + } + removeField(name) { if (!this.fields.has(name)) { return; @@ -199,7 +219,7 @@ export class Collection< if (bool) { this.emit('field.afterRemove', field); } - return bool; + return field as Field; } /** diff --git a/packages/core/database/src/database.ts b/packages/core/database/src/database.ts index 6c74eec1ca..2d78e77605 100644 --- a/packages/core/database/src/database.ts +++ b/packages/core/database/src/database.ts @@ -12,6 +12,7 @@ import { QueryOptions, Sequelize, SyncOptions, + Transactionable, Utils } from 'sequelize'; import { SequelizeStorage, Umzug } from 'umzug'; @@ -26,7 +27,6 @@ import extendOperators from './operators'; import { RelationRepository } from './relation-repository/relation-repository'; import { Repository } from './repository'; - export interface MergeOptions extends merge.Options {} export interface PendingOptions { @@ -230,11 +230,13 @@ export class Database extends EventEmitter implements AsyncEmitter { const result = this.collections.delete(name); + this.sequelize.modelManager.removeModel(collection.model); + if (result) { this.emit('afterRemoveCollection', collection); } - return result; + return collection; } getModel(name: string) { @@ -339,9 +341,11 @@ export class Database extends EventEmitter implements AsyncEmitter { } } - async doesCollectionExistInDb(name) { - const tables = await this.sequelize.getQueryInterface().showAllTables(); - return tables.find((table) => table === `${this.getTablePrefix()}${name}`); + async collectionExistsInDb(name, options?: Transactionable) { + const tables = await this.sequelize.getQueryInterface().showAllTables({ + transaction: options?.transaction, + }); + return !!tables.find((table) => table === `${this.getTablePrefix()}${name}`); } public isSqliteMemory() { diff --git a/packages/core/database/src/fields/field.ts b/packages/core/database/src/fields/field.ts index 895d4ea9a0..2d34986687 100644 --- a/packages/core/database/src/fields/field.ts +++ b/packages/core/database/src/fields/field.ts @@ -1,5 +1,12 @@ import _ from 'lodash'; -import { DataType, ModelAttributeColumnOptions, ModelIndexesOptions, SyncOptions } from 'sequelize'; +import { + DataType, + ModelAttributeColumnOptions, + ModelIndexesOptions, + QueryInterfaceOptions, + SyncOptions, + Transactionable +} from 'sequelize'; import { Collection } from '../collection'; import { Database } from '../database'; @@ -79,6 +86,76 @@ export abstract class Field { return this.collection.removeField(this.name); } + async removeFromDb(options?: QueryInterfaceOptions) { + if (!this.collection.model.rawAttributes[this.name]) { + this.remove(); + // console.log('field is not attribute'); + return; + } + if ((this.collection.model as any)._virtualAttributes.has(this.name)) { + this.remove(); + // console.log('field is virtual attribute'); + return; + } + if (this.collection.model.primaryKeyAttributes.includes(this.name)) { + // 主键不能删除 + return; + } + if (this.collection.model.options.timestamps !== false) { + // timestamps 相关字段不删除 + if (['createdAt', 'updatedAt', 'deletedAt'].includes(this.name)) { + return; + } + } + // 排序字段通过 sortable 控制 + const sortable = this.collection.options.sortable; + if (sortable) { + let sortField: string; + if (sortable === true) { + sortField = 'sort'; + } else if (typeof sortable === 'string') { + sortField = sortable; + } else if (sortable.name) { + sortField = sortable.name || 'sort'; + } + if (this.name === sortField) { + return; + } + } + if (this.options.field && this.name !== this.options.field) { + // field 指向的是真实的字段名，如果与 name 不一样，说明字段只是引用 + this.remove(); + return; + } + if ( + await this.existsInDb({ + transaction: options?.transaction, + }) + ) { + const queryInterface = this.database.sequelize.getQueryInterface(); + await queryInterface.removeColumn(this.collection.model.tableName, this.name, options); + } + this.remove(); + } + + async existsInDb(options?: Transactionable) { + const opts = { + transaction: options?.transaction, + }; + let sql; + if (this.database.sequelize.getDialect() === 'sqlite') { + sql = `SELECT * from pragma_table_info('${this.collection.model.tableName}') WHERE name = '${this.name}'`; + } else { + sql = ` + select column_name + from INFORMATION_SCHEMA.COLUMNS + where TABLE_NAME='${this.collection.model.tableName}' AND column_name='${this.name}' + `; + } + const [rows] = await this.database.sequelize.query(sql, opts); + return rows.length > 0; + } + merge(obj: any) { Object.assign(this.options, obj); } diff --git a/packages/core/server/src/application.ts b/packages/core/server/src/application.ts index e7b52fe80c..8c4fcfa6cf 100644 --- a/packages/core/server/src/application.ts +++ b/packages/core/server/src/application.ts @@ -97,7 +97,7 @@ export class ApplicationVersion { } async get() { - if (await this.app.db.doesCollectionExistInDb('applicationVersion')) { + if (await this.app.db.collectionExistsInDb('applicationVersion')) { const model = await this.collection.model.findOne(); return model.get('value') as any; } @@ -115,7 +115,7 @@ export class ApplicationVersion { } async satisfies(range: string) { - if (await this.app.db.doesCollectionExistInDb('applicationVersion')) { + if (await this.app.db.collectionExistsInDb('applicationVersion')) { const model = await this.collection.model.findOne(); const version = model.get('value') as any; return semver.satisfies(version, range); diff --git a/packages/core/server/src/commands/install.ts b/packages/core/server/src/commands/install.ts index ab0495c6a8..07e71df5ae 100644 --- a/packages/core/server/src/commands/install.ts +++ b/packages/core/server/src/commands/install.ts @@ -20,7 +20,7 @@ export default (app: Application) => { } if (!opts?.clean && !opts?.force) { - if (app.db.doesCollectionExistInDb('applicationVersion')) { + if (app.db.collectionExistsInDb('applicationVersion')) { installed = true; if (!opts.silent) { console.log('NocoBase is already installed. To reinstall, please execute:'); diff --git a/packages/plugins/collection-manager/src/__tests__/index.ts b/packages/plugins/collection-manager/src/__tests__/index.ts index 8a7a811b93..51fa038cb7 100644 --- a/packages/plugins/collection-manager/src/__tests__/index.ts +++ b/packages/plugins/collection-manager/src/__tests__/index.ts @@ -1,8 +1,7 @@ -import { mockServer } from '@nocobase/test'; import PluginUiSchema from '@nocobase/plugin-ui-schema-storage'; - -import CollectionManagerPlugin from '..'; +import { mockServer } from '@nocobase/test'; import lodash from 'lodash'; +import CollectionManagerPlugin from '../'; export async function createApp(options = {}) { const app = mockServer(); diff --git a/packages/plugins/collection-manager/src/__tests__/remove-collection.test.ts b/packages/plugins/collection-manager/src/__tests__/remove-collection.test.ts index d23679c394..da915fac10 100644 --- a/packages/plugins/collection-manager/src/__tests__/remove-collection.test.ts +++ b/packages/plugins/collection-manager/src/__tests__/remove-collection.test.ts @@ -25,7 +25,7 @@ describe('collections repository', () => { context: {}, values: { name: 'posts', - fields: [{ type: 'hasMany', name: 'comments', options: { target: 'comments' } }], + fields: [{ type: 'hasMany', name: 'comments', target: 'comments' }], }, }); diff --git a/packages/plugins/collection-manager/src/__tests__/resources/collections.fields.test.ts b/packages/plugins/collection-manager/src/__tests__/resources/collections.fields.test.ts new file mode 100644 index 0000000000..1a3762b6f9 --- /dev/null +++ b/packages/plugins/collection-manager/src/__tests__/resources/collections.fields.test.ts @@ -0,0 +1,123 @@ +import { MockServer } from '@nocobase/test'; +import { createApp } from '..'; + +describe('collections.fields', () => { + let app: MockServer; + + beforeEach(async () => { + app = await createApp(); + await app.install({ clean: true }); + }); + + afterEach(async () => { + await app.destroy(); + }); + + test('destroy field', async () => { + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test1', + fields: [ + { + type: 'string', + name: 'name', + }, + ], + }, + }); + const collection = app.db.getCollection('test1'); + const field = collection.getField('name'); + expect(collection.hasField('name')).toBeTruthy(); + const r1 = await field.existsInDb(); + expect(r1).toBeTruthy(); + await app.agent().resource('collections.fields', 'test1').destroy({ + filterByTk: 'name', + }); + expect(collection.hasField('name')).toBeFalsy(); + const r2 = await field.existsInDb(); + expect(r2).toBeFalsy(); + }); + + test('destroy field', async () => { + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test1', + }, + }); + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test2', + }, + }); + await app + .agent() + .resource('collections.fields', 'test1') + .create({ + values: { + type: 'string', + name: 'name', + }, + }); + const collection = app.db.getCollection('test1'); + const field = collection.getField('name'); + expect(collection.hasField('name')).toBeTruthy(); + const r1 = await field.existsInDb(); + expect(r1).toBeTruthy(); + await app.agent().resource('collections.fields', 'test1').destroy({ + filterByTk: 'name', + }); + expect(collection.hasField('name')).toBeFalsy(); + const r2 = await field.existsInDb(); + expect(r2).toBeFalsy(); + }); + + test('remove association field', async () => { + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test1', + }, + }); + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test2', + }, + }); + await app + .agent() + .resource('collections.fields', 'test1') + .create({ + values: { + type: 'belongsTo', + name: 'test2', + target: 'test2', + reverseField: { + name: 'test1', + }, + }, + }); + const collection = app.db.getCollection('test1'); + const collection2 = app.db.getCollection('test2'); + expect(collection.hasField('test2')).toBeTruthy(); + expect(collection2.hasField('test1')).toBeTruthy(); + await app.agent().resource('collections.fields', 'test1').destroy({ + filterByTk: 'test2', + }); + expect(collection.hasField('test2')).toBeFalsy(); + expect(collection2.hasField('test1')).toBeTruthy(); + }); +}); diff --git a/packages/plugins/collection-manager/src/__tests__/resources/collections.test.ts b/packages/plugins/collection-manager/src/__tests__/resources/collections.test.ts new file mode 100644 index 0000000000..c789fc06d2 --- /dev/null +++ b/packages/plugins/collection-manager/src/__tests__/resources/collections.test.ts @@ -0,0 +1,130 @@ +import { HasManyRepository } from '@nocobase/database'; +import { MockServer } from '@nocobase/test'; +import { createApp } from '..'; + +describe('collections', () => { + let app: MockServer; + + beforeEach(async () => { + app = await createApp(); + await app.install({ clean: true }); + }); + + afterEach(async () => { + await app.destroy(); + }); + + test('remove collection', async () => { + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test', + }, + }); + const collection = app.db.getCollection('test'); + const r1 = await collection.existsInDb(); + expect(r1).toBe(true); + await app.agent().resource('collections').destroy({ + filterByTk: 'test', + }); + const r2 = await collection.existsInDb(); + expect(r2).toBe(false); + }); + + test('remove collection', async () => { + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test1', + }, + }); + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test2', + }, + }); + await app + .agent() + .resource('collections.fields', 'test1') + .create({ + values: { + type: 'belongsTo', + name: 'test2', + target: 'test2', + reverseField: { + name: 'test1', + }, + }, + }); + await app.agent().resource('collections').destroy({ + filterByTk: 'test1', + }); + expect(app.db.hasCollection('test1')).toBeFalsy(); + expect(!!app.db.sequelize.modelManager.getModel('test1')).toBeFalsy(); + const collection2 = app.db.getCollection('test2'); + expect(collection2.hasField('test2')).toBeFalsy(); + const count = await app.db.getRepository('collections.fields', 'test2').count({ + filter: { + name: 'test2', + }, + }); + expect(count).toBe(0); + }); + + test('remove collection', async () => { + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test1', + }, + }); + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test2', + }, + }); + await app + .agent() + .resource('collections') + .create({ + values: { + name: 'test3', + }, + }); + await app + .agent() + .resource('collections.fields', 'test1') + .create({ + values: { + type: 'belongsToMany', + name: 'test2', + target: 'test2', + through: 'test3', + reverseField: { + name: 'test1', + }, + }, + }); + await app.agent().resource('collections').destroy({ + filterByTk: 'test3', + }); + expect(app.db.hasCollection('test3')).toBeFalsy(); + expect(!!app.db.sequelize.modelManager.getModel('test3')).toBeFalsy(); + const collection1 = app.db.getCollection('test1'); + expect(collection1.hasField('test2')).toBeFalsy(); + const collection2 = app.db.getCollection('test2'); + expect(collection2.hasField('test1')).toBeFalsy(); + }); +}); diff --git a/packages/plugins/collection-manager/src/models/collection.ts b/packages/plugins/collection-manager/src/models/collection.ts index 7e3662a97e..4ab0245beb 100644 --- a/packages/plugins/collection-manager/src/models/collection.ts +++ b/packages/plugins/collection-manager/src/models/collection.ts @@ -50,19 +50,29 @@ export class CollectionModel extends MagicAttributeModel { } } - /** - * TODO: drop table from the database - * - * @param options - * @returns - */ async remove(options?: any) { + const { transaction } = options || {}; 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; + const collection = this.db.getCollection(name); + if (!collection) { + return; + } + const fields = await this.db.getRepository('fields').find({ + filter: { + 'type.$in': ['belongsToMany', 'belongsTo', 'hasMany', 'hasOne'], + }, + transaction, + }); + for (const field of fields) { + if (field.get('target') && field.get('target') === name) { + await field.destroy({ transaction }); + } else if (field.get('through') && field.get('through') === name) { + await field.destroy({ transaction }); + } + } + await collection.removeFromDb({ + transaction, + }); } async migrate(options?: SyncOptions & Transactionable) { diff --git a/packages/plugins/collection-manager/src/models/field.ts b/packages/plugins/collection-manager/src/models/field.ts index e6f2b998b4..1218039040 100644 --- a/packages/plugins/collection-manager/src/models/field.ts +++ b/packages/plugins/collection-manager/src/models/field.ts @@ -50,23 +50,18 @@ export class FieldModel extends MagicAttributeModel { } } - /** - * 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; + const field = collection.getField(this.get('name')); + if (!field) { + return; + } + return field.removeFromDb({ + transaction: options.transaction, + }); } } diff --git a/packages/plugins/collection-manager/src/server.ts b/packages/plugins/collection-manager/src/server.ts index 70537814e3..33ff5156c7 100644 --- a/packages/plugins/collection-manager/src/server.ts +++ b/packages/plugins/collection-manager/src/server.ts @@ -65,28 +65,6 @@ export class CollectionManagerPlugin extends Plugin { } }); - this.app.db.on('collections.afterDestroy', async (model, { transaction }) => { - const name = model.get('name'); - - const fields = await this.app.db.getRepository('fields').find({ - filter: { - 'type.$in': ['belongsToMany', 'belongsTo', 'hasMany', 'hasOne'], - }, - transaction, - }); - - const deleteFieldsKey = fields - .filter((field) => (field.get('options') as any)?.target === name) - .map((field) => field.get('key') as string); - - await this.app.db.getRepository('fields').destroy({ - filter: { - 'key.$in': deleteFieldsKey, - }, - transaction, - }); - }); - this.app.db.on('fields.afterCreate', async (model, { context, transaction }) => { if (context) { await model.migrate({ transaction }); @@ -99,126 +77,6 @@ export class CollectionManagerPlugin extends Plugin { } }); - // 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); });