fix(core): respect filters with joined loading strategy

Previously, the filters were applied only when using the select-in strategy, as part of the `where` condition.
With this change, when using the joined strategy, filters will be also applied to the query, via `join on` conditions.

Closes #704
Closes #2440

packages/core/src/EntityManager.ts

@@ -1,6 +1,16 @@
 import { inspect } from 'util';
-import { QueryHelper, TransactionContext, Utils, type Configuration, getOnConflictReturningFields } from './utils';
-import { EntityAssigner, EntityFactory, EntityLoader, EntityValidator, helper, Reference, type AssignOptions, type EntityLoaderOptions, type EntityRepository } from './entity';
+import { type Configuration, getOnConflictReturningFields, QueryHelper, TransactionContext, Utils } from './utils';
+import {
+  type AssignOptions,
+  EntityAssigner,
+  EntityFactory,
+  EntityLoader,
+  type EntityLoaderOptions,
+  type EntityRepository,
+  EntityValidator,
+  helper,
+  Reference,
+} from './entity';
 import { ChangeSet, ChangeSetType, UnitOfWork } from './unit-of-work';
 import type {
   CountOptions,
@@ -15,8 +25,8 @@ import type {
   LockOptions,
   NativeInsertUpdateOptions,
   UpdateOptions,
-  UpsertOptions,
   UpsertManyOptions,
+  UpsertOptions,
 } from './drivers';
 import type {
   AnyEntity,
@@ -34,16 +44,26 @@ import type {
   IHydrator,
   Loaded,
   MaybePromise,
+  ObjectQuery,
   Populate,
   PopulateOptions,
   Primary,
-  RequiredEntityData,
   Ref,
+  RequiredEntityData,
 } from './typings';
-import { EventType, FlushMode, LoadStrategy, LockMode, PopulateHint, ReferenceType, SCALAR_TYPES, type TransactionOptions } from './enums';
+import {
+  EventType,
+  FlushMode,
+  LoadStrategy,
+  LockMode,
+  PopulateHint,
+  ReferenceType,
+  SCALAR_TYPES,
+  type TransactionOptions,
+} from './enums';
 import type { MetadataStorage } from './metadata';
 import type { Transaction } from './connections';
-import { EventManager, TransactionEventBroadcaster, type FlushEventArgs } from './events';
+import { EventManager, type FlushEventArgs, TransactionEventBroadcaster } from './events';
 import type { EntityComparator } from './utils/EntityComparator';
 import { OptimisticLockError, ValidationError } from './errors';
 
@@ -166,7 +186,7 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
     if (cached?.data) {
       await em.entityLoader.populate<Entity, Hint>(entityName, cached.data as Entity[], populate, {
         ...options as Dictionary,
-        ...em.getPopulateWhere(where as FilterQuery<Entity>, options),
+        ...em.getPopulateWhere(where as ObjectQuery<Entity>, options),
         convertCustomTypes: false,
         ignoreLazyScalarProperties: true,
         lookup: false,
@@ -175,14 +195,16 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
       return cached.data;
     }
 
+    const meta = this.metadata.get<Entity>(entityName);
+    options = { ...options };
+    options.populateWhere = await this.applyJoinedFilters(meta, { ...where } as ObjectQuery<Entity>, options);
     const results = await em.driver.find<Entity, Hint>(entityName, where, { ctx: em.transactionContext, ...options });
 
     if (results.length === 0) {
       await em.storeCache(options.cache, cached!, []);
       return [];
     }
 
-    const meta = this.metadata.get(entityName);
     const ret: Entity[] = [];
 
     for (const data of results) {
@@ -206,7 +228,7 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
     const unique = Utils.unique(ret);
     await em.entityLoader.populate<Entity, Hint>(entityName, unique, populate, {
       ...options as Dictionary,
-      ...em.getPopulateWhere(where as FilterQuery<Entity>, options),
+      ...em.getPopulateWhere(where as ObjectQuery<Entity>, options),
       convertCustomTypes: false,
       ignoreLazyScalarProperties: true,
       lookup: false,
@@ -219,14 +241,14 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
 
   private getPopulateWhere<
     Entity extends object,
-    Hint extends string,
-  >(where: FilterQuery<Entity>, options: Pick<FindOptions<Entity, Hint>, 'populateWhere'>): { where: FilterQuery<Entity>; populateWhere?: PopulateHint } {
+    Hint extends string = never,
+  >(where: ObjectQuery<Entity>, options: Pick<FindOptions<Entity, Hint>, 'populateWhere'>): { where: ObjectQuery<Entity>; populateWhere?: PopulateHint } {
     if (options.populateWhere === undefined) {
       options.populateWhere = this.config.get('populateWhere');
     }
 
     if (options.populateWhere === PopulateHint.ALL) {
-      return { where: {} as FilterQuery<Entity>, populateWhere: options.populateWhere };
+      return { where: {} as ObjectQuery<Entity>, populateWhere: options.populateWhere };
     }
 
     if (options.populateWhere === PopulateHint.INFER) {
@@ -323,6 +345,45 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
     return where;
   }
 
+  protected async applyJoinedFilters<Entity extends object>(meta: EntityMetadata<Entity>, cond: ObjectQuery<Entity>, options: FindOptions<Entity, any> | FindOneOptions<Entity, any>): Promise<ObjectQuery<Entity>> {
+    const ret = {} as ObjectQuery<Entity>;
+    const populateWhere = options.populateWhere ?? this.config.get('populateWhere');
+
+    if (populateWhere === PopulateHint.INFER) {
+      Utils.merge(ret, cond);
+    } else if (typeof populateWhere === 'object') {
+      Utils.merge(ret, populateWhere);
+    }
+
+    if (options.populate) {
+      for (const hint of (options.populate as unknown as PopulateOptions<Entity>[])) {
+        const prop = meta.properties[hint.field];
+        const joined = (hint.strategy || prop.strategy || this.config.get('loadStrategy')) === LoadStrategy.JOINED && prop.reference !== ReferenceType.SCALAR;
+
+        if (!joined) {
+          continue;
+        }
+
+        const where = await this.applyFilters(prop.type, {}, options.filters ?? {}, 'read', { ...options, populate: hint.children });
+        const where2 = await this.applyJoinedFilters<Entity>(prop.targetMeta!, {} as ObjectQuery<Entity>, { ...options, populate: hint.children as any, populateWhere: PopulateHint.ALL });
+
+        if (Utils.hasObjectKeys(where)) {
+          ret[hint.field] = ret[hint.field] ? { $and: [where, ret[hint.field]] } : where;
+        }
+
+        if (Utils.hasObjectKeys(where2)) {
+          if (ret[hint.field]) {
+            Utils.merge(ret[hint.field], where2);
+          } else {
+            ret[hint.field] = where2;
+          }
+        }
+      }
+    }
+
+    return ret;
+  }
+
   /**
    * @internal
    */
@@ -457,7 +518,7 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
     if (cached?.data) {
       await em.entityLoader.populate<Entity, Hint>(entityName, [cached.data as Entity], options.populate as unknown as PopulateOptions<Entity>[], {
         ...options as Dictionary,
-        ...em.getPopulateWhere(where as FilterQuery<Entity>, options),
+        ...em.getPopulateWhere(where as ObjectQuery<Entity>, options),
         convertCustomTypes: false,
         ignoreLazyScalarProperties: true,
         lookup: false,
@@ -466,7 +527,12 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
       return cached.data;
     }
 
-    const data = await em.driver.findOne<Entity, Hint>(entityName, where, { ctx: em.transactionContext, ...options });
+    options = { ...options };
+    options.populateWhere = await this.applyJoinedFilters(meta, { ...where } as ObjectQuery<Entity>, options);
+    const data = await em.driver.findOne<Entity, Hint>(entityName, where, {
+      ctx: em.transactionContext,
+      ...options,
+    });
 
     if (!data) {
       await em.storeCache(options.cache, cached!, null);
@@ -515,6 +581,7 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
       const key = options.strict ? 'findExactlyOneOrFailHandler' : 'findOneOrFailHandler';
       options.failHandler ??= this.config.get(key);
       entityName = Utils.className(entityName);
+      where = Utils.isEntity(where) ? helper(where).getPrimaryKey() as any : where;
       throw options.failHandler!(entityName, where);
     }
 
@@ -1616,7 +1683,7 @@ export class EntityManager<D extends IDatabaseDriver = IDatabaseDriver> {
     const preparedPopulate = this.preparePopulate<T, P>(entityName, options);
     await this.entityLoader.populate(entityName, [entity], preparedPopulate, {
       ...options as Dictionary,
-      ...this.getPopulateWhere(where, options),
+      ...this.getPopulateWhere<T>(where as ObjectQuery<T>, options),
       convertCustomTypes: false,
       ignoreLazyScalarProperties: true,
       lookup: false,

packages/knex/src/AbstractSqlDriver.ts

@@ -112,7 +112,7 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
     const result = await this.rethrow(qb.execute('all'));
 
     if (joinedProps.length > 0) {
-      return this.mergeJoinedResult(result, meta);
+      return this.mergeJoinedResult(result, meta, joinedProps);
     }
 
     return result;
@@ -770,14 +770,22 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
   /**
    * @internal
    */
-  mergeJoinedResult<T extends object>(rawResults: EntityData<T>[], meta: EntityMetadata<T>): EntityData<T>[] {
+  mergeJoinedResult<T extends object>(rawResults: EntityData<T>[], meta: EntityMetadata<T>, joinedProps: PopulateOptions<T>[]): EntityData<T>[] {
     // group by the root entity primary key first
-    const map: Dictionary = {};
+    const map: Dictionary<Dictionary[]> = {};
     const res: EntityData<T>[] = [];
     rawResults.forEach(item => {
       const pk = Utils.getCompositeKeyHash(item, meta);
 
       if (map[pk]) {
+        joinedProps.forEach(hint => {
+          const item1 = map[pk][0];
+          const item2 = item;
+          if (Array.isArray(item1[hint.field]) && Array.isArray(item2[hint.field])) {
+            item1[hint.field].push(...item2[hint.field]);
+          }
+        });
+
         map[pk].push(item);
       } else {
         map[pk] = [item];

packages/knex/src/query/QueryBuilder.ts

@@ -669,7 +669,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
     let res = await this.execute<EntityData<T>[]>('all', true);
 
     if (this._joinedProps.size > 0) {
-      res = this.driver.mergeJoinedResult(res, this.mainAlias.metadata!);
+      res = this.driver.mergeJoinedResult(res, this.mainAlias.metadata!, [...this._joinedProps.values()]);
     }
 
     const entities: T[] = [];

tests/features/filters/filters.postgres.test.ts

@@ -1,8 +1,42 @@
-import { Collection, Entity, ManyToMany, MikroORM, PrimaryKey, Property, Filter, ManyToOne } from '@mikro-orm/core';
+import {
+  Collection,
+  Entity,
+  Filter,
+  LoadStrategy,
+  ManyToMany,
+  ManyToOne,
+  MikroORM,
+  OneToMany,
+  PrimaryKey,
+  Property,
+  Ref,
+} from '@mikro-orm/core';
 import type { AbstractSqlDriver, EntityManager } from '@mikro-orm/knex';
 import { mockLogger } from '../../helpers';
 import { PostgreSqlDriver } from '@mikro-orm/postgresql';
 
+@Entity()
+@Filter({
+  name: 'isActive',
+  cond: { active: true },
+  default: true,
+})
+class BenefitDetail {
+
+  @PrimaryKey()
+  id!: number;
+
+  @Property()
+  description!: string;
+
+  @ManyToOne(() => Benefit, { ref: true })
+  benefit!: Ref<Benefit>;
+
+  @Property()
+  active: boolean = false;
+
+}
+
 @Filter({
   name: 'isActive',
   cond: { benefitStatus: 'A' },
@@ -24,6 +58,9 @@ class Benefit extends BaseBenefit {
   @Property({ nullable: true })
   name?: string;
 
+  @OneToMany(() => BenefitDetail, d => d.benefit)
+  details = new Collection<BenefitDetail>(this);
+
 }
 
 @Entity()
@@ -103,27 +140,50 @@ describe('filters [postgres]', () => {
     const mock = mockLogger(orm, ['query']);
 
     const benefit = new Benefit();
+    benefit.name = 'b1';
     benefit.benefitStatus = 'IA';
+    const benefit2 = new Benefit();
+    benefit2.name = 'b2';
+    benefit2.benefitStatus = 'A';
+    orm.em.assign(benefit, { details: [{ description: 'detail 11', active: true }, { description: 'detail 12', active: false }, { description: 'detail 13', active: true }] });
+    orm.em.assign(benefit2, { details: [{ description: 'detail 21', active: false }, { description: 'detail 22', active: true }, { description: 'detail 23', active: false }] });
     const employee = new Employee();
-    employee.benefits.add(benefit);
+    employee.benefits.add(benefit, benefit2);
     await orm.em.persistAndFlush(employee);
+    expect(mock.mock.calls[0][0]).toMatch(`begin`);
+    expect(mock.mock.calls[1][0]).toMatch(`insert into "employee" ("id") values (default) returning "id"`);
+    expect(mock.mock.calls[2][0]).toMatch(`insert into "benefit" ("benefit_status", "name") values ($1, $2), ($3, $4) returning "id"`);
+    expect(mock.mock.calls[3][0]).toMatch(`insert into "benefit_detail" ("description", "benefit_id", "active") values ($1, $2, $3), ($4, $5, $6), ($7, $8, $9), ($10, $11, $12), ($13, $14, $15), ($16, $17, $18) returning "id", "active"`);
+    expect(mock.mock.calls[4][0]).toMatch(`insert into "employee_benefits" ("employee_id", "benefit_id") values ($1, $2)`);
+    expect(mock.mock.calls[5][0]).toMatch(`commit`);
     orm.em.clear();
+    mock.mockReset();
 
     const b1 = await orm.em.find(Benefit, {});
-    expect(b1).toHaveLength(0);
+    expect(b1).toHaveLength(1);
+    expect(mock.mock.calls[0][0]).toMatch(`select "b0".* from "benefit" as "b0" where "b0"."benefit_status" = $1`);
     orm.em.clear();
+    mock.mockReset();
 
-    const e1 = await orm.em.findOneOrFail(Employee, employee.id, { populate: ['benefits'] });
-    expect(e1.benefits).toHaveLength(0);
+    const e1 = await orm.em.findOneOrFail(Employee, employee.id, { populate: ['benefits.details'] });
+    expect(mock.mock.calls[0][0]).toMatch(`select "e0".* from "employee" as "e0" where "e0"."id" = $1 limit $2`);
+    expect(mock.mock.calls[1][0]).toMatch(`select "b0".*, "e1"."benefit_id" as "fk__benefit_id", "e1"."employee_id" as "fk__employee_id" from "benefit" as "b0" left join "employee_benefits" as "e1" on "b0"."id" = "e1"."benefit_id" where "b0"."benefit_status" = $1 and "e1"."employee_id" in ($2)`);
+    expect(mock.mock.calls[2][0]).toMatch(`select "b0".* from "benefit_detail" as "b0" where "b0"."active" = $1 and "b0"."benefit_id" in ($2) order by "b0"."benefit_id" asc`);
+    expect(e1.benefits).toHaveLength(1);
+    expect(e1.benefits[0].details).toHaveLength(1);
 
-    expect(mock.mock.calls[0][0]).toMatch(`begin`);
-    expect(mock.mock.calls[1][0]).toMatch(`insert into "employee" ("id") values (default) returning "id"`);
-    expect(mock.mock.calls[2][0]).toMatch(`insert into "benefit" ("benefit_status") values ($1) returning "id"`);
-    expect(mock.mock.calls[3][0]).toMatch(`insert into "employee_benefits" ("employee_id", "benefit_id") values ($1, $2)`);
-    expect(mock.mock.calls[4][0]).toMatch(`commit`);
-    expect(mock.mock.calls[5][0]).toMatch(`select "b0".* from "benefit" as "b0" where "b0"."benefit_status" = $1`);
-    expect(mock.mock.calls[6][0]).toMatch(`select "e0".* from "employee" as "e0" where "e0"."id" = $1 limit $2`);
-    expect(mock.mock.calls[7][0]).toMatch(`select "b0".*, "e1"."benefit_id" as "fk__benefit_id", "e1"."employee_id" as "fk__employee_id" from "benefit" as "b0" left join "employee_benefits" as "e1" on "b0"."id" = "e1"."benefit_id" where "b0"."benefit_status" = $1 and "e1"."employee_id" in ($2)`);
+    orm.em.clear();
+    mock.mockReset();
+
+    const e2 = await orm.em.findOneOrFail(Employee, employee.id, { populate: ['benefits.details'], strategy: LoadStrategy.JOINED });
+    expect(mock.mock.calls[0][0]).toMatch('select "e0"."id", "b1"."id" as "b1__id", "b1"."benefit_status" as "b1__benefit_status", "b1"."name" as "b1__name", "d3"."id" as "d3__id", "d3"."description" as "d3__description", "d3"."benefit_id" as "d3__benefit_id", "d3"."active" as "d3__active" ' +
+        'from "employee" as "e0" ' +
+        'left join "employee_benefits" as "e2" on "e0"."id" = "e2"."employee_id" ' +
+        'left join "benefit" as "b1" on "e2"."benefit_id" = "b1"."id" and "b1"."benefit_status" = $1 ' +
+        'left join "benefit_detail" as "d3" on "b1"."id" = "d3"."benefit_id" and "d3"."active" = $2 ' +
+        'where "e0"."id" = $3');
+    expect(e2.benefits).toHaveLength(1);
+    expect(e2.benefits[0].details).toHaveLength(1);
   });
 
   test('merging $or conditions', async () => {