diff --git a/packages/agent/src/routes/capabilities.ts b/packages/agent/src/routes/capabilities.ts index 7817383b0b..390a19539d 100644 --- a/packages/agent/src/routes/capabilities.ts +++ b/packages/agent/src/routes/capabilities.ts @@ -61,6 +61,7 @@ export default class Capabilities extends BaseRoute { // front offers oldest first on the project page only when it is set. canSortAuditTrailTimeline: Boolean(this.options.auditTrail?.store?.listTimeline), checksRelationReadPermissions: !this.options.skipRelationReadPermissions, + canUseNestedConditionTree: true, }, collections: collections?.map(collection => { diff --git a/packages/agent/src/services/authorization/authorization.ts b/packages/agent/src/services/authorization/authorization.ts index f14bfad821..870e221072 100644 --- a/packages/agent/src/services/authorization/authorization.ts +++ b/packages/agent/src/services/authorization/authorization.ts @@ -281,7 +281,11 @@ export default class AuthorizationService { if (!scope) return null; - return ConditionTreeParser.fromPlainObject(collection, scope); + try { + return ConditionTreeParser.fromPlainObject(collection, scope); + } catch (error) { + throw new UnprocessableError(`Invalid scope (${error.message})`); + } } public invalidateScopeCache(renderingId: number | string) { diff --git a/packages/agent/src/utils/condition-tree-parser.ts b/packages/agent/src/utils/condition-tree-parser.ts index 406be45b51..18a5026897 100644 --- a/packages/agent/src/utils/condition-tree-parser.ts +++ b/packages/agent/src/utils/condition-tree-parser.ts @@ -25,6 +25,10 @@ const STRING_TO_BOOLEAN = { export default class ConditionTreeParser { static fromPlainObject(collection: Collection, json: unknown): ConditionTree { + return ConditionTreeParser.parse(collection, json, true); + } + + private static parse(collection: Collection, json: unknown, isRoot: boolean): ConditionTree { if (ConditionTreeParser.isLeaf(json)) { const operator = ConditionTreeParser.toPascalCase(json.operator); const value = ConditionTreeParser.parseValue(collection, { ...json, operator }); @@ -33,9 +37,15 @@ export default class ConditionTreeParser { } if (ConditionTreeParser.isBranch(json)) { + // An empty nested Or would match nothing and an empty nested And everything: dropping it + // widens the filter, keeping it silently empties the result. Either is wrong under a bulk action. + if (!isRoot && json.conditions.length === 0) { + throw new Error('A nested group of conditions cannot be empty'); + } + const aggregator = ConditionTreeParser.toPascalCase(json.aggregator) as Aggregator; const conditions = json.conditions.map(subTree => - ConditionTreeParser.fromPlainObject(collection, subTree), + ConditionTreeParser.parse(collection, subTree, false), ); return conditions.length !== 1 @@ -115,10 +125,15 @@ export default class ConditionTreeParser { } private static isLeaf(raw: unknown): raw is { field: string; operator: string; value: unknown } { - return typeof raw === 'object' && 'field' in raw && 'operator' in raw; + return typeof raw === 'object' && raw !== null && 'field' in raw && 'operator' in raw; } private static isBranch(raw: unknown): raw is { aggregator: string; conditions: unknown[] } { - return typeof raw === 'object' && 'aggregator' in raw && 'conditions' in raw; + return ( + typeof raw === 'object' && + raw !== null && + 'aggregator' in raw && + Array.isArray((raw as { conditions?: unknown }).conditions) + ); } } diff --git a/packages/agent/test/routes/capabilities.test.ts b/packages/agent/test/routes/capabilities.test.ts index 0d03d88581..797441a2bc 100644 --- a/packages/agent/test/routes/capabilities.test.ts +++ b/packages/agent/test/routes/capabilities.test.ts @@ -109,6 +109,7 @@ describe('Capabilities', () => { canUseAuditTrailTimeline: false, canSortAuditTrailTimeline: false, checksRelationReadPermissions: true, + canUseNestedConditionTree: true, }, collections: [], }); @@ -137,6 +138,7 @@ describe('Capabilities', () => { canUseAuditTrailTimeline: false, canSortAuditTrailTimeline: false, checksRelationReadPermissions: true, + canUseNestedConditionTree: true, }, collections: [], }); @@ -176,6 +178,7 @@ describe('Capabilities', () => { canUseAuditTrailTimeline: false, canSortAuditTrailTimeline: false, checksRelationReadPermissions: true, + canUseNestedConditionTree: true, }, collections: [], }); @@ -278,6 +281,7 @@ describe('Capabilities', () => { canUseAuditTrailTimeline: false, canSortAuditTrailTimeline: false, checksRelationReadPermissions: true, + canUseNestedConditionTree: true, }, collections: [ { diff --git a/packages/agent/test/services/authorization/authorization.test.ts b/packages/agent/test/services/authorization/authorization.test.ts index 30ebb5a815..cbe6851947 100644 --- a/packages/agent/test/services/authorization/authorization.test.ts +++ b/packages/agent/test/services/authorization/authorization.test.ts @@ -1,6 +1,6 @@ import type { Context } from 'koa'; -import { Projection } from '@forestadmin/datasource-toolkit'; +import { Projection, UnprocessableError } from '@forestadmin/datasource-toolkit'; import { ChainedSQLQueryError, ChartType, @@ -205,6 +205,23 @@ describe('AuthorizationService', () => { }); expect(ConditionTreeParser.fromPlainObject).toHaveBeenCalledWith(collection, { foo: 'bar' }); }); + + it('should throw an UnprocessableError when the scope cannot be parsed', async () => { + const forestAdminClient = factories.forestAdminClient.build(); + const authorizationService = new AuthorizationService(forestAdminClient); + const context = { state: { user: { id: 666, renderingId: 42 } } } as Context; + + (forestAdminClient.getScope as jest.Mock).mockResolvedValue({ foo: 'bar' }); + (ConditionTreeParser.fromPlainObject as jest.Mock).mockImplementation(() => { + throw new Error('Failed to instantiate condition tree from json'); + }); + + await expect( + authorizationService.getScope(factories.collection.build({ name: 'books' }), context), + ).rejects.toThrow( + new UnprocessableError('Invalid scope (Failed to instantiate condition tree from json)'), + ); + }); }); describe('assertCanExecuteChart', () => { diff --git a/packages/agent/test/utils/condition-tree-parser.test.ts b/packages/agent/test/utils/condition-tree-parser.test.ts index 879b70f32c..0eab9f6a20 100644 --- a/packages/agent/test/utils/condition-tree-parser.test.ts +++ b/packages/agent/test/utils/condition-tree-parser.test.ts @@ -38,6 +38,91 @@ describe('ConditionTreeParser', () => { ); }); + test('should keep a group of And nested in an Or', () => { + const tree = ConditionTreeParser.fromPlainObject(collection, { + aggregator: 'or', + conditions: [ + { + aggregator: 'and', + conditions: [ + { field: 'number', operator: 'greater_than', value: '1' }, + { field: 'boolean', operator: 'equal', value: 'true' }, + ], + }, + { field: 'string', operator: 'equal', value: 'a' }, + ], + }); + + expect(tree).toStrictEqual( + new ConditionTreeBranch('Or', [ + new ConditionTreeBranch('And', [ + new ConditionTreeLeaf('number', 'GreaterThan', 1), + new ConditionTreeLeaf('boolean', 'Equal', true), + ]), + new ConditionTreeLeaf('string', 'Equal', 'a'), + ]), + ); + }); + + test('should unwrap a nested group holding a single condition', () => { + const tree = ConditionTreeParser.fromPlainObject(collection, { + aggregator: 'and', + conditions: [ + { aggregator: 'or', conditions: [{ field: 'string', operator: 'equal', value: 'a' }] }, + { field: 'number', operator: 'equal', value: 2 }, + ], + }); + + expect(tree).toStrictEqual( + new ConditionTreeBranch('And', [ + new ConditionTreeLeaf('string', 'Equal', 'a'), + new ConditionTreeLeaf('number', 'Equal', 2), + ]), + ); + }); + + test.each(['and', 'or'])('should reject an empty nested %s group', aggregator => { + expect(() => + ConditionTreeParser.fromPlainObject(collection, { + aggregator: 'and', + conditions: [ + { field: 'string', operator: 'equal', value: 'a' }, + { aggregator, conditions: [] }, + ], + }), + ).toThrow('A nested group of conditions cannot be empty'); + }); + + test('should reject an empty group nested deeper than one level', () => { + expect(() => + ConditionTreeParser.fromPlainObject(collection, { + aggregator: 'or', + conditions: [ + { field: 'string', operator: 'equal', value: 'a' }, + { aggregator: 'and', conditions: [{ aggregator: 'or', conditions: [] }] }, + ], + }), + ).toThrow('A nested group of conditions cannot be empty'); + }); + + test.each([ + ['a null condition', { aggregator: 'and', conditions: [null] }], + ['conditions that are not an array', { aggregator: 'or', conditions: 'x' }], + ])('should reject a group holding %s', (_, json) => { + expect(() => ConditionTreeParser.fromPlainObject(collection, json)).toThrow( + 'Failed to instantiate condition tree from json', + ); + }); + + test('should keep an empty root group as an empty branch', () => { + const tree = ConditionTreeParser.fromPlainObject(collection, { + aggregator: 'and', + conditions: [], + }); + + expect(tree).toStrictEqual(new ConditionTreeBranch('And', [])); + }); + test('should work with snake case', () => { const tree = ConditionTreeParser.fromPlainObject(collection, { field: 'id', diff --git a/packages/agent/test/utils/query-string.test.ts b/packages/agent/test/utils/query-string.test.ts index 44bd13b988..aa2ede5a8f 100644 --- a/packages/agent/test/utils/query-string.test.ts +++ b/packages/agent/test/utils/query-string.test.ts @@ -109,6 +109,32 @@ describe('QueryStringParser', () => { expect(fn).toThrow(ValidationError); }); + + test('should reject a bulk action selection holding an empty nested group', () => { + const context = createMockContext({ + requestBody: { + data: { + attributes: { + all_records_subset_query: { + filters: JSON.stringify({ + aggregator: 'and', + conditions: [ + { field: 'name', operator: 'equal', value: 'Foundation' }, + { aggregator: 'or', conditions: [] }, + ], + }), + }, + }, + }, + }, + }); + + const fn = () => QueryStringParser.parseConditionTree(collectionSimple, context); + + expect(fn).toThrow( + new ValidationError('Invalid filters (A nested group of conditions cannot be empty)'), + ); + }); }); describe('parseProjection', () => { diff --git a/packages/datasource-mongoose/src/utils/pipeline/filter.ts b/packages/datasource-mongoose/src/utils/pipeline/filter.ts index d3b897c627..f4718ee0ef 100644 --- a/packages/datasource-mongoose/src/utils/pipeline/filter.ts +++ b/packages/datasource-mongoose/src/utils/pipeline/filter.ts @@ -109,6 +109,13 @@ export default class FilterGenerator { const schema = MongooseSchema.fromModel(model).applyStack(stack, true); if (tree instanceof ConditionTreeBranch) { + // MongoDB refuses `$and: []` and `$or: []`; the toolkit's MatchNone is an empty Or. + if (tree.conditions.length === 0 && tree.aggregator === 'Or') { + return { $expr: { $literal: false } }; + } + + if (tree.conditions.length === 0 && tree.aggregator === 'And') return {}; + return { [`$${tree.aggregator.toLowerCase()}`]: tree.conditions.map(condition => this.computeMatch(model, stack, condition, fields), diff --git a/packages/datasource-mongoose/test/integration/review/collection_list.test.ts b/packages/datasource-mongoose/test/integration/review/collection_list.test.ts index 7f88adda4a..29badda198 100644 --- a/packages/datasource-mongoose/test/integration/review/collection_list.test.ts +++ b/packages/datasource-mongoose/test/integration/review/collection_list.test.ts @@ -4,7 +4,9 @@ import type { Aggregator, ConditionTreeLeaf, Operator } from '@forestadmin/datas import type { Connection } from 'mongoose'; import { + ConditionTreeBranch as Branch, ConditionTreeEquivalent, + ConditionTreeFactory, ConditionTreeLeaf as Leaf, Page, PaginatedFilter, @@ -388,6 +390,85 @@ describe('MongooseCollection', () => { ).rejects.toThrow('unknown top level operator: $bad.'); }); + describe('with branches', () => { + const setupRecords = async () => { + connection = await setupReview('collection_review_list'); + const review = new MongooseDatasource(connection).getCollection('review'); + await review.create(factories.caller.build(), [ + { message: 'A', rating: 1 }, + { message: 'B', rating: 2 }, + { message: 'C', rating: 3 }, + ]); + + return review; + }; + + it('returns nothing for MatchNone', async () => { + const review = await setupRecords(); + + const records = await review.list( + factories.caller.build(), + factories.filter.build({ conditionTree: ConditionTreeFactory.MatchNone }), + new Projection('message'), + ); + + expect(records).toEqual([]); + }); + + it('returns nothing for MatchNone intersected with another condition', async () => { + const review = await setupRecords(); + + const records = await review.list( + factories.caller.build(), + factories.filter.build({ + conditionTree: new Branch('And', [ + new Leaf('rating', 'GreaterThan', 0), + ConditionTreeFactory.MatchNone, + ]), + }), + new Projection('message'), + ); + + expect(records).toEqual([]); + }); + + it('returns every record for an empty And', async () => { + const review = await setupRecords(); + + const records = await review.list( + factories.caller.build(), + factories.filter.build({ + conditionTree: new Branch('And', []), + sort: new Sort({ field: 'message', ascending: true }), + }), + new Projection('message'), + ); + + expect(records).toEqual([{ message: 'A' }, { message: 'B' }, { message: 'C' }]); + }); + + it('applies an And group nested in an Or', async () => { + const review = await setupRecords(); + + const records = await review.list( + factories.caller.build(), + factories.filter.build({ + conditionTree: new Branch('Or', [ + new Branch('And', [ + new Leaf('rating', 'GreaterThan', 1), + new Leaf('message', 'Equal', 'B'), + ]), + new Leaf('message', 'Equal', 'A'), + ]), + sort: new Sort({ field: 'message', ascending: true }), + }), + new Projection('message'), + ); + + expect(records).toEqual([{ message: 'A' }, { message: 'B' }]); + }); + }); + it('supports default operators when the primary key objectId is given', async () => { connection = await setupReview('collection_review_list'); const dataSource = new MongooseDatasource(connection); diff --git a/packages/datasource-sequelize/test/integration/list/branch.integration.test.ts b/packages/datasource-sequelize/test/integration/list/branch.integration.test.ts new file mode 100644 index 0000000000..0c44c6535c --- /dev/null +++ b/packages/datasource-sequelize/test/integration/list/branch.integration.test.ts @@ -0,0 +1,132 @@ +import type { Caller, Collection, ConditionTree } from '@forestadmin/datasource-toolkit'; + +import { + ConditionTreeBranch, + ConditionTreeFactory, + ConditionTreeLeaf, + PaginatedFilter, + Projection, +} from '@forestadmin/datasource-toolkit'; +import { DataTypes, Sequelize } from 'sequelize'; + +import { createSequelizeDataSource } from '../../../src/index'; +import CONNECTIONS from '../../__tests/connections'; + +describe('Condition tree branches', () => { + const caller = {} as Caller; + const sqlite = { + name: 'SQLite', + open: async () => new Sequelize({ dialect: 'sqlite', storage: ':memory:', logging: false }), + }; + const servers = CONNECTIONS.map(connection => ({ + name: connection.name, + open: async () => { + const db = 'forest_test_branches'; + const server = new Sequelize(connection.uri(), { logging: false }); + await server.getQueryInterface().dropDatabase(db); + await connection.query.createDatabase(server, db); + await server.close(); + + return new Sequelize(connection.uri(db), { logging: false }); + }, + })); + + describe.each([sqlite, ...servers])('On $name', ({ open }) => { + let sequelize: Sequelize; + let collection: Collection; + + const listNames = async (conditionTree: ConditionTree) => { + const records = await collection.list( + caller, + new PaginatedFilter({ conditionTree }), + new Projection('name'), + ); + + return records.map(record => record.name).sort(); + }; + + beforeAll(async () => { + sequelize = await open(); + const user = sequelize.define('User', { + name: { type: DataTypes.STRING, allowNull: true }, + amount: { type: DataTypes.INTEGER, allowNull: true }, + }); + const company = sequelize.define('Company', { + name: { type: DataTypes.STRING, allowNull: true }, + }); + user.belongsTo(company); + await sequelize.sync({ force: true }); + + await company.bulkCreate([ + { id: 1, name: 'Forest' }, + { id: 2, name: 'Evil Corp' }, + ]); + await user.bulkCreate([ + { name: 'John', amount: 1, CompanyId: 1 }, + { name: 'Jane', amount: 10, CompanyId: 1 }, + { name: 'Smith', amount: 11, CompanyId: 2 }, + { name: 'Orphan', amount: 42, CompanyId: null }, + ]); + + collection = (await createSequelizeDataSource(sequelize)(jest.fn(), jest.fn())).getCollection( + 'User', + ); + }, 60_000); + + afterAll(async () => { + await sequelize?.close(); + }); + + it('keeps a record without relation when the relation is filtered in another Or branch', async () => { + const names = await listNames( + new ConditionTreeBranch('Or', [ + new ConditionTreeBranch('And', [ + new ConditionTreeLeaf('Company:name', 'Equal', 'Forest'), + new ConditionTreeLeaf('amount', 'GreaterThan', 5), + ]), + new ConditionTreeLeaf('amount', 'Equal', 42), + ]), + ); + + expect(names).toEqual(['Jane', 'Orphan']); + }); + + it('applies an Or group nested in an And', async () => { + const names = await listNames( + new ConditionTreeBranch('And', [ + new ConditionTreeLeaf('amount', 'GreaterThan', 5), + new ConditionTreeBranch('Or', [ + new ConditionTreeLeaf('Company:name', 'Equal', 'Evil Corp'), + new ConditionTreeLeaf('name', 'Equal', 'Orphan'), + ]), + ]), + ); + + expect(names).toEqual(['Orphan', 'Smith']); + }); + + it('returns nothing for MatchNone', async () => { + expect(await listNames(ConditionTreeFactory.MatchNone)).toEqual([]); + }); + + it('returns nothing for MatchNone intersected with another condition', async () => { + const names = await listNames( + new ConditionTreeBranch('And', [ + new ConditionTreeLeaf('amount', 'GreaterThan', 0), + ConditionTreeFactory.MatchNone, + ]), + ); + + expect(names).toEqual([]); + }); + + it('returns every record for an empty And', async () => { + expect(await listNames(new ConditionTreeBranch('And', []))).toEqual([ + 'Jane', + 'John', + 'Orphan', + 'Smith', + ]); + }); + }); +});