Skip to content

Commit

Permalink
fix(query-builder): merge raw join results in qb.execute() (#4825)
Browse files Browse the repository at this point in the history 
Previously, the `qb.execute()` method returned cartesian product when
joining to-many relations. Now the results will be automatically merged,
as if you would map the results via `qb.getResult()`.

Closes #4816
Related #4741
  • Loading branch information
@B4nan
B4nan committed Oct 15, 2023
1 parent 06c7dbf commit 5a28e9b
Show file tree
Hide file tree
Showing 6 changed files with 233 additions and 114 deletions.
42 changes: 0 additions & 42 deletions packages/core/src/utils/Utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1049,48 +1049,6 @@ export class Utils {
}
}

private static processValue(obj: Dictionary, k: keyof typeof obj, v: unknown, meta: EntityMetadata, seen: Set<unknown>, idx?: number) {
const targetMeta = meta.properties[k]?.targetMeta;

if (typeof v !== 'object' || !v || !targetMeta) {
return;
}

if (Array.isArray(v)) {
let i = 0;

for (const el of v) {
this.processValue(obj, k, el, meta, seen, i++);
}

return;
}

if (!seen.has(v)) {
this.removeCycles(v, targetMeta, seen);
return;
}

const pk = Utils.getPrimaryKeyValues(v, targetMeta.primaryKeys, true);

if (idx != null) {
obj[k][idx] = pk;
} else {
obj[k] = pk;
}
}

/**
* Removes cycles from an entity graph, replacing the entity with its primary key value.
*/
static removeCycles(obj: Dictionary, meta: EntityMetadata, seen = new Set()) {
seen.add(obj);

for (const [k, v] of Object.entries(obj)) {
this.processValue(obj, k, v, meta, seen);
}
}

static unwrapProperty<T>(entity: T, meta: EntityMetadata<T>, prop: EntityProperty<T>, payload = false): [unknown, number[]][] {
let p = prop;
const path: string[] = [];
Expand Down
66 changes: 27 additions & 39 deletions packages/knex/src/AbstractSqlDriver.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,7 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
}

Utils.asArray(options.flags).forEach(flag => qb.setFlag(flag));
const result = await this.rethrow(qb.execute('all'));

if (joinedProps.length > 0) {
return this.mergeJoinedResult(result, meta, joinedProps);
}

return result;
return this.rethrow(qb.execute('all'));
}

async findOne<T extends object, P extends string = never>(entityName: string, where: FilterQuery<T>, options?: FindOneOptions<T, P>): Promise<EntityData<T> | null> {
Expand Down Expand Up @@ -222,8 +216,6 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
if (qb) {
// here we map the aliased results (cartesian product) to an object graph
this.mapJoinedProps<T>(ret, meta, populate, qb, ret, map);
// we need to remove the cycles from the mapped values
Utils.removeCycles(ret, meta);
}

return ret;
Expand All @@ -243,7 +235,7 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
const meta2 = this.metadata.find(relation.type)!;
const path = parentJoinPath ? `${parentJoinPath}.${relation.name}` : `${meta.name}.${relation.name}`;
const relationAlias = qb.getAliasForJoinPath(path);
let relationPojo: EntityData<unknown> = {};
const relationPojo: EntityData<unknown> = {};

// If the primary key value for the relation is null, we know we haven't joined to anything
// and therefore we don't return any record (since all values would be null)
Expand Down Expand Up @@ -287,23 +279,6 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
}
});

const key = `${meta.name}-${Utils.getCompositeKeyHash(result, meta)}`;
const key2 = `${meta2.name}-${Utils.getCompositeKeyHash(relationPojo, meta2)}`;

if (map[key2]) {
const old = map[key2];
Object.assign(old, relationPojo);
relationPojo = old;
} else {
map[key2] = relationPojo;
}

if (map[key]) {
result[relation.name] = map[key][relation.name];
} else {
map[key] = result;
}

if ([ReferenceType.MANY_TO_MANY, ReferenceType.ONE_TO_MANY].includes(relation.reference)) {
result[relation.name] = result[relation.name] || [] as unknown as T[keyof T & string];
this.appendToCollection(meta2, result[relation.name] as Dictionary[], relationPojo);
Expand Down Expand Up @@ -775,7 +750,10 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
return [...populate, ...toPopulate];
}

