From 3082a7d6f8cfaa7211f96e376a0b72bece3bd5b8 Mon Sep 17 00:00:00 2001 From: ChengLei Shao Date: Mon, 10 Oct 2022 15:34:00 +0800 Subject: [PATCH] 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 | 6 +- .../hasone-repository.test.ts | 1 + .../src/__tests__/repository/update.test.ts | 35 +++ .../decorators/must-have-filter-decorator.ts | 17 + .../{ => decorators}/transaction-decorator.ts | 0 .../multiple-relation-repository.ts | 2 +- .../relation-repository.ts | 2 +- packages/core/database/src/repository.ts | 6 +- .../plugins/acl/src/__tests__/acl.test.ts | 296 +++++++++--------- packages/plugins/acl/src/__tests__/prepare.ts | 1 - .../plugins/acl/src/__tests__/role.test.ts | 28 +- packages/plugins/acl/src/server.ts | 2 + .../__tests__/field-options/indexes.test.ts | 94 +++--- .../plugins/collection-manager/src/server.ts | 6 +- 14 files changed, 270 insertions(+), 226 deletions(-) create mode 100644 packages/core/database/src/decorators/must-have-filter-decorator.ts rename packages/core/database/src/{ => decorators}/transaction-decorator.ts (100%) diff --git a/packages/core/actions/src/actions/update.ts b/packages/core/actions/src/actions/update.ts index 77ddae7e2..d7f0bb34f 100644 --- a/packages/core/actions/src/actions/update.ts +++ b/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(); } diff --git a/packages/core/database/src/__tests__/relation-repository/hasone-repository.test.ts b/packages/core/database/src/__tests__/relation-repository/hasone-repository.test.ts index 8c710981f..569aaeac2 100644 --- a/packages/core/database/src/__tests__/relation-repository/hasone-repository.test.ts +++ b/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(); diff --git a/packages/core/database/src/__tests__/repository/update.test.ts b/packages/core/database/src/__tests__/repository/update.test.ts index d017f553e..d68d5fb5a 100644 --- a/packages/core/database/src/__tests__/repository/update.test.ts +++ b/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'); + }); }); diff --git a/packages/core/database/src/decorators/must-have-filter-decorator.ts b/packages/core/database/src/decorators/must-have-filter-decorator.ts new file mode 100644 index 000000000..a089ed2fa --- /dev/null +++ b/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; diff --git a/packages/core/database/src/transaction-decorator.ts b/packages/core/database/src/decorators/transaction-decorator.ts similarity index 100% rename from packages/core/database/src/transaction-decorator.ts rename to packages/core/database/src/decorators/transaction-decorator.ts diff --git a/packages/core/database/src/relation-repository/multiple-relation-repository.ts b/packages/core/database/src/relation-repository/multiple-relation-repository.ts index a055cc32b..f76477d05 100644 --- a/packages/core/database/src/relation-repository/multiple-relation-repository.ts +++ b/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'; diff --git a/packages/core/database/src/relation-repository/relation-repository.ts b/packages/core/database/src/relation-repository/relation-repository.ts index a092e4f23..3193692f4 100644 --- a/packages/core/database/src/relation-repository/relation-repository.ts +++ b/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'; diff --git a/packages/core/database/src/repository.ts b/packages/core/database/src/repository.ts index bc21dbf28..9812404cf 100644 --- a/packages/core/database/src/repository.ts +++ b/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 { 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'), - }), { type: 'bearer' }); + + adminAgent = app.agent().auth( + 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 - .resource('roles') - .update({ - resourceIndex: 'new', - values: { - strategy: { - actions: ['create'], - }, + await adminAgent.resource('roles').update({ + resourceIndex: 'new', + values: { + strategy: { + actions: ['create'], }, - }); + }, + forceUpdate: true, + }); expect( acl.can({ @@ -104,15 +107,13 @@ describe('acl', () => { }, }); - await adminAgent - .resource('roles.resources', 'new') - .create({ - values: { - name: 'c1', - usingActionsConfig: true, - actions: [], - }, - }); + await adminAgent.resource('roles.resources', 'new').create({ + values: { + name: 'c1', + usingActionsConfig: true, + actions: [], + }, + }); expect( acl.can({ @@ -150,39 +151,37 @@ describe('acl', () => { }); // create c1 published scope - const { body: { data: publishedScope } } = await adminAgent - .resource('rolesResourcesScopes') - .create({ - values: { - resourceName: 'c1', - name: 'published', - scope: { - published: true, - }, + const { + body: { data: publishedScope }, + } = await adminAgent.resource('rolesResourcesScopes').create({ + values: { + resourceName: 'c1', + name: 'published', + scope: { + published: true, }, - }); + }, + }); // await db.getRepository('rolesResourcesScopes').findOne(); // set admin resources - await adminAgent - .resource('roles.resources', 'new') - .create({ - values: { - name: 'c1', - usingActionsConfig: true, - actions: [ - { - name: 'create', - scope: publishedScope.id, - }, - { - name: 'view', - fields: ['title', 'age'], - }, - ], - }, - }); + await adminAgent.resource('roles.resources', 'new').create({ + values: { + name: 'c1', + usingActionsConfig: true, + actions: [ + { + name: 'create', + scope: publishedScope.id, + }, + { + name: 'view', + fields: ['title', 'age'], + }, + ], + }, + }); expect( acl.can({ @@ -215,32 +214,28 @@ describe('acl', () => { }); // revoke action - const response = await adminAgent - .resource('roles.resources', role.get('name')) - .list({ - appends: ['actions'], - }); + const response = await adminAgent.resource('roles.resources', role.get('name')).list({ + appends: ['actions'], + }); expect(response.statusCode).toEqual(200); const actions = response.body.data[0].actions; const collectionName = response.body.data[0].name; - await adminAgent - .resource('roles.resources', role.get('name')) - .update({ - filterByTk: collectionName, - values: { - name: 'c1', - usingActionsConfig: true, - actions: [ - { - name: 'view', - fields: ['title', 'age'], - }, - ], - }, - }); + await adminAgent.resource('roles.resources', role.get('name')).update({ + filterByTk: collectionName, + values: { + name: 'c1', + usingActionsConfig: true, + actions: [ + { + 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 - .resource('roles.resources') - .create({ - associatedIndex: 'new', - values: { - name: 'posts', - usingActionsConfig: true, - actions: [ - { - name: 'view', - fields: ['title'], - }, - ], - }, - }); + await adminAgent.resource('roles.resources').create({ + associatedIndex: 'new', + values: { + name: 'posts', + usingActionsConfig: true, + actions: [ + { + 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 - .resource('roles.resources') - .create({ - associatedIndex: 'new', - values: { - name: 'posts', - usingActionsConfig: true, - actions: [ - { - name: 'create', - }, - ], - }, - }); + await adminAgent.resource('roles.resources').create({ + associatedIndex: 'new', + values: { + name: 'posts', + usingActionsConfig: true, + actions: [ + { + name: 'create', + }, + ], + }, + }); expect( acl.can({ @@ -352,21 +343,19 @@ describe('acl', () => { action: 'create', }); - await adminAgent - .resource('roles.resources', 'new') - .update({ - filterByTk: ( - await db.getRepository('rolesResources').findOne({ - filter: { - name: 'posts', - roleName: 'new', - }, - }) - ).get('name') as string, - values: { - usingActionsConfig: false, - }, - }); + await adminAgent.resource('roles.resources', 'new').update({ + filterByTk: ( + await db.getRepository('rolesResources').findOne({ + filter: { + name: 'posts', + roleName: 'new', + }, + }) + ).get('name') as string, + values: { + usingActionsConfig: false, + }, + }); expect( acl.can({ @@ -376,21 +365,19 @@ describe('acl', () => { }), ).toBeNull(); - await adminAgent - .resource('roles.resources', 'new') - .update({ - filterByTk: ( - await db.getRepository('rolesResources').findOne({ - filter: { - name: 'posts', - roleName: 'new', - }, - }) - ).get('name') as string, - values: { - usingActionsConfig: true, - }, - }); + await adminAgent.resource('roles.resources', 'new').update({ + filterByTk: ( + await db.getRepository('rolesResources').findOne({ + filter: { + name: 'posts', + roleName: 'new', + }, + }) + ).get('name') as string, + 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 - .resource('roles.resources') - .create({ - associatedIndex: 'new', - values: { - name: 'posts', - usingActionsConfig: true, - actions: [ - { - name: 'view', - fields: ['title'], - }, - ], - }, - }); + await adminAgent.resource('roles.resources').create({ + associatedIndex: 'new', + values: { + name: 'posts', + usingActionsConfig: true, + actions: [ + { + 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({ - userId: user.get('id'), - }), { type: 'bearer' }); + const userAgent = app.agent().auth( + userPlugin.jwtService.sign({ + userId: user.get('id'), + }), + { type: 'bearer' }, + ); const schema = { 'x-uid': 'test', diff --git a/packages/plugins/acl/src/__tests__/prepare.ts b/packages/plugins/acl/src/__tests__/prepare.ts index 62f07f6b2..0bbf480cf 100644 --- a/packages/plugins/acl/src/__tests__/prepare.ts +++ b/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(); diff --git a/packages/plugins/acl/src/__tests__/role.test.ts b/packages/plugins/acl/src/__tests__/role.test.ts index 056ac7a16..a28a51019 100644 --- a/packages/plugins/acl/src/__tests__/role.test.ts +++ b/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({ - userId: admin.get('id'), - }), { type: 'bearer' }); + adminAgent = app.agent().auth( + userPlugin.jwtService.sign({ + 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 - .resource('roles') - .update({ - values: { - strategy: { - actions: ['create:all', 'view:own'], - }, + const response = await adminAgent.resource('roles').update({ + forceUpdate: true, + values: { + strategy: { + actions: ['create:all', 'view:own'], }, - }); + }, + }); expect(response.statusCode).toEqual(200); diff --git a/packages/plugins/acl/src/server.ts b/packages/plugins/acl/src/server.ts index 0b94c6a5d..6ab2ba0e8 100644 --- a/packages/plugins/acl/src/server.ts +++ b/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'); diff --git a/packages/plugins/collection-manager/src/__tests__/field-options/indexes.test.ts b/packages/plugins/collection-manager/src/__tests__/field-options/indexes.test.ts index f354728e7..f82659570 100644 --- a/packages/plugins/collection-manager/src/__tests__/field-options/indexes.test.ts +++ b/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 - .resource('collections') - .create({ - values: { - name: 'test1', - }, - }); + await agent.resource('collections').create({ + 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 - .resource('collections.fields', tableName) - .create({ - values: { - name: 'title', - type: 'string' - }, - }); + const field = await agent.resource('collections.fields', tableName).create({ + values: { + 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 - const field = await agent - .resource('collections.fields', tableName) - .create({ - values: { - name: 'title', - type: 'string', - unique: true - }, - }); + // create a field with unique constraint + const field = await agent.resource('collections.fields', tableName).create({ + values: { + name: 'title', + 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); }); diff --git a/packages/plugins/collection-manager/src/server.ts b/packages/plugins/collection-manager/src/server.ts index 43a427014..e4ed074a7 100644 --- a/packages/plugins/collection-manager/src/server.ts +++ b/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, }); } });