feat: update option must have filter or filterByTk (#847)

* feat: update option must have filter or filterByTk

* fix: typo

* fix: typo

(cherry picked from commit 83e6f93e1e3a188781530ec7492de1ed917a4dc1)

# Conflicts:
#	packages/plugins/acl/src/server.ts
#	packages/plugins/collection-manager/src/__tests__/field-options/indexes.test.ts

packages/core/actions/src/actions/update.ts

@@ -3,9 +3,9 @@ import { getRepositoryFromParams } from '../utils';
 
 export async function update(ctx: Context, next) {
   const repository = getRepositoryFromParams(ctx);
-  const { filterByTk, values, whitelist, blacklist, filter, updateAssociationValues } = ctx.action.params;
+  const { forceUpdate, filterByTk, values, whitelist, blacklist, filter, updateAssociationValues } = ctx.action.params;
 
-  const instance = await repository.update({
+  ctx.body = await repository.update({
     filterByTk,
     values,
     whitelist,
@@ -13,8 +13,8 @@ export async function update(ctx: Context, next) {
     filter,
     updateAssociationValues,
     context: ctx,
+    forceUpdate,
   });
 
-  ctx.body = instance;
   await next();
 }

packages/core/database/src/__tests__/relation-repository/hasone-repository.test.ts

@@ -87,6 +87,7 @@ describe('has one repository', () => {
         avatar: 'new_updated_avatar',
       },
     });
+
     expect((await UserProfileRepository.find())['avatar']).toEqual('new_updated_avatar');
     await UserProfileRepository.destroy();
     expect(await UserProfileRepository.find()).toBeNull();

packages/core/database/src/__tests__/repository/update.test.ts

@@ -57,4 +57,39 @@ describe('update', () => {
 
     expect(p1.toJSON()['tags']).toEqual([]);
   });
+
+  it('should not update items without filter or filterByPk', async () => {
+    await db.getRepository('posts').create({
+      values: {
+        title: 'p1',
+      },
+    });
+
+    let error;
+
+    try {
+      await db.getRepository('posts').update({
+        values: {
+          title: 'p3',
+        },
+      });
+    } catch (e) {
+      error = e;
+    }
+
+    expect(error).not.toBeUndefined();
+
+    const p1 = await db.getRepository('posts').findOne();
+    expect(p1.toJSON()['title']).toEqual('p1');
+
+    await db.getRepository('posts').update({
+      values: {
+        title: 'p3',
+      },
+      filterByTk: p1.get('id') as number,
+    });
+
+    await p1.reload();
+    expect(p1.toJSON()['title']).toEqual('p3');
+  });
 });

packages/core/database/src/decorators/must-have-filter-decorator.ts

@@ -0,0 +1,17 @@
+const mustHaveFilter = () => (target: any, propertyKey: string, descriptor: PropertyDescriptor) => {
+  const oldValue = descriptor.value;
+
+  descriptor.value = function () {
+    const options = arguments[0];
+
+    if (!options?.filter && !options?.filterByTk && !options?.forceUpdate) {
+      throw new Error(`must provide filter or filterByTk for ${propertyKey} call, or set forceUpdate to true`);
+    }
+
+    return oldValue.apply(this, arguments);
+  };
+
+  return descriptor;
+};
+
+export default mustHaveFilter;

packages/core/database/src/relation-repository/multiple-relation-repository.ts

@@ -9,7 +9,7 @@ import {
   FindOptions,
   TargetKey,
   TK,
-  UpdateOptions
+  UpdateOptions,
 } from '../repository';
 import { updateModelByValues } from '../update-associations';
 import { UpdateGuard } from '../update-guard';

packages/core/database/src/relation-repository/relation-repository.ts

@@ -7,7 +7,7 @@ import FilterParser from '../filter-parser';
 import { Model } from '../model';
 import { OptionsParser } from '../options-parser';
 import { CreateOptions, Filter, FindOptions } from '../repository';
-import { transactionWrapperBuilder } from '../transaction-decorator';
+import { transactionWrapperBuilder } from '../decorators/transaction-decorator';
 import { updateAssociations } from '../update-associations';
 import { UpdateGuard } from '../update-guard';
 

packages/core/database/src/repository.ts

@@ -13,6 +13,8 @@ import {
 } from 'sequelize';
 import { Collection } from './collection';
 import { Database } from './database';
+import mustHaveFilter from './decorators/must-have-filter-decorator';
+import { transactionWrapperBuilder } from './decorators/transaction-decorator';
 import { RelationField } from './fields';
 import FilterParser from './filter-parser';
 import { Model } from './model';
@@ -22,7 +24,6 @@ import { BelongsToRepository } from './relation-repository/belongs-to-repository
 import { HasManyRepository } from './relation-repository/hasmany-repository';
 import { HasOneRepository } from './relation-repository/hasone-repository';
 import { RelationRepository } from './relation-repository/relation-repository';
-import { transactionWrapperBuilder } from './transaction-decorator';
 import { updateAssociations, updateModelByValues } from './update-associations';
 import { UpdateGuard } from './update-guard';
 
@@ -345,7 +346,8 @@ export class Repository<TModelAttributes extends {} = any, TCreationAttributes e
    * @param options
    */
   @transaction()
-  async update(options: UpdateOptions) {
+  @mustHaveFilter()
+  async update(options: UpdateOptions & { forceUpdate?: boolean }) {
     const transaction = await this.getTransaction(options);
     const guard = UpdateGuard.fromOptions(this.model, options);
 

packages/plugins/acl/src/__tests__/acl.test.ts

@@ -26,14 +26,18 @@ describe('acl', () => {
     const UserRepo = db.getCollection('users').repository;
     admin = await UserRepo.create({
       values: {
-        roles: ['admin']
+        roles: ['admin'],
-      }
+      },
     });
 
     const userPlugin = app.getPlugin('@nocobase/plugin-users') as UsersPlugin;
-    adminAgent = app.agent().auth(userPlugin.jwtService.sign({
+
-      userId: admin.get('id'),
+    adminAgent = app.agent().auth(
-    }), { type: 'bearer' });
+      userPlugin.jwtService.sign({
+        userId: admin.get('id'),
+      }),
+      { type: 'bearer' },
+    );
 
     uiSchemaRepository = db.getRepository('uiSchemas');
   });
@@ -41,7 +45,7 @@ describe('acl', () => {
   it('should works with universal actions', async () => {
     await db.getRepository('roles').create({
       values: {
-        name: 'new'
+        name: 'new',
       },
     });
 
@@ -54,16 +58,15 @@ describe('acl', () => {
     ).toBeNull();
 
     // grant universal action
-    await adminAgent
+    await adminAgent.resource('roles').update({
-      .resource('roles')
+      resourceIndex: 'new',
-      .update({
+      values: {
-        resourceIndex: 'new',
+        strategy: {
-        values: {
+          actions: ['create'],
-          strategy: {
-            actions: ['create'],
-          },
         },
-      });
+      },
+      forceUpdate: true,
+    });
 
     expect(
       acl.can({
@@ -104,15 +107,13 @@ describe('acl', () => {
       },
     });
 
-    await adminAgent
+    await adminAgent.resource('roles.resources', 'new').create({
-      .resource('roles.resources', 'new')
+      values: {
-      .create({
+        name: 'c1',
-        values: {
+        usingActionsConfig: true,
-          name: 'c1',
+        actions: [],
-          usingActionsConfig: true,
+      },
-          actions: [],
+    });
-        },
-      });
 
     expect(
       acl.can({
@@ -150,39 +151,37 @@ describe('acl', () => {
     });
 
     // create c1 published scope
-    const { body: { data: publishedScope } } = await adminAgent
+    const {
-      .resource('rolesResourcesScopes')
+      body: { data: publishedScope },
-      .create({
+    } = await adminAgent.resource('rolesResourcesScopes').create({
-        values: {
+      values: {
-          resourceName: 'c1',
+        resourceName: 'c1',
-          name: 'published',
+        name: 'published',
-          scope: {
+        scope: {
-            published: true,
+          published: true,
-          },
         },
-      });
+      },
+    });
 
     // await db.getRepository('rolesResourcesScopes').findOne();
 
     // set admin resources
-    await adminAgent
+    await adminAgent.resource('roles.resources', 'new').create({
-      .resource('roles.resources', 'new')
+      values: {
-      .create({
+        name: 'c1',
-        values: {
+        usingActionsConfig: true,
-          name: 'c1',
+        actions: [
-          usingActionsConfig: true,
+          {
-          actions: [
+            name: 'create',
-            {
+            scope: publishedScope.id,
-              name: 'create',
+          },
-              scope: publishedScope.id,
+          {
-            },
+            name: 'view',
-            {
+            fields: ['title', 'age'],
-              name: 'view',
+          },
-              fields: ['title', 'age'],
+        ],
-            },
+      },
-          ],
+    });
-        },
-      });
 
     expect(
       acl.can({
@@ -215,32 +214,28 @@ describe('acl', () => {
     });
 
     // revoke action
-    const response = await adminAgent
+    const response = await adminAgent.resource('roles.resources', role.get('name')).list({
-      .resource('roles.resources', role.get('name'))
+      appends: ['actions'],
-      .list({
+    });
-        appends: ['actions'],
-      });
 
     expect(response.statusCode).toEqual(200);
 
     const actions = response.body.data[0].actions;
     const collectionName = response.body.data[0].name;
 
-    await adminAgent
+    await adminAgent.resource('roles.resources', role.get('name')).update({
-      .resource('roles.resources', role.get('name'))
+      filterByTk: collectionName,
-      .update({
+      values: {
-        filterByTk: collectionName,
+        name: 'c1',
-        values: {
+        usingActionsConfig: true,
-          name: 'c1',
+        actions: [
-          usingActionsConfig: true,
+          {
-          actions: [
+            name: 'view',
-            {
+            fields: ['title', 'age'],
-              name: 'view',
+          },
-              fields: ['title', 'age'],
+        ],
-            },
+      },
-          ],
+    });
-        },
-      });
 
     expect(
       acl.can({
@@ -254,7 +249,7 @@ describe('acl', () => {
   it('should revoke resource when collection destroy', async () => {
     await db.getRepository('roles').create({
       values: {
-        name: 'new'
+        name: 'new',
       },
     });
 
@@ -272,21 +267,19 @@ describe('acl', () => {
       },
     });
 
-    await adminAgent
+    await adminAgent.resource('roles.resources').create({
-      .resource('roles.resources')
+      associatedIndex: 'new',
-      .create({
+      values: {
-        associatedIndex: 'new',
+        name: 'posts',
-        values: {
+        usingActionsConfig: true,
-          name: 'posts',
+        actions: [
-          usingActionsConfig: true,
+          {
-          actions: [
+            name: 'view',
-            {
+            fields: ['title'],
-              name: 'view',
+          },
-              fields: ['title'],
+        ],
-            },
+      },
-          ],
+    });
-        },
-      });
 
     expect(
       acl.can({
@@ -314,7 +307,7 @@ describe('acl', () => {
   it('should revoke actions when not using actions config', async () => {
     await db.getRepository('roles').create({
       values: {
-        name: 'new'
+        name: 'new',
       },
     });
 
@@ -325,20 +318,18 @@ describe('acl', () => {
       },
     });
 
-    await adminAgent
+    await adminAgent.resource('roles.resources').create({
-      .resource('roles.resources')
+      associatedIndex: 'new',
-      .create({
+      values: {
-        associatedIndex: 'new',
+        name: 'posts',
-        values: {
+        usingActionsConfig: true,
-          name: 'posts',
+        actions: [
-          usingActionsConfig: true,
+          {
-          actions: [
+            name: 'create',
-            {
+          },
-              name: 'create',
+        ],
-            },
+      },
-          ],
+    });
-        },
-      });
 
     expect(
       acl.can({
@@ -352,21 +343,19 @@ describe('acl', () => {
       action: 'create',
     });
 
-    await adminAgent
+    await adminAgent.resource('roles.resources', 'new').update({
-      .resource('roles.resources', 'new')
+      filterByTk: (
-      .update({
+        await db.getRepository('rolesResources').findOne({
-        filterByTk: (
+          filter: {
-          await db.getRepository('rolesResources').findOne({
+            name: 'posts',
-            filter: {
+            roleName: 'new',
-              name: 'posts',
+          },
-              roleName: 'new',
+        })
-            },
+      ).get('name') as string,
-          })
+      values: {
-        ).get('name') as string,
+        usingActionsConfig: false,
-        values: {
+      },
-          usingActionsConfig: false,
+    });
-        },
-      });
 
     expect(
       acl.can({
@@ -376,21 +365,19 @@ describe('acl', () => {
       }),
     ).toBeNull();
 
-    await adminAgent
+    await adminAgent.resource('roles.resources', 'new').update({
-      .resource('roles.resources', 'new')
+      filterByTk: (
-      .update({
+        await db.getRepository('rolesResources').findOne({
-        filterByTk: (
+          filter: {
-          await db.getRepository('rolesResources').findOne({
+            name: 'posts',
-            filter: {
+            roleName: 'new',
-              name: 'posts',
+          },
-              roleName: 'new',
+        })
-            },
+      ).get('name') as string,
-          })
+      values: {
-        ).get('name') as string,
+        usingActionsConfig: true,
-        values: {
+      },
-          usingActionsConfig: true,
+    });
-        },
-      });
 
     expect(
       acl.can({
@@ -408,7 +395,7 @@ describe('acl', () => {
   it('should add fields when field created', async () => {
     await db.getRepository('roles').create({
       values: {
-        name: 'new'
+        name: 'new',
       },
     });
 
@@ -426,21 +413,19 @@ describe('acl', () => {
       },
     });
 
-    await adminAgent
+    await adminAgent.resource('roles.resources').create({
-      .resource('roles.resources')
+      associatedIndex: 'new',
-      .create({
+      values: {
-        associatedIndex: 'new',
+        name: 'posts',
-        values: {
+        usingActionsConfig: true,
-          name: 'posts',
+        actions: [
-          usingActionsConfig: true,
+          {
-          actions: [
+            name: 'view',
-            {
+            fields: ['title'],
-              name: 'view',
+          },
-              fields: ['title'],
+        ],
-            },
+      },
-          ],
+    });
-        },
-      });
 
     const allowFields = acl.can({
       role: 'new',
@@ -494,14 +479,17 @@ describe('acl', () => {
     const UserRepo = db.getCollection('users').repository;
     const user = await UserRepo.create({
       values: {
-        roles: ['new']
+        roles: ['new'],
-      }
+      },
     });
 
     const userPlugin = app.getPlugin('@nocobase/plugin-users') as UsersPlugin;
-    const userAgent = app.agent().auth(userPlugin.jwtService.sign({
+    const userAgent = app.agent().auth(
-      userId: user.get('id'),
+      userPlugin.jwtService.sign({
-    }), { type: 'bearer' });
+        userId: user.get('id'),
+      }),
+      { type: 'bearer' },
+    );
 
     const schema = {
       'x-uid': 'test',

packages/plugins/acl/src/__tests__/prepare.ts

@@ -17,7 +17,6 @@ export async function prepareApp() {
   app.plugin(PluginUiSchema);
   app.plugin(PluginErrorHandler);
   app.plugin(PluginCollectionManager);
-
   app.plugin(PluginACL);
   await app.loadAndInstall();
 

packages/plugins/acl/src/__tests__/role.test.ts

@@ -32,14 +32,17 @@ describe('role api', () => {
       const UserRepo = db.getCollection('users').repository;
       admin = await UserRepo.create({
         values: {
-          roles: ['admin']
+          roles: ['admin'],
-        }
+        },
       });
 
       const userPlugin = app.getPlugin('@nocobase/plugin-users') as UsersPlugin;
-      adminAgent = app.agent().auth(userPlugin.jwtService.sign({
+      adminAgent = app.agent().auth(
-        userId: admin.get('id'),
+        userPlugin.jwtService.sign({
-      }), { type: 'bearer' });
+          userId: admin.get('id'),
+        }),
+        { type: 'bearer' },
+      );
     });
 
     it('should list actions', async () => {
@@ -49,15 +52,14 @@ describe('role api', () => {
 
     it('should grant universal role actions', async () => {
       // grant role actions
-      const response = await adminAgent
+      const response = await adminAgent.resource('roles').update({
-        .resource('roles')
+        forceUpdate: true,
-        .update({
+        values: {
-          values: {
+          strategy: {
-            strategy: {
+            actions: ['create:all', 'view:own'],
-              actions: ['create:all', 'view:own'],
-            },
           },
-        });
+        },
+      });
 
       expect(response.statusCode).toEqual(200);
 

packages/plugins/acl/src/server.ts

@@ -416,10 +416,12 @@ export class PluginACL extends Plugin {
     }
 
     const User = this.db.getCollection('users');
+
     await User.repository.update({
       values: {
         roles: ['root', 'admin', 'member'],
       },
+      forceUpdate: true,
     });
 
     const RolesUsers = this.db.getCollection('rolesUsers');

packages/plugins/collection-manager/src/__tests__/field-options/indexes.test.ts

@@ -8,73 +8,70 @@ describe('field indexes', () => {
   beforeEach(async () => {
     app = await createApp();
     agent = app.agent();
-    await agent
+    await agent.resource('collections').create({
-      .resource('collections')
+      values: {
-      .create({
+        name: 'test1',
-        values: {
+      },
-          name: 'test1',
+    });
-        },
-      });
   });
 
   afterEach(async () => {
     await app.destroy();
   });
 
-  it.only('create unique constraint after added dulplicated records', async () => {
+  it('create unique constraint after added duplicated records', async () => {
     const tableName = 'test1';
     // create an field with unique constraint
-    const field = await agent
+    const field = await agent.resource('collections.fields', tableName).create({
-      .resource('collections.fields', tableName)
+      values: {
-      .create({
+        name: 'title',
-        values: {
+        type: 'string',
-          name: 'title',
+      },
-          type: 'string'
+    });
-        },
-      });
 
     // create a record
     const response1 = await agent.resource(tableName).create({
-      values: { title: 't1' }
+      values: { title: 't1' },
     });
+
     // create another same record
     const response2 = await agent.resource(tableName).create({
-      values: { title: 't1' }
+      values: { title: 't1' },
     });
 
     const response3 = await agent.resource('fields').update({
-      filterByTk: field.id,
+      filterByTk: field.body.data.key,
       values: {
-        unique: true
+        unique: true,
-      }
+      },
     });
+
     expect(response3.status).toBe(400);
 
     const response4 = await agent.resource(tableName).create({
-      values: { title: 't1' }
+      values: { title: 't1' },
     });
+
     expect(response4.status).toBe(200);
     expect(response4.body.data.title).toBe('t1');
   });
 
   it('field value cannot be duplicated with unique index', async () => {
     const tableName = 'test1';
-    // create an field with unique constraint
+    // create a field with unique constraint
-    const field = await agent
+    const field = await agent.resource('collections.fields', tableName).create({
-      .resource('collections.fields', tableName)
+      values: {
-      .create({
+        name: 'title',
-        values: {
+        type: 'string',
-          name: 'title',
+        unique: true,
-          type: 'string',
+      },
-          unique: true
+    });
-        },
-      });
 
     // create a record
     const response1 = await agent.resource(tableName).create({
       values: {
-        title: 't1'
+        title: 't1',
-      }
+      },
     });
     expect(response1.status).toBe(200);
     expect(response1.body.data.title).toBe('t1');
@@ -82,48 +79,49 @@ describe('field indexes', () => {
     // create another record with the same value on unique field should fail
     const response2 = await agent.resource(tableName).create({
       values: {
-        title: 't1'
+        title: 't1',
-      }
+      },
     });
     expect(response2.status).toBe(400);
 
     // update field to remove unique constraint
     await agent.resource('fields').update({
-      filterByTk: field.id,
+      filterByTk: field.body.data.key,
       values: {
-        unique: false
+        unique: false,
-      }
+      },
     });
 
     // create another record with the same value on unique field should be ok
     const response3 = await agent.resource(tableName).create({
       values: {
-        title: 't1'
+        title: 't1',
-      }
+      },
     });
     expect(response3.status).toBe(200);
     expect(response3.body.data.title).toBe('t1');
 
     // update field to add unique constraint should fail because of duplicated records
     const response4 = await agent.resource('fields').update({
-      filterByTk: field.id,
+      filterByTk: field.body.data.key,
       values: {
-        unique: true
+        unique: true,
-      }
+      },
     });
+
     expect(response4.status).toBe(400);
 
     // remove a duplicated record
     await agent.resource(tableName).destroy({
-      filterByTk: response3.body.data.id
+      filterByTk: response3.body.data.id,
     });
 
     // update field to add unique constraint should be ok
     const response6 = await agent.resource('fields').update({
-      filterByTk: field.id,
+      filterByTk: field.body.data.key,
       values: {
-        unique: true
+        unique: true,
-      }
+      },
     });
     expect(response6.status).toBe(200);
   });

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

@@ -11,7 +11,7 @@ import {
   afterCreateForReverseField,
   beforeCreateForChildrenCollection,
   beforeCreateForReverseField,
-  beforeInitOptions
+  beforeInitOptions,
 } from './hooks';
 import { CollectionModel, FieldModel } from './models';
 
@@ -67,7 +67,7 @@ export class CollectionManagerPlugin extends Plugin {
       if (context) {
         await model.migrate({
           isNew: true,
-          transaction
+          transaction,
         });
       }
     });
@@ -76,7 +76,7 @@ export class CollectionManagerPlugin extends Plugin {
       if (context) {
         await model.migrate({
           isNew: true,
-          transaction
+          transaction,
         });
       }
     });