Skip to content

Commit

Permalink
fix(core): improve partial loading of 1:m relations
Browse files Browse the repository at this point in the history 
- automatically select the owner's FK, so we can wire the relation
- ignore collection properties in fields as they don't represent any db column

Closes #2651
  • Loading branch information
@B4nan
B4nan committed Jan 21, 2022
1 parent 5ec7393 commit 3ddde1e
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 68 deletions.
35 changes: 25 additions & 10 deletions packages/core/src/entity/EntityLoader.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ export class EntityLoader {
const where = this.mergePrimaryCondition(ids, pk, options, meta, this.metadata, this.driver.getPlatform());
const { filters, convertCustomTypes, lockMode, strategy } = options;

await this.em.find(meta.className, where as FilterQuery<T>, {
await this.em.find(meta.className, where, {
filters, convertCustomTypes, lockMode, strategy,
fields: [prop.name] as never,
});
Expand Down Expand Up @@ -248,20 +248,20 @@ export class EntityLoader {
const fields = this.buildFields(options.fields, prop);
const { refresh, filters, convertCustomTypes, lockMode, strategy } = options;

return this.em.find<T>(prop.type, where as FilterQuery<T>, {
return this.em.find(prop.type, where, {
refresh, filters, convertCustomTypes, lockMode,
orderBy: [...Utils.asArray(options.orderBy), ...Utils.asArray(prop.orderBy), { [fk]: QueryOrder.ASC }] as QueryOrderMap<T>[],
populate: populate.children as never ?? populate.all,
strategy,
fields: fields.length > 0 ? fields : undefined,
}) as Promise<T[]>;
fields,
});
}

private mergePrimaryCondition<T>(ids: T[], pk: string, options: EntityLoaderOptions<T>, meta: EntityMetadata, metadata: MetadataStorage, platform: Platform) {
private mergePrimaryCondition<T>(ids: T[], pk: string, options: EntityLoaderOptions<T>, meta: EntityMetadata, metadata: MetadataStorage, platform: Platform): FilterQuery<T> {
const cond1 = QueryHelper.processWhere({ [pk]: { $in: ids } }, meta.name!, metadata, platform, !options.convertCustomTypes);

return options.where![pk]
? { $and: [cond1, options.where] }
? { $and: [cond1, options.where] } as FilterQuery<any>
: { ...cond1, ...(options.where as Dictionary) };
}

Expand Down Expand Up @@ -301,7 +301,7 @@ export class EntityLoader {
await this.populate<T>(prop.type, filtered, populate.children, {
where: await this.extractChildCondition(options, prop, false) as FilterQuery<T>,
orderBy: innerOrderBy as QueryOrderMap<T>[],
fields: fields.length > 0 ? fields : undefined,
fields,
validate: false,
lookup: false,
refresh,
Expand All @@ -318,7 +318,7 @@ export class EntityLoader {
const options2 = { ...options } as FindOptions<T>;
delete options2.limit;
delete options2.offset;
options2.fields = (fields.length > 0 ? fields : undefined) as EntityField<T>[];
options2.fields = fields;
options2.populate = (populate?.children ?? []) as never;

if (prop.customType) {
Expand Down Expand Up @@ -381,8 +381,8 @@ export class EntityLoader {
return subCond;
}

private buildFields<T, P extends string>(fields: readonly EntityField<T, P>[] = [], prop: EntityProperty<T>): readonly EntityField<T>[] {
return fields.reduce((ret, f) => {
private buildFields<T, P extends string>(fields: readonly EntityField<T, P>[] = [], prop: EntityProperty<T>): readonly EntityField<T>[] | undefined {
const ret = fields.reduce((ret, f) => {
if (Utils.isPlainObject(f)) {
Object.keys(f)
.filter(ff => ff === prop.name)
Expand All @@ -400,6 +400,21 @@ export class EntityLoader {

return ret;
}, [] as EntityField<T>[]);

if (ret.length === 0) {
return undefined;
}

// we need to automatically select the FKs too, e.g. for 1:m relations to be able to wire them with the items
if (prop.reference === ReferenceType.ONE_TO_MANY) {
const owner = prop.targetMeta!.properties[prop.mappedBy] as EntityProperty<T>;

if (!ret.includes(owner.name)) {
ret.push(owner.name);
}
}

return ret;
}

private getChildReferences<T extends AnyEntity<T>>(entities: T[], prop: EntityProperty<T>, refresh: boolean): AnyEntity[] {
Expand Down
19 changes: 13 additions & 6 deletions packages/knex/src/query/QueryBuilder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -625,21 +625,28 @@ export class QueryBuilder<T extends AnyEntity<T> = AnyEntity> {
return prop;
}

private prepareFields<T extends AnyEntity<T>, U extends string | Knex.Raw = string | Knex.Raw>(fields: Field<T>[], type: 'where' | 'groupBy' | 'sub-query' = 'where'): U[] {
private prepareFields<T extends AnyEntity<T>, U extends string | Knex.Raw>(fields: Field<T>[], type: 'where' | 'groupBy' | 'sub-query' = 'where'): U[] {
const ret: Field<T>[] = [];

fields.forEach(f => {
if (!Utils.isString(f)) {
return ret.push(f);
fields.forEach(field => {
if (!Utils.isString(field)) {
return ret.push(field);
}

const join = Object.keys(this._joins).find(k => f === k.substring(0, k.indexOf('#')))!;
const join = Object.keys(this._joins).find(k => field === k.substring(0, k.indexOf('#')))!;

if (join && type === 'where') {
return ret.push(...this.helper.mapJoinColumns(this.type, this._joins[join]) as string[]);
}

ret.push(this.helper.mapper(f, this.type) as string);
const [a, f] = this.helper.splitField(field);
const prop = this.helper.getProperty(f, a);

if (prop && [ReferenceType.ONE_TO_MANY, ReferenceType.MANY_TO_MANY].includes(prop.reference)) {
return;
}

ret.push(this.helper.mapper(field, this.type) as string);
});

const meta = this.metadata.find(this.entityName);
Expand Down
2 changes: 1 addition & 1 deletion packages/knex/src/query/QueryBuilderHelper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,7 @@ export class QueryBuilderHelper {
return prop.fieldNames[0] ?? field;
}

private getProperty(field: string, alias?: string): EntityProperty | undefined {
getProperty(field: string, alias?: string): EntityProperty | undefined {
const entityName = this.aliasMap[alias!] || this.entityName;
const meta = this.metadata.find(entityName);

Expand Down
102 changes: 51 additions & 51 deletions tests/features/partial-loading/partial-loading.mysql.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,23 @@ describe('partial loading (mysql)', () => {
beforeEach(async () => wipeDatabaseMySql(orm.em));
afterAll(async () => orm.close(true));

async function createEntities() {
const god = new Author2(`God `, `hello@heaven.god`);
const b1 = new Book2(`Bible 1`, god);
b1.price = 123;
b1.tags.add(new BookTag2('t1'), new BookTag2('t2'));
const b2 = new Book2(`Bible 2`, god);
b2.price = 456;
b2.tags.add(new BookTag2('t3'), new BookTag2('t4'));
const b3 = new Book2(`Bible 3`, god);
b3.price = 789;
b3.tags.add(new BookTag2('t5'), new BookTag2('t6'));
await orm.em.persistAndFlush(god);
orm.em.clear();

return god;
}

test('partial selects', async () => {
const author = new Author2('Jon Snow', 'snow@wall.st');
author.born = new Date('1990-03-23');
Expand All @@ -25,23 +42,14 @@ describe('partial loading (mysql)', () => {
});

test('partial nested loading (1:m)', async () => {
const god = new Author2(`God `, `hello@heaven.god`);
const b1 = new Book2(`Bible 1`, god);
b1.price = 123;
const b2 = new Book2(`Bible 2`, god);
b2.price = 456;
const b3 = new Book2(`Bible 3`, god);
b3.price = 789;
await orm.em.persistAndFlush(god);
orm.em.clear();

const god = await createEntities();
const mock = mockLogger(orm, ['query']);

const r1 = await orm.em.find(Author2, god, { fields: ['id', 'books.author', 'books.title'] });
expect(r1).toHaveLength(1);
expect(r1[0].id).toBe(god.id);
expect(r1[0].name).toBeUndefined();
expect(r1[0].books[0].uuid).toBe(b1.uuid);
expect(r1[0].books[0].uuid).toBe(god.books[0].uuid);
expect(r1[0].books[0].title).toBe('Bible 1');
expect(r1[0].books[0].price).toBeUndefined();
expect(r1[0].books[0].author).toBeDefined();
Expand All @@ -54,25 +62,45 @@ describe('partial loading (mysql)', () => {
expect(r2).toHaveLength(1);
expect(r2[0].id).toBe(god.id);
expect(r2[0].name).toBeUndefined();
expect(r2[0].books[0].uuid).toBe(b1.uuid);
expect(r2[0].books[0].uuid).toBe(god.books[0].uuid);
expect(r2[0].books[0].title).toBe('Bible 1');
expect(r2[0].books[0].price).toBeUndefined();
expect(r2[0].books[0].author).toBeDefined();
expect(mock.mock.calls[0][0]).toMatch('select `a0`.`id` from `author2` as `a0` where `a0`.`id` = ?');
expect(mock.mock.calls[1][0]).toMatch('select `b0`.`uuid_pk`, `b0`.`author_id`, `b0`.`title` from `book2` as `b0` where `b0`.`author_id` is not null and `b0`.`author_id` in (?) order by `b0`.`title` asc');
});
orm.em.clear();
mock.mock.calls.length = 0;

test('partial nested loading (m:1)', async () => {
const god = new Author2(`God `, `hello@heaven.god`);
const b1 = new Book2(`Bible 1`, god);
b1.price = 123;
const b2 = new Book2(`Bible 2`, god);
b2.price = 456;
const b3 = new Book2(`Bible 3`, god);
b3.price = 789;
await orm.em.persistAndFlush(god);
// collection properties in `fields` are ignored
const r0 = await orm.em.find(Author2, god, { fields: ['id', 'books', 'books.author', 'books.title'] });
expect(r0).toHaveLength(1);
expect(r0[0].id).toBe(god.id);
expect(r0[0].name).toBeUndefined();
expect(r0[0].books[0].uuid).toBe(god.books[0].uuid);
expect(r0[0].books[0].title).toBe('Bible 1');
expect(r0[0].books[0].price).toBeUndefined();
expect(r0[0].books[0].author).toBeDefined();
expect(mock.mock.calls[0][0]).toMatch('select `a0`.`id` from `author2` as `a0` where `a0`.`id` = ?');
expect(mock.mock.calls[1][0]).toMatch('select `b0`.`uuid_pk`, `b0`.`author_id`, `b0`.`title` from `book2` as `b0` where `b0`.`author_id` is not null and `b0`.`author_id` in (?) order by `b0`.`title` asc');
orm.em.clear();
mock.mock.calls.length = 0;

// when populating collections, the owner is selected automatically (here book.author)
const r00 = await orm.em.find(Author2, god, { fields: ['id', 'books.title'] });
expect(r00).toHaveLength(1);
expect(r00[0].id).toBe(god.id);
expect(r00[0].name).toBeUndefined();
expect(r00[0].books[0].uuid).toBe(god.books[0].uuid);
expect(r00[0].books[0].title).toBe('Bible 1');
expect(r00[0].books[0].price).toBeUndefined();
expect(r00[0].books[0].author).toBeDefined();
expect(mock.mock.calls[0][0]).toMatch('select `a0`.`id` from `author2` as `a0` where `a0`.`id` = ?');
expect(mock.mock.calls[1][0]).toMatch('select `b0`.`uuid_pk`, `b0`.`title`, `b0`.`author_id` from `book2` as `b0` where `b0`.`author_id` is not null and `b0`.`author_id` in (?) order by `b0`.`title` asc');
});

test('partial nested loading (m:1)', async () => {
const god = await createEntities();
const b1 = god.books[0];
const mock = mockLogger(orm, ['query']);

const r1 = await orm.em.find(Book2, b1, { fields: ['uuid', 'title', 'author', 'author.email'], populate: ['author'], filters: false });
Expand Down Expand Up @@ -103,19 +131,7 @@ describe('partial loading (mysql)', () => {
});

test('partial nested loading (m:n)', async () => {
const god = new Author2(`God `, `hello@heaven.god`);
const b1 = new Book2(`Bible 1`, god);
b1.price = 123;
b1.tags.add(new BookTag2('t1'), new BookTag2('t2'));
const b2 = new Book2(`Bible 2`, god);
b2.price = 456;
b2.tags.add(new BookTag2('t3'), new BookTag2('t4'));
const b3 = new Book2(`Bible 3`, god);
b3.price = 789;
b3.tags.add(new BookTag2('t5'), new BookTag2('t6'));
await orm.em.persistAndFlush(god);
orm.em.clear();

await createEntities();
const mock = mockLogger(orm, ['query']);

const r1 = await orm.em.find(BookTag2, {}, { fields: ['name', 'books.title'], filters: false });
Expand All @@ -139,22 +155,6 @@ describe('partial loading (mysql)', () => {
expect(mock.mock.calls[1][0]).toMatch('select `b0`.`uuid_pk`, `b0`.`title`, `b1`.`book2_uuid_pk` as `fk__book2_uuid_pk`, `b1`.`book_tag2_id` as `fk__book_tag2_id`, `t2`.`id` as `test_id` from `book2` as `b0` left join `book2_tags` as `b1` on `b0`.`uuid_pk` = `b1`.`book2_uuid_pk` left join `test2` as `t2` on `b0`.`uuid_pk` = `t2`.`book_uuid_pk` where `b1`.`book_tag2_id` in (?) order by `b1`.`order` asc');
});

async function createEntities() {
const god = new Author2(`God `, `hello@heaven.god`);
const b1 = new Book2(`Bible 1`, god);
b1.price = 123;
b1.tags.add(new BookTag2('t1'), new BookTag2('t2'));
const b2 = new Book2(`Bible 2`, god);
b2.price = 456;
b2.tags.add(new BookTag2('t3'), new BookTag2('t4'));
const b3 = new Book2(`Bible 3`, god);
b3.price = 789;
b3.tags.add(new BookTag2('t5'), new BookTag2('t6'));
await orm.em.persistAndFlush(god);
orm.em.clear();
return god;
}

test('partial nested loading (dot notation)', async () => {
const god = await createEntities();
const mock = mockLogger(orm, ['query']);
Expand Down

0 comments on commit 3ddde1e

Please sign in to comment.