protected joinedProps<T>(meta: EntityMetadata, populate: PopulateOptions<T>[]): PopulateOptions<T>[] {
/**
* @internal
*/
joinedProps<T>(meta: EntityMetadata, populate: PopulateOptions<T>[]): PopulateOptions<T>[] {
return populate.filter(p => {
const prop = meta.properties[p.field] || {};
return (p.strategy || prop.strategy || this.config.get('loadStrategy')) === LoadStrategy.JOINED && prop.reference !== ReferenceType.SCALAR;
Expand All @@ -786,26 +764,36 @@ export abstract class AbstractSqlDriver<Connection extends AbstractSqlConnection
* @internal
*/
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<Dictionary> = {};
const res: EntityData<T>[] = [];
rawResults.forEach(item => {
const map: Dictionary<Dictionary> = {};

for (const item of rawResults) {
const pk = Utils.getCompositeKeyHash(item, meta);

if (map[pk]) {
joinedProps.forEach(hint => {
// Sometimes we might join a M:N relation with additional filter on the target entity, and as a result, we get
// the first result with `null` for all target values, which is mapped as empty array. When we see that happen,
// we need to merge the results of the next item.
if (Array.isArray(map[pk][hint.field]) && Array.isArray(item[hint.field]) && map[pk][hint.field].length === 0) {
item[hint.field].forEach((el: T) => map[pk][hint.field].push(el));
for (const hint of joinedProps) {
const prop = meta.properties[hint.field];

if (!item[hint.field]) {
continue;
}
});

switch (prop.reference) {
case ReferenceType.ONE_TO_MANY:
case ReferenceType.MANY_TO_MANY:
map[pk][hint.field] = this.mergeJoinedResult<T>([...map[pk][hint.field], ...item[hint.field]], prop.targetMeta!, hint.children ?? []);
break;
case ReferenceType.MANY_TO_ONE:
case ReferenceType.ONE_TO_ONE:
map[pk][hint.field] = this.mergeJoinedResult<T>([map[pk][hint.field], item[hint.field]], prop.targetMeta!, hint.children ?? [])[0];
break;
}
}
} else {
map[pk] = item;
res.push(item);
}
});
}

return res;
}
Expand Down
32 changes: 20 additions & 12 deletions packages/knex/src/query/QueryBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -640,18 +640,31 @@ export class QueryBuilder<T extends object = AnyEntity> {
return res as unknown as U;
}

if (method === 'all' && Array.isArray(res)) {
if (method === 'run') {
return res as U;
}

const joinedProps = this.driver.joinedProps(meta, this._populate);
let mapped: EntityData<T>[];

if (Array.isArray(res)) {
const map: Dictionary = {};
const mapped = res.map(r => this.driver.mapResult<T>(r, meta, this._populate, this, map)) as unknown as U;
await this.em?.storeCache(this._cache, cached!, mapped);
mapped = res.map(r => this.driver.mapResult<T>(r, meta, this._populate, this, map)!);

return mapped;
if (joinedProps.length > 0) {
mapped = this.driver.mergeJoinedResult(mapped, this.mainAlias.metadata!, joinedProps);
}
} else {
mapped = [this.driver.mapResult<T>(res, meta, joinedProps, this)!];
}

const mapped = this.driver.mapResult(res as T, meta, this._populate, this) as unknown as U;
await this.em?.storeCache(this._cache, cached!, mapped);

return mapped;
if (method === 'get') {
return mapped[0] as U;
}

return mapped as U;
}

/**
Expand All @@ -666,12 +679,7 @@ export class QueryBuilder<T extends object = AnyEntity> {
*/
async getResultList(): Promise<T[]> {
await this.em!.tryFlush(this.mainAlias.entityName, { flushMode: this.flushMode });
let res = await this.execute<EntityData<T>[]>('all', true);

if (this._joinedProps.size > 0) {
res = this.driver.mergeJoinedResult(res, this.mainAlias.metadata!, [...this._joinedProps.values()]);
}

const res = await this.execute<EntityData<T>[]>('all', true);
const entities: T[] = [];

function propagatePopulateHint<U>(entity: U, hint: PopulateOptions<U>[]) {
Expand Down
34 changes: 17 additions & 17 deletions tests/EntityManager.mysql.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1481,13 +1481,13 @@ describe('EntityManagerMySql', () => {
populateWhere: PopulateHint.INFER,
orderBy: { title: QueryOrder.DESC, tags: { name: QueryOrder.ASC } },
});
await expect(books.length).toBe(3);
await expect(books[0].title).toBe('My Life on The Wall, part 3');
await expect(books[0].tags.getItems().map(t => t.name)).toEqual(['awkward', 'sexy', 'strange']);
await expect(books[1].title).toBe('My Life on The Wall, part 2');
await expect(books[1].tags.getItems().map(t => t.name)).toEqual(['sexy', 'silly', 'zupa']);
await expect(books[2].title).toBe('My Life on The Wall, part 1');
await expect(books[2].tags.getItems().map(t => t.name)).toEqual(['awkward', 'sick', 'silly', 'zupa']);
expect(books.length).toBe(3);
expect(books[0].title).toBe('My Life on The Wall, part 3');
expect(books[0].tags.getItems().map(t => t.name)).toEqual(['awkward', 'sexy', 'strange']);
expect(books[1].title).toBe('My Life on The Wall, part 2');
expect(books[1].tags.getItems().map(t => t.name)).toEqual(['sexy', 'silly', 'zupa']);
expect(books[2].title).toBe('My Life on The Wall, part 1');
expect(books[2].tags.getItems().map(t => t.name)).toEqual(['awkward', 'sick', 'silly', 'zupa']);
});

test('many to many with composite pk', async () => {
Expand Down Expand Up @@ -1525,13 +1525,13 @@ describe('EntityManagerMySql', () => {
'left join `test2` as `t3` on `b0`.`uuid_pk` = `t3`.`book_uuid_pk` ' +
'where `b0`.`author_id` is not null and `b1`.`name` != ? ' +
'order by `b0`.`title` desc');
await expect(books.length).toBe(3);
await expect(books[0].title).toBe('My Life on The Wall, part 3');
await expect(books[0].tagsUnordered.getItems().map(t => t.name)).toEqual(['awkward', 'sexy', 'strange']);
await expect(books[1].title).toBe('My Life on The Wall, part 2');
await expect(books[1].tagsUnordered.getItems().map(t => t.name)).toEqual(['sexy', 'silly', 'zupa']);
await expect(books[2].title).toBe('My Life on The Wall, part 1');
await expect(books[2].tagsUnordered.getItems().map(t => t.name)).toEqual(['awkward', 'sick', 'silly', 'zupa']);
expect(books.length).toBe(3);
expect(books[0].title).toBe('My Life on The Wall, part 3');
expect(books[0].tagsUnordered.getItems().map(t => t.name)).toEqual(['awkward', 'sexy', 'strange']);
expect(books[1].title).toBe('My Life on The Wall, part 2');
expect(books[1].tagsUnordered.getItems().map(t => t.name)).toEqual(['sexy', 'silly', 'zupa']);
expect(books[2].title).toBe('My Life on The Wall, part 1');
expect(books[2].tagsUnordered.getItems().map(t => t.name)).toEqual(['awkward', 'sick', 'silly', 'zupa']);

orm.em.clear();
mock.mock.calls.length = 0;
Expand All @@ -1544,9 +1544,9 @@ describe('EntityManagerMySql', () => {
'left join `book_to_tag_unordered` as `b1` on `b0`.`uuid_pk` = `b1`.`book2_uuid_pk` ' +
'left join `test2` as `t2` on `b0`.`uuid_pk` = `t2`.`book_uuid_pk` ' +
'where `b0`.`author_id` is not null and `b0`.`title` != ? and `b1`.`book_tag2_id` in (?, ?, ?, ?, ?, ?)');
await expect(tags.length).toBe(6);
await expect(tags.map(tag => tag.name)).toEqual(['awkward', 'funny', 'sexy', 'sick', 'silly', 'zupa']);
await expect(tags.map(tag => tag.booksUnordered.count())).toEqual([1, 1, 1, 1, 2, 2]);
expect(tags.length).toBe(6);
expect(tags.map(tag => tag.name)).toEqual(['awkward', 'funny', 'sexy', 'sick', 'silly', 'zupa']);
expect(tags.map(tag => tag.booksUnordered.count())).toEqual([1, 1, 1, 1, 2, 2]);
});

test('self referencing M:N (unidirectional)', async () => {
Expand Down
4 changes: 0 additions & 4 deletions tests/issues/GH4741.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,6 @@ beforeEach(async () => {

// Outer --> active Division --> [Inners] --> Geometry
test(`GH4741 issue (1/3)`, async () => {

const em = orm.em.fork();
const qb = em.createQueryBuilder(Outer, 'o');

Expand Down Expand Up @@ -135,7 +134,6 @@ test(`GH4741 issue (1/3)`, async () => {
expect(geom).toBeInstanceOf(Geometry);
expect(wrap(geom).isInitialized()).toBeTruthy(); // Succeeds
}
em.clear();
});

// Outer --> active Division --> [Inners] --> Geometry
Expand Down Expand Up @@ -175,7 +173,6 @@ test(`GH4741 issue (2/3)`, async () => {
expect(geom).toBeInstanceOf(Geometry);
expect(wrap(geom).isInitialized()).toBeTruthy(); // Succeeds
}
em.clear();
});

// Outer --> active Division --> [Inners] --> Geometry
Expand Down Expand Up @@ -215,5 +212,4 @@ test(`GH4741 issue (3/3)`, async () => {
expect(geom).toBeInstanceOf(Geometry);
expect(wrap(geom).isInitialized()).toBeTruthy(); // Fails
}
em.clear();
});

0 comments on commit 5a28e9b

Please sign in to comment.