fix(sql): sort fetch-joined properties on their orderBy #1336

TiesB · 2021-01-23T11:15:23Z

Merges a joined relation's orderBy with the parent's, keeping tableAliases in mind. Fixes #1331

B4nan

thanks! the fix itself looks fine, but i don't really like the way it is coded inside existing methods with different purposes. should be refactored based on my comments, but if you would struggle with that, I will be happy to merge as is and refactor it myself

B4nan · 2021-01-23T12:04:00Z

packages/knex/src/AbstractSqlDriver.ts

@@ -39,7 +39,8 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
    const populate = this.autoJoinOneToOneOwner(meta, options.populate as PopulateOptions<T>[], options.fields);
    const joinedProps = this.joinedProps(meta, populate);
    const qb = this.createQueryBuilder<T>(entityName, ctx, !!ctx, false);
-    const fields = this.buildFields(meta, populate, joinedProps, qb, options.fields as Field<T>[]);
+    const fieldsAndJoinedPropsOrderBy = this.buildFields(meta, populate, joinedProps, qb, options.fields as Field<T>[]);


I don't really like huge property names, especially if they contain conjunctions. Why dont we destruct instead?

Suggested change

const fieldsAndJoinedPropsOrderBy = this.buildFields(meta, populate, joinedProps, qb, options.fields as Field<T>[]);

const { fields, orderBy } = this.buildFields(meta, populate, joinedProps, qb, options.fields as Field<T>[]);

But I would much rather see a dedicated method - this one is called buildFields, it should not have anything to do with ordering.

B4nan · 2021-01-23T12:04:57Z

packages/knex/src/AbstractSqlDriver.ts

@@ -393,8 +394,8 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
    orderBy = this.getPivotOrderBy(prop, orderBy);
    const qb = this.createQueryBuilder<T>(prop.type, ctx, !!ctx).unsetFlag(QueryFlag.CONVERT_CUSTOM_TYPES);
    const populate = this.autoJoinOneToOneOwner(targetMeta, [{ field: prop.pivotTable }]);
-    const fields = this.buildFields(targetMeta, (options?.populate ?? []) as PopulateOptions<T>[], [], qb, options?.fields as Field<T>[]);
-    qb.select(fields).populate(populate).where(where).orderBy(orderBy!);
+    const fieldsAndJoinedPropsOrderBy = this.buildFields(targetMeta, (options?.populate ?? []) as PopulateOptions<T>[], [], qb, options?.fields as Field<T>[]);


B4nan · 2021-01-23T12:07:15Z

packages/knex/src/AbstractSqlDriver.ts

@@ -457,9 +458,10 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
    return res;
  }

-  protected getFieldsForJoinedLoad<T extends AnyEntity<T>>(qb: QueryBuilder<T>, meta: EntityMetadata<T>, populate: PopulateOptions<T>[] = [], parentTableAlias?: string, parentJoinPath?: string): Field<T>[] {
+  protected getFieldsAndOrderByForJoinedLoad<T extends AnyEntity<T>>(qb: QueryBuilder<T>, meta: EntityMetadata<T>, populate: PopulateOptions<T>[] = [], parentTableAlias?: string, parentJoinPath?: string): { fields: Field<T>[]; orderBy: QueryOrderMap } {


same here, having conjunctions inside method name feels even worse that in a property name...

B4nan · 2021-01-23T12:09:58Z

packages/knex/src/AbstractSqlDriver.ts

+        }
+      }
+      fields.push(...propFieldsAndOrderBy.fields);
+      orderBy = { ...orderBy, ...propOrderBy, ...propFieldsAndOrderBy.orderBy };


you should not need to reassign the variable here. this is equal to the following, right?

Suggested change

orderBy = { ...orderBy, ...propOrderBy, ...propFieldsAndOrderBy.orderBy };

