From d725e3528f0da11e27207ca8db7e1b6bf669fc34 Mon Sep 17 00:00:00 2001 From: Claudio Catterina Date: Thu, 15 Jul 2021 11:22:16 +0200 Subject: [PATCH] fix(sql): Interpreters should add one join per relation at most (#24) * fix(sql): Interpreters should add one join per relation at most If a condition specifies the same relation several times the interpreters should add the join statement only once. * refactor: replaces `find` with `some` Co-authored-by: Claudio Catterina Co-authored-by: Sergii --- packages/sql/spec/mikro-orm.spec.ts | 21 +++++++++++++++++++-- packages/sql/spec/objection.spec.ts | 21 +++++++++++++++++++-- packages/sql/spec/sequelize.spec.ts | 15 ++++++++++++++- packages/sql/spec/typeorm.spec.ts | 20 +++++++++++++++++++- packages/sql/src/lib/typeorm.ts | 8 +++++++- 5 files changed, 78 insertions(+), 7 deletions(-) diff --git a/packages/sql/spec/mikro-orm.spec.ts b/packages/sql/spec/mikro-orm.spec.ts index 43cfbf7..193947b 100644 --- a/packages/sql/spec/mikro-orm.spec.ts +++ b/packages/sql/spec/mikro-orm.spec.ts @@ -1,4 +1,4 @@ -import { FieldCondition } from '@ucast/core' +import { FieldCondition, CompoundCondition } from '@ucast/core' import { MikroORM, Collection, EntitySchema, QueryBuilder } from 'mikro-orm' import { interpret } from '../src/lib/mikro-orm' import { expect } from './specHelper' @@ -46,6 +46,21 @@ describe('Condition interpreter for MikroORM', () => { 'where (`projects`.`name` = ?)' ].join(' ')) }) + + it('should join the same relation exactly one time', () => { + const condition = new CompoundCondition('and', [ + new FieldCondition('eq', 'projects.name', 'test'), + new FieldCondition('eq', 'projects.active', true), + ]) + const query = interpret(condition, orm.em.createQueryBuilder(User).select([])) + + expect(query.getQuery()).to.equal([ + 'select *', + 'from `user` as `e0`', + 'inner join `project` as `projects` on `e0`.`id` = `projects`.`user_id`', + 'where ((`projects`.`name` = ? and `projects`.`active` = ?))' + ].join(' ')) + }) }) async function configureORM() { @@ -63,7 +78,8 @@ async function configureORM() { constructor( public id: number, public name: string, - public user: User + public user: User, + public active: boolean ) {} } @@ -85,6 +101,7 @@ async function configureORM() { id: { type: 'number', primary: true }, name: { type: 'string' }, user: { type: 'User', reference: 'm:1' }, + active: { type: 'boolean' } } }) diff --git a/packages/sql/spec/objection.spec.ts b/packages/sql/spec/objection.spec.ts index 4bd6539..9843f02 100644 --- a/packages/sql/spec/objection.spec.ts +++ b/packages/sql/spec/objection.spec.ts @@ -1,4 +1,4 @@ -import { FieldCondition } from '@ucast/core' +import { FieldCondition, CompoundCondition } from '@ucast/core' import { Model, QueryBuilder } from 'objection' import Knex from 'knex' import { interpret } from '../src/lib/objection' @@ -43,6 +43,22 @@ describe('Condition interpreter for Objection', () => { where "projects"."name" = 'test' `.trim()) }) + + it('should join the same relation exactly one time', () => { + const condition = new CompoundCondition('and', [ + new FieldCondition('eq', 'projects.name', 'test'), + new FieldCondition('eq', 'projects.active', true), + ]) + + const query = interpret(condition, User.query()) + + expect(query.toKnexQuery().toString()).to.equal(linearize` + select "users".* + from "users" + inner join "projects" on "projects"."user_id" = "users"."id" + where ("projects"."name" = 'test' and "projects"."active" = true) + `.trim()) + }) }) function configureORM() { @@ -70,7 +86,8 @@ function configureORM() { user: { relation: Model.BelongsToOneRelation, modelClass: User, - join: { from: 'users.id', to: 'projects.user_id' } + join: { from: 'users.id', to: 'projects.user_id' }, + active: Boolean } } } diff --git a/packages/sql/spec/sequelize.spec.ts b/packages/sql/spec/sequelize.spec.ts index 31c995b..fae4e03 100644 --- a/packages/sql/spec/sequelize.spec.ts +++ b/packages/sql/spec/sequelize.spec.ts @@ -1,4 +1,4 @@ -import { FieldCondition } from '@ucast/core' +import { FieldCondition, CompoundCondition } from '@ucast/core' import { Model, Sequelize, DataTypes } from 'sequelize' import { interpret } from '../src/lib/sequelize' import { expect } from './specHelper' @@ -31,6 +31,19 @@ describe('Condition interpreter for Sequelize', () => { ]) expect(query.where.val).to.equal('`projects`.`name` = \'test\'') }) + + it('should join the same relation exactly one time', () => { + const condition = new CompoundCondition('and', [ + new FieldCondition('eq', 'projects.name', 'test'), + new FieldCondition('eq', 'projects.active', true), + ]) + + const query = interpret(condition, User) + expect(query.include).to.deep.equal([ + { association: 'projects', required: true } + ]) + expect(query.where.val).to.equal('(`projects`.`name` = \'test\' and `projects`.`active` = 1)') + }) }) function configureORM() { diff --git a/packages/sql/spec/typeorm.spec.ts b/packages/sql/spec/typeorm.spec.ts index c1b80a2..4a34756 100644 --- a/packages/sql/spec/typeorm.spec.ts +++ b/packages/sql/spec/typeorm.spec.ts @@ -1,4 +1,4 @@ -import { FieldCondition } from '@ucast/core' +import { FieldCondition, CompoundCondition } from '@ucast/core' import { EntitySchema, createConnection, @@ -66,6 +66,22 @@ describe('Condition interpreter for TypeORM', () => { ].join(' ')) expect(query.getParameters()).to.eql({ 0: 'test' }) }) + + it("shouldn't join the same relation several times", () => { + const condition = new CompoundCondition('and', [ + new FieldCondition('eq', 'projects.name', 'test'), + new FieldCondition('eq', 'projects.active', true), + ]) + const query = interpret(condition, conn.createQueryBuilder(User, 'u')) + + expect(query.getQuery()).to.equal([ + 'SELECT "u"."id" AS "u_id", "u"."name" AS "u_name"', + 'FROM "user" "u"', + 'INNER JOIN "project" "projects" ON "projects"."userId"="u"."id"', + 'WHERE ("projects"."name" = :0 and "projects"."active" = :1)' + ].join(' ')) + expect(query.getParameters()).to.eql({ 0: 'test', 1: true }) + }) }) async function configureORM() { @@ -79,6 +95,7 @@ async function configureORM() { id!: number name!: string user!: User + active!: boolean } const UserSchema = new EntitySchema({ @@ -103,6 +120,7 @@ async function configureORM() { columns: { id: { primary: true, type: 'int', generated: true }, name: { type: 'varchar' }, + active: { type: 'boolean' } }, relations: { user: { target: 'User', type: 'many-to-one' } diff --git a/packages/sql/src/lib/typeorm.ts b/packages/sql/src/lib/typeorm.ts index 73cef03..5aac9d0 100644 --- a/packages/sql/src/lib/typeorm.ts +++ b/packages/sql/src/lib/typeorm.ts @@ -9,8 +9,14 @@ import { function joinRelation(relationName: string, query: SelectQueryBuilder) { const meta = query.expressionMap.mainAlias!.metadata; - const relation = meta.findRelationWithPropertyPath(relationName); + const joinAlreadyExists = query.expressionMap.joinAttributes + .some(j => j.alias.name === relationName); + if (joinAlreadyExists) { + return true; + } + + const relation = meta.findRelationWithPropertyPath(relationName); if (relation) { query.innerJoin(`${query.alias}.${relationName}`, relationName); return true;