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

@@ -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<SequelizeUpdateOptions, 'where'> {
   context?: any;
 }
 
-interface UpdateManyOptions extends UpdateOptions {
+interface UpdateManyOptions extends Omit<UpdateOptions, 'values'> {
   records: Values[];
 }
 
@@ -221,9 +219,7 @@ interface FirstOrCreateOptions extends Transactionable {
   values?: Values;
 }
 
-export class Repository<TModelAttributes extends {} = any, TCreationAttributes extends {} = TModelAttributes>
+export class Repository<TModelAttributes extends {} = any, TCreationAttributes extends {} = TModelAttributes> {
-  implements IRepository
-{
   database: Database;
   collection: Collection;
   model: ModelStatic<Model>;
@@ -242,7 +238,7 @@ export class Repository<TModelAttributes extends {} = any, TCreationAttributes e
       const chunks = key.split('.');
       return chunks
         .filter((chunk) => {
-          return !Boolean(chunk.match(/\d+/));
+          return !chunk.match(/\d+/);
         })
         .join('.');
     };

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

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

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');
+      await mutex.runExclusive(async () => {
-      const fieldName = model.get('name');
+        const collectionName = model.get('collectionName');
+        const fieldName = model.get('name');
 
-      const resourceActions = await this.app.db.getRepository('rolesResourcesActions').find({
+        const resourceActions = await this.app.db.getRepository('rolesResourcesActions').find({
-        filter: {
+          filter: {
-          'resource.name': collectionName,
+            'resource.name': collectionName,
-          'fields.$anyOf': [fieldName],
+            '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,
         });
-      }
+
+        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) => {

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

packages/plugins/collection-manager/src/server/resourcers/collections.ts

@@ -21,14 +21,50 @@ export default {
         transaction,
       });
 
-      await db.getRepository('collections').update({
+      const existFields = await collection.getFields({
-        filterByTk,
-        values: {
-          fields,
-        },
         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,
       });

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"