From 56d1d1b85f048973e20c855be72f7636c13e2910 Mon Sep 17 00:00:00 2001 From: ChengLei Shao Date: Wed, 20 Sep 2023 11:53:56 +0800 Subject: [PATCH] feat(database): perform data validation before the update/create operation (#2681) * chore: test * chore: tmp commit * feat: value guard check --- .../src/__tests__/repository/update.test.ts | 148 +++++++++++++++++- packages/core/database/src/update-guard.ts | 39 +++++ 2 files changed, 185 insertions(+), 2 deletions(-) diff --git a/packages/core/database/src/__tests__/repository/update.test.ts b/packages/core/database/src/__tests__/repository/update.test.ts index d68d5fb5a..5241d8508 100644 --- a/packages/core/database/src/__tests__/repository/update.test.ts +++ b/packages/core/database/src/__tests__/repository/update.test.ts @@ -2,21 +2,58 @@ import { Collection, Database, mockDatabase } from '@nocobase/database'; describe('update', () => { let db: Database; - let User: Collection; + let Post: Collection; let Tag: Collection; + let PostTag: Collection; + afterEach(async () => { await db.close(); }); beforeEach(async () => { db = mockDatabase(); - User = db.collection({ + + await db.clean({ drop: true }); + PostTag = db.collection({ + name: 'post_tag', + fields: [ + { + type: 'bigInt', + name: 'id', + primaryKey: true, + autoIncrement: true, + }, + { + type: 'belongsTo', + name: 'post', + target: 'posts', + foreignKey: 'post_id', + }, + { + type: 'belongsTo', + name: 'tag', + target: 'tags', + foreignKey: 'tag_id', + }, + ], + }); + + Post = db.collection({ name: 'posts', fields: [ { type: 'string', name: 'title' }, { type: 'belongsToMany', name: 'tags', + through: 'post_tag', + foreignKey: 'post_id', + otherKey: 'tag_id', + }, + { + type: 'hasMany', + name: 'posts_tags', + foreignKey: 'post_id', + target: 'post_tag', }, ], }); @@ -31,6 +68,9 @@ describe('update', () => { { type: 'belongsToMany', name: 'posts', + through: 'post_tag', + foreignKey: 'tag_id', + otherKey: 'post_id', }, ], }); @@ -38,6 +78,110 @@ describe('update', () => { await db.sync(); }); + it('should throw error when update data conflicted', async () => { + const t1 = await Tag.repository.create({ + values: { + name: 't1', + }, + }); + const t2 = await Tag.repository.create({ + values: { + name: 't2', + }, + }); + const post1 = await Post.repository.create({ + values: { + title: 'p1', + tags: [t1.get('id')], + }, + }); + + const postTag = await PostTag.repository.findOne(); + + let error; + try { + await Post.repository.update({ + filterByTk: post1.get('id'), + values: { + posts_tags: [ + { + id: postTag.get('id'), + tag: t1.get('id'), + post: post1.get('id'), + }, + ], + tags: [t2.get('id')], + }, + }); + } catch (err) { + error = err; + } + expect(error).toBeDefined(); + + let error2; + try { + await Post.repository.update({ + filterByTk: post1.get('id'), + values: { + tags: [t2.get('id')], + posts_tags: [ + { + id: postTag.get('id'), + tag: t1.get('id'), + post: post1.get('id'), + }, + ], + }, + }); + } catch (err) { + error2 = err; + } + expect(error2).toBeDefined(); + + const User = db.collection({ + name: 'users', + fields: [ + { type: 'string', name: 'name' }, + { type: 'hasMany', name: 'posts', target: 'posts', foreignKey: 'user_id' }, + ], + }); + + await db.sync(); + const u1 = await User.repository.create({ + values: { + name: 'u1', + posts: [post1.get('id')], + }, + }); + + let error3; + + try { + await User.repository.update({ + filterByTk: u1.get('id'), + values: { + posts: [ + { + id: post1.get('id'), + tags: [t2.get('id')], + posts_tags: [ + { + id: postTag.get('id'), + tag: t1.get('id'), + post: post1.get('id'), + }, + ], + }, + ], + }, + }); + } catch (err) { + error3 = err; + } + + expect(error3).toBeDefined(); + }); + it('should update tags to null', async () => { await db.getRepository('posts').create({ values: { diff --git a/packages/core/database/src/update-guard.ts b/packages/core/database/src/update-guard.ts index a46f49c42..122c93b97 100644 --- a/packages/core/database/src/update-guard.ts +++ b/packages/core/database/src/update-guard.ts @@ -2,6 +2,7 @@ import lodash from 'lodash'; import { ModelStatic } from 'sequelize'; import { Model } from './model'; import { AssociationKeysToBeUpdate, BlackList, WhiteList } from './repository'; +import { isPlainObject } from '@nocobase/utils/src'; type UpdateValueItem = string | number | UpdateValues; @@ -58,6 +59,42 @@ export class UpdateGuard { this.blackList = blackList; } + checkValues(values) { + const dfs = (values, model) => { + const associations = model.associations; + const belongsToManyThroughNames = []; + + const associationValueKeys = Object.keys(associations).filter((key) => { + return Object.keys(values).includes(key); + }); + + const belongsToManyValueKeys = associationValueKeys.filter((key) => { + return associations[key].associationType === 'BelongsToMany'; + }); + + const hasManyValueKeys = associationValueKeys.filter((key) => { + return associations[key].associationType === 'HasMany'; + }); + + for (const belongsToManyKey of belongsToManyValueKeys) { + const association = associations[belongsToManyKey]; + const through = association.through.model; + belongsToManyThroughNames.push(through.name); + } + + for (const hasManyKey of hasManyValueKeys) { + const association = associations[hasManyKey]; + if (belongsToManyThroughNames.includes(association.target.name)) { + throw new Error( + `HasMany association ${hasManyKey} cannot be used with BelongsToMany association ${association.target.name} with same through model`, + ); + } + } + }; + + dfs(values, this.model); + } + /** * Sanitize values by whitelist blacklist * @param values @@ -69,6 +106,8 @@ export class UpdateGuard { throw new Error('please set model first'); } + this.checkValues(values); + const associations = this.model.associations; const associationsValues = lodash.pick(values, Object.keys(associations));