Object.assign(orderBy, { ...propOrderBy, ...propFieldsAndOrderBy.orderBy };

tests/issues/GH1331.test.ts

B4nan · 2021-01-23T12:13:52Z

tests/issues/GH1331.test.ts

+    await orm.close(true);
+  });
+
+  test(`GH issue 1278`, async () => {


wrong issue number

tests/issues/GH1331.test.ts

B4nan · 2021-01-23T12:16:56Z

tests/issues/GH1331.test.ts

+    const mock = jest.fn();
+    const logger = new Logger(mock, ['query']);
+    Object.assign(orm.config, { logger });


this code is not used anyhow. i am curious, as large portion of repros contain it - where/why did you copy it from?

Suggested change

const mock = jest.fn();

const logger = new Logger(mock, ['query']);

Object.assign(orm.config, { logger });

Oh yeah I copied GH1278.test.ts and the code was in there. I have no idea who originally wrote it.

I probably forgot to remove it myself. Thanks @TiesB for fixing.

I am quite sure it was me :] I am using this exact snippet on many places in tests, not just in repros (oh yeah, I should have a helper method long time ago, lol). But as long as there are no asserts on the mocked logger, there is no need for mocking it.

B4nan · 2021-01-23T12:17:46Z

tests/issues/GH1331.test.ts

+    await orm.em.persistAndFlush(project);
+    orm.em.clear();
+
+    const loadedProject = await orm.em.findOne(Project, { id: 1 });


use findOneOrFail so the ! is not needed

Suggested change

const loadedProject = await orm.em.findOne(Project, { id: 1 });

const loadedProject = await orm.em.findOneOrFail(Project, project.id);

B4nan · 2021-01-23T12:25:17Z

tests/issues/GH1331.test.ts

+
+    const loadedProject = await orm.em.findOne(Project, { id: 1 });
+    expect(loadedProject?.radios.getItems().map(r => r.order)).toStrictEqual([0, 1, 2]);
+    expect(loadedProject?.radios.getItems()[0].options.getItems().map(r => r.order).toString()).toBe([1, 2, 3, 4].toString());


this looks also very hard to read...

Suggested change

expect(loadedProject?.radios.getItems()[0].options.getItems().map(r => r.order).toString()).toBe([1, 2, 3, 4].toString());

expect(loadedProject.radios[0].options.getIdentifiers('order').toEqual([1, 2, 3, 4]);

TiesB · 2021-01-23T14:14:46Z

Thanks a lot for the feedback! I refactored it to one single function and applied your other suggestions (I hope correctly ;-) ). I was not sure about how to deal with the orderBy in the loadFromPivotTable method; it would be great if you could look at that.

B4nan

looking much cleaner now!

B4nan · 2021-01-23T14:33:52Z

packages/knex/src/AbstractSqlDriver.ts

@@ -40,6 +40,7 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
    const joinedProps = this.joinedProps(meta, populate);
    const qb = this.createQueryBuilder<T>(entityName, ctx, !!ctx, false);
    const fields = this.buildFields(meta, populate, joinedProps, qb, options.fields as Field<T>[]);
+    const orderBy = this.fillInOrderBy(entityName, qb, meta, options.orderBy!, joinedProps);


I was expecting something like buildOrderBy, and I'd rather not mutate the exiting options.orderBy there.

Renamed it to buildJoinedPropsOrderBy now, since I feel buildOrderBy is not right because it doesn't touch the options.orderBy anymore.

B4nan · 2021-01-23T14:38:22Z

packages/knex/src/AbstractSqlDriver.ts

@@ -583,6 +584,19 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
    await this.rethrow(qb.execute());
  }

