From 1694eb6d7384af8adbb48f2721bfe96f5a178518 Mon Sep 17 00:00:00 2001 From: ChengLei Shao Date: Fri, 1 Sep 2023 13:51:48 +0800 Subject: [PATCH] fix(collection-manager): redundant fields after set collection fields (#2575) * fix(collection-manager): redundant fields after set collection fields * fix: destory with individuals hook * chore: save * chore: test * chore: mutex in fields.afterDestroy * chore: yarn.lock * chore: update collections.setFields --- packages/core/database/src/repository.ts | 10 +-- packages/plugins/acl/package.json | 3 +- .../__tests__/association-field.test.ts | 84 +++++++++++++++++++ packages/plugins/acl/src/server/server.ts | 43 +++++----- .../__tests__/http-api/recreate.test.ts | 31 +++++++ .../src/server/resourcers/collections.ts | 46 ++++++++-- yarn.lock | 7 ++ 7 files changed, 192 insertions(+), 32 deletions(-) diff --git a/packages/core/database/src/repository.ts b/packages/core/database/src/repository.ts index 7f3215180..3c9ad81a5 100644 --- a/packages/core/database/src/repository.ts +++ b/packages/core/database/src/repository.ts @@ -37,8 +37,6 @@ import { UpdateGuard } from './update-guard'; const debug = require('debug')('noco-database'); -export interface IRepository {} - interface CreateManyOptions extends BulkCreateOptions { records: Values[]; } @@ -145,7 +143,7 @@ export interface UpdateOptions extends Omit { context?: any; } -interface UpdateManyOptions extends UpdateOptions { +interface UpdateManyOptions extends Omit { records: Values[]; } @@ -221,9 +219,7 @@ interface FirstOrCreateOptions extends Transactionable { values?: Values; } -export class Repository - implements IRepository -{ +export class Repository { database: Database; collection: Collection; model: ModelStatic; @@ -242,7 +238,7 @@ export class Repository { - return !Boolean(chunk.match(/\d+/)); + return !chunk.match(/\d+/); }) .join('.'); }; diff --git a/packages/plugins/acl/package.json b/packages/plugins/acl/package.json index fcfdc7c15..769c16d9f 100644 --- a/packages/plugins/acl/package.json +++ b/packages/plugins/acl/package.json @@ -11,7 +11,8 @@ "@types/jsonwebtoken": "^8.5.8", "jsonwebtoken": "^8.5.1", "react": "^18.2.0", - "react-dom": "^18.2.0" + "react-dom": "^18.2.0", + "async-mutex": "^0.4.0" }, "peerDependencies": { "@nocobase/acl": "0.x", diff --git a/packages/plugins/acl/src/server/__tests__/association-field.test.ts b/packages/plugins/acl/src/server/__tests__/association-field.test.ts index 0bb7ed942..e17274129 100644 --- a/packages/plugins/acl/src/server/__tests__/association-field.test.ts +++ b/packages/plugins/acl/src/server/__tests__/association-field.test.ts @@ -297,6 +297,90 @@ describe('association field acl', () => { ).toBeNull(); }); + it('should not redundant fields after field set', async () => { + await db.getRepository('collections').create({ + values: { + name: 'posts', + }, + context: {}, + }); + + await db.getRepository('collections.fields', 'posts').create({ + values: { + name: 'title', + type: 'string', + }, + context: {}, + }); + + await db.getRepository('collections.fields', 'posts').create({ + values: { + name: 'content', + type: 'string', + }, + context: {}, + }); + + await adminAgent.resource('roles.resources', 'new').create({ + values: { + name: 'posts', + usingActionsConfig: true, + actions: [ + { + name: 'create', + fields: ['content', 'title'], + }, + ], + }, + }); + + expect( + acl.can({ + role: 'new', + resource: 'posts', + action: 'create', + }), + ).toMatchObject({ + role: 'new', + resource: 'posts', + action: 'create', + params: { + whitelist: ['content', 'title'], + }, + }); + + await adminAgent.resource('collections').setFields({ + filterByTk: 'posts', + values: { + fields: [ + { + name: 'name', + type: 'string', + }, + { + name: 'content', + type: 'text', + }, + ], + }, + }); + + expect( + acl.can({ + role: 'new', + resource: 'posts', + action: 'create', + }), + ).toMatchObject({ + role: 'new', + resource: 'posts', + action: 'create', + params: { + whitelist: ['content', 'name'], + }, + }); + }); + it('should revoke association action on field deleted', async () => { await adminAgent.resource('roles.resources', 'new').update({ filterByTk: 'users', diff --git a/packages/plugins/acl/src/server/server.ts b/packages/plugins/acl/src/server/server.ts index e3f6f14cc..f90fe9ac8 100644 --- a/packages/plugins/acl/src/server/server.ts +++ b/packages/plugins/acl/src/server/server.ts @@ -12,6 +12,7 @@ import { setCurrentRole } from './middlewares/setCurrentRole'; import { RoleModel } from './model/RoleModel'; import { RoleResourceActionModel } from './model/RoleResourceActionModel'; import { RoleResourceModel } from './model/RoleResourceModel'; +import { Mutex } from 'async-mutex'; export interface AssociationFieldAction { associationActions: string[]; @@ -316,30 +317,34 @@ export class PluginACL extends Plugin { } }); + const mutex = new Mutex(); + this.app.db.on('fields.afterDestroy', async (model, options) => { - const collectionName = model.get('collectionName'); - const fieldName = model.get('name'); + await mutex.runExclusive(async () => { + const collectionName = model.get('collectionName'); + const fieldName = model.get('name'); - const resourceActions = await this.app.db.getRepository('rolesResourcesActions').find({ - filter: { - 'resource.name': collectionName, - 'fields.$anyOf': [fieldName], - }, - transaction: options.transaction, - }); - - for (const resourceAction of resourceActions) { - const fields = resourceAction.get('fields') as string[]; - const newFields = fields.filter((field) => field != fieldName); - - await this.app.db.getRepository('rolesResourcesActions').update({ - filterByTk: resourceAction.get('id') as number, - values: { - fields: newFields, + const resourceActions = await this.app.db.getRepository('rolesResourcesActions').find({ + filter: { + 'resource.name': collectionName, + 'fields.$anyOf': [fieldName], }, transaction: options.transaction, }); - } + + for (const resourceAction of resourceActions) { + const fields = resourceAction.get('fields') as string[]; + const newFields = fields.filter((field) => field != fieldName); + + await this.app.db.getRepository('rolesResourcesActions').update({ + filterByTk: resourceAction.get('id') as number, + values: { + fields: newFields, + }, + transaction: options.transaction, + }); + } + }); }); const writeRolesToACL = async (app, options) => { diff --git a/packages/plugins/collection-manager/src/server/__tests__/http-api/recreate.test.ts b/packages/plugins/collection-manager/src/server/__tests__/http-api/recreate.test.ts index 750d5f74d..f1ffff2e9 100644 --- a/packages/plugins/collection-manager/src/server/__tests__/http-api/recreate.test.ts +++ b/packages/plugins/collection-manager/src/server/__tests__/http-api/recreate.test.ts @@ -63,4 +63,35 @@ describe('recreate field', () => { expect(response.statusCode).toBe(200); }); + + it('should reset fields', async () => { + await agent.resource('collections').create({ + values: { + name: 'a1', + fields: [ + { + name: 'a', + type: 'string', + }, + ], + }, + }); + + expect(await app.db.getRepository('fields').count()).toBe(1); + const response = await agent.resource('collections').setFields({ + filterByTk: 'a1', + values: { + fields: [ + { + name: 'a', + type: 'bigInt', + }, + ], + }, + }); + + expect(response.statusCode).toBe(200); + + expect(await app.db.getRepository('fields').count()).toBe(1); + }); }); diff --git a/packages/plugins/collection-manager/src/server/resourcers/collections.ts b/packages/plugins/collection-manager/src/server/resourcers/collections.ts index 78b954dfc..66bc2749d 100644 --- a/packages/plugins/collection-manager/src/server/resourcers/collections.ts +++ b/packages/plugins/collection-manager/src/server/resourcers/collections.ts @@ -21,14 +21,50 @@ export default { transaction, }); - await db.getRepository('collections').update({ - filterByTk, - values: { - fields, - }, + const existFields = await collection.getFields({ transaction, }); + const needUpdateFields = fields + .filter((f) => { + return existFields.find((ef) => ef.name === f.name); + }) + .map((f) => { + return { + ...f, + key: existFields.find((ef) => ef.name === f.name).key, + }; + }); + + const needDestroyFields = existFields.filter((ef) => { + return !fields.find((f) => f.name === ef.name); + }); + + const needCreatedFields = fields.filter((f) => { + return !existFields.find((ef) => ef.name === f.name); + }); + + if (needDestroyFields.length) { + await db.getRepository('fields').destroy({ + filterByTk: needDestroyFields.map((f) => f.key), + transaction, + }); + } + + if (needUpdateFields.length) { + await db.getRepository('fields').updateMany({ + records: needUpdateFields, + transaction, + }); + } + + if (needCreatedFields.length) { + await db.getRepository('collections.fields', filterByTk).create({ + values: needCreatedFields, + transaction, + }); + } + await collection.loadFields({ transaction, }); diff --git a/yarn.lock b/yarn.lock index 7d6b4e7f8..baa470c1c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -9003,6 +9003,13 @@ async-mutex@^0.3.2: dependencies: tslib "^2.3.1" +async-mutex@^0.4.0: + version "0.4.0" + resolved "https://registry.yarnpkg.com/async-mutex/-/async-mutex-0.4.0.tgz#ae8048cd4d04ace94347507504b3cf15e631c25f" + integrity sha512-eJFZ1YhRR8UN8eBLoNzcDPcy/jqjsg6I1AP+KvWQX80BqOSW1oJPJXDylPUEeMr2ZQvHgnQ//Lp6f3RQ1zI7HA== + dependencies: + tslib "^2.4.0" + async-ratelimiter@^1.3.0: version "1.3.8" resolved "https://registry.npmmirror.com/async-ratelimiter/-/async-ratelimiter-1.3.8.tgz#05198a322543de43d98807c96295a9d712306928"