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 <red2@r4m.co>
Co-authored-by: Sergii <sergiy.stotskiy@gmail.com>

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' }
     }
   })
 

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
         }
       }
     }

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() {

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<User>({
@@ -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' }

packages/sql/src/lib/typeorm.ts

@@ -9,8 +9,14 @@ import {
 
 function joinRelation<Entity>(relationName: string, query: SelectQueryBuilder<Entity>) {
   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;