+  protected fillInOrderBy<T extends AnyEntity<T>>(entityName: string, qb: QueryBuilder<T>, meta: EntityMetadata<T>, orderBy: QueryOrderMap, joinedProps: PopulateOptions<T>[]): QueryOrderMap {
+    joinedProps.forEach(prop => {
+      const propAlias = qb.getAliasForJoinPath(`${entityName}.${prop.field}`);


this probably won't work with more than one level. the join path always starts with root entity name, but for if you would load author, its books and their tags, it would be something like Author.books.tags

would be good to add some tests for such use case

Very good point! I changed it such that it can handle nested relations (I looked at getFieldsForJoinedLoad() for reference) and extended the test with a level deeper relation

What is intriguing me is the amount of queries executed; a query is added for retrieving the 'ds', even though they are also retrieved with the 2nd massive query. Do you know why this is and whether it's needed?

select `e0`.* from `a` as `e0` where `e0`.`id` = ? limit ? select `e0`.`id`, `e0`.`order`, `e0`.`a_id`, `c1`.`id` as `c1__id`, `c1`.`order` as `c1__order`, `c1`.`b_id` as `c1__b_id`, `d2`.`id` as `d2__id`, `d2`.`order` as `d2__order`, `d2`.`c_id` as `d2__c_id` from `b` as `e0` left join `c` as `c1` on `e0`.`id` = `c1`.`b_id` left join `d` as `d2` on `c1`.`id` = `d2`.`c_id` where `e0`.`a_id` in (?) order by `e0`.`order` asc, `e0`.`id` asc, `c1`.`order` asc, `c1`.`id` asc, `d2`.`order` asc, `d2`.`id` asc select `e0`.* from `d` as `e0` where `e0`.`c_id` in (?, ?, ?, ?, ?, ?, ?, ?) order by `e0`.`order` asc, `e0`.`id` asc

hard to say, might be some other issue, either with reference wrapper, or eager: true combined with joined strategy

B4nan · 2021-01-23T14:49:33Z

Regarding the loadFromPivotTable() method, it should be used only for select-in strategy (or when lazy loading m:n collections via init() method).

TiesB · 2021-01-23T16:38:04Z

I am running into the 500loc limit in codeclimate. I'd like to leave the refactoring to get the loc within limits to you, since you know the code waaayy better than I do :-)

Regarding the loadFromPivotTable() method, it should be used only for select-in strategy (or when lazy loading m:n collections via init() method).

Cool. Then I'll leave that untouched.

B4nan · 2021-01-23T16:39:23Z

Sure, don't worry too much about codeclimate.

Langstra mentioned this pull request Jan 23, 2021

test(loadstrategy): added failing test for LoadingStrategy.JOINED #1332

Closed

TiesB force-pushed the master branch from 1270e05 to a960995 Compare January 23, 2021 11:24

B4nan requested changes Jan 23, 2021

View reviewed changes

test(loadstrategy): added failing test for LoadingStrategy.JOINED

6837201

TiesB force-pushed the master branch from a960995 to 45e660a Compare January 23, 2021 14:09

TiesB requested a review from B4nan January 23, 2021 14:40

B4nan requested changes Jan 23, 2021

View reviewed changes

TiesB force-pushed the master branch from 45e660a to ffc1616 Compare January 23, 2021 16:10

fix(sql): sort fetch-joined properties on their orderBy

ee273e5

TiesB force-pushed the master branch from ffc1616 to ee273e5 Compare January 23, 2021 16:17

TiesB requested a review from B4nan January 23, 2021 16:38

B4nan approved these changes Jan 23, 2021

View reviewed changes

B4nan merged commit f18cd88 into mikro-orm:master Jan 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql): sort fetch-joined properties on their orderBy #1336

fix(sql): sort fetch-joined properties on their orderBy #1336

TiesB commented Jan 23, 2021

B4nan left a comment •

edited

Loading

B4nan Jan 23, 2021

B4nan Jan 23, 2021

B4nan Jan 23, 2021

B4nan Jan 23, 2021

B4nan Jan 23, 2021

B4nan Jan 23, 2021

Langstra Jan 23, 2021

B4nan Jan 23, 2021

B4nan Jan 23, 2021

B4nan Jan 23, 2021

TiesB commented Jan 23, 2021

B4nan left a comment

B4nan Jan 23, 2021

TiesB Jan 23, 2021

B4nan Jan 23, 2021

TiesB Jan 23, 2021

TiesB Jan 23, 2021

B4nan Jan 23, 2021

B4nan commented Jan 23, 2021

TiesB commented Jan 23, 2021

B4nan commented Jan 23, 2021

	const fieldsAndJoinedPropsOrderBy = this.buildFields(meta, populate, joinedProps, qb, options.fields as Field<T>[]);
	const { fields, orderBy } = this.buildFields(meta, populate, joinedProps, qb, options.fields as Field<T>[]);

	orderBy = { ...orderBy, ...propOrderBy, ...propFieldsAndOrderBy.orderBy };
	Object.assign(orderBy, { ...propOrderBy, ...propFieldsAndOrderBy.orderBy };

	const mock = jest.fn();
	const logger = new Logger(mock, ['query']);
	Object.assign(orm.config, { logger });

	const loadedProject = await orm.em.findOne(Project, { id: 1 });
	const loadedProject = await orm.em.findOneOrFail(Project, project.id);

	expect(loadedProject?.radios.getItems()[0].options.getItems().map(r => r.order).toString()).toBe([1, 2, 3, 4].toString());
	expect(loadedProject.radios[0].options.getIdentifiers('order').toEqual([1, 2, 3, 4]);

fix(sql): sort fetch-joined properties on their orderBy #1336

fix(sql): sort fetch-joined properties on their orderBy #1336

Conversation

TiesB commented Jan 23, 2021

B4nan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TiesB commented Jan 23, 2021

B4nan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

B4nan commented Jan 23, 2021

TiesB commented Jan 23, 2021

B4nan commented Jan 23, 2021

B4nan left a comment •

edited

Loading