Skip to content

Commit

Permalink
fix(sql): rework implicit m:n pivot joining
Browse files Browse the repository at this point in the history
  • Loading branch information
@B4nan
B4nan committed Aug 9, 2020
1 parent afdb437 commit 7928c50
Show file tree
Hide file tree
Showing 7 changed files with 71 additions and 33 deletions.
2 changes: 1 addition & 1 deletion packages/knex/src/AbstractSqlDriver.ts
View file
Expand Up @@ -100,7 +100,7 @@ export abstract class AbstractSqlDriver<C extends AbstractSqlConnection = Abstra
const relation = meta.properties[p.field as keyof T & string];
const meta2 = this.metadata.get(relation.type);
const path = parentJoinPath ? `${parentJoinPath}.${relation.name}` : `${meta.name}.${relation.name}`;
const relationAlias = qb.getAliasForJoinPath(relation.type, path);
const relationAlias = qb.getAliasForJoinPath(path);
const relationPojo = {};

// If the primary key value for the relation is null, we know we haven't joined to anything
Expand Down
23 changes: 19 additions & 4 deletions packages/knex/src/query/CriteriaNode.ts
View file
Expand Up @@ -79,9 +79,7 @@ export class CriteriaNode {

renameFieldToPK(qb: QueryBuilder): string {
if (this.prop!.reference === ReferenceType.MANY_TO_MANY) {
const pivotTable = this.prop!.pivotTable;
const alias = qb.getAliasForEntity(pivotTable, this);

const alias = qb.getAliasForJoinPath(this.getPath());
return Utils.getPrimaryKeyHash(this.prop!.inverseJoinColumns.map(col => `${alias}.${col}`));
}

Expand All @@ -90,7 +88,7 @@ export class CriteriaNode {
}

const meta = this.metadata.get(this.prop!.type);
const alias = qb.getAliasForEntity(meta.name, this);
const alias = qb.getAliasForJoinPath(this.getPath());
const pks = Utils.flatten(meta.primaryKeys.map(primaryKey => meta.properties[primaryKey].fieldNames));

return Utils.getPrimaryKeyHash(pks.map(col => `${alias}.${col}`));
Expand All @@ -113,9 +111,26 @@ export class CriteriaNode {
}
}

if (!this.key || !this.prop) {
return ret;
}

const customExpression = QueryBuilderHelper.isCustomExpression(this.key);
const scalar = Utils.isPrimaryKey(this.payload) || this.payload instanceof RegExp || this.payload instanceof Date || customExpression;
const operator = Utils.isObject(this.payload) && Object.keys(this.payload).every(k => Utils.isOperator(k, false));
const pivotJoin = this.prop.reference === ReferenceType.MANY_TO_MANY && (scalar || operator);

if (pivotJoin) {
return this.getPivotPath(ret);
}

return ret;
}

getPivotPath(path: string): string {
return path + '[pivot]';
}

[inspect.custom]() {
return `${this.constructor.name} ${inspect({ entityName: this.entityName, key: this.key, payload: this.payload })}`;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/knex/src/query/ObjectCriteriaNode.ts
View file
Expand Up @@ -33,7 +33,7 @@ export class ObjectCriteriaNode extends CriteriaNode {
}

process(qb: QueryBuilder, alias?: string): any {
const nestedAlias = qb.getAliasForEntity(this.entityName, this);
const nestedAlias = qb.getAliasForJoinPath(this.getPath());
const ownerAlias = alias || qb.alias;

if (nestedAlias) {
Expand Down Expand Up @@ -65,7 +65,7 @@ export class ObjectCriteriaNode extends CriteriaNode {
}

willAutoJoin(qb: QueryBuilder, alias?: string): boolean {
const nestedAlias = qb.getAliasForEntity(this.entityName, this);
const nestedAlias = qb.getAliasForJoinPath(this.getPath());
const ownerAlias = alias || qb.alias;

if (nestedAlias) {
Expand Down
26 changes: 10 additions & 16 deletions packages/knex/src/query/QueryBuilder.ts
View file
Expand Up @@ -249,23 +249,11 @@ export class QueryBuilder<T extends AnyEntity<T> = AnyEntity> {
return this.getKnexQuery().toSQL().toNative().bindings;
}

getAliasForEntity(entityName: string, node: CriteriaNode): string | undefined {
if (node.prop) {
const join = Object.values(this._joins).find(j => j.path === node.getPath());

if (!join) {
return undefined;
}

return join.inverseAlias || join.alias;
getAliasForJoinPath(path: string): string | undefined {
if (path === this.entityName) {
return this.alias;
}

const found = Object.entries(this._aliasMap).find(([, e]) => e === entityName);

return found ? found[0] : undefined;
}

getAliasForJoinPath(entityName: string, path: string): string | undefined {
const join = Object.values(this._joins).find(j => j.path === path);
/* istanbul ignore next */
return join?.inverseAlias || join?.alias;
Expand Down Expand Up @@ -354,7 +342,13 @@ export class QueryBuilder<T extends AnyEntity<T> = AnyEntity> {
if (prop.reference === ReferenceType.ONE_TO_MANY) {
this._joins[aliasedName] = this.helper.joinOneToReference(prop, fromAlias, alias, type, cond);
} else if (prop.reference === ReferenceType.MANY_TO_MANY) {
const pivotAlias = type === 'pivotJoin' ? alias : `e${this.aliasCounter++}`;
let pivotAlias = alias;

if (type !== 'pivotJoin') {
const oldPivotAlias = this.getAliasForJoinPath(path + '[pivot]');
pivotAlias = oldPivotAlias ?? `e${this.aliasCounter++}`;
}

const joins = this.helper.joinManyToManyReference(prop, fromAlias, alias, pivotAlias, type, cond);
Object.assign(this._joins, joins);
this._aliasMap[pivotAlias] = prop.pivotTable;
Expand Down
2 changes: 1 addition & 1 deletion packages/knex/src/query/ScalarCriteriaNode.ts
View file
Expand Up @@ -12,7 +12,7 @@ export class ScalarCriteriaNode extends CriteriaNode {

process(qb: QueryBuilder, alias?: string): any {
if (this.shouldJoin()) {
const nestedAlias = qb.getAliasForEntity(this.entityName, this) || qb.getNextAlias();
const nestedAlias = qb.getAliasForJoinPath(this.getPath()) || qb.getNextAlias();
const field = `${alias}.${this.prop!.name}`;

if (this.prop!.reference === ReferenceType.MANY_TO_MANY) {
Expand Down
42 changes: 36 additions & 6 deletions tests/EntityManager.mysql.test.ts
View file
Expand Up @@ -1319,8 +1319,7 @@ describe('EntityManagerMySql', () => {
await expect(tags.map(tag => tag.booksUnordered.count())).toEqual([1, 1, 1, 1, 2, 2]);
});

// TODO need to rework pivot joins, this was resulting in false positives (wrong alias in the order by clause)
test.skip('self referencing M:N (unidirectional)', async () => {
test('self referencing M:N (unidirectional)', async () => {
const a1 = new Author2('A1', 'a1@wall.st');
const a2 = new Author2('A2', 'a2@wall.st');
const a3 = new Author2('A3', 'a3@wall.st');
Expand All @@ -1329,18 +1328,50 @@ describe('EntityManagerMySql', () => {
await orm.em.persistAndFlush(author);
orm.em.clear();

const mock = jest.fn();
const logger = new Logger(mock, true);
Object.assign(orm.config, { logger });

const jon = await orm.em.findOneOrFail(Author2, author.id, ['friends'], { friends: { name: QueryOrder.ASC } });
expect(jon.friends.isInitialized(true)).toBe(true);
expect(jon.friends.getIdentifiers()).toEqual([a1.id, a2.id, a3.id, author.id]);
expect(mock.mock.calls[0][0]).toMatch('select `e0`.*, `e3`.`author_id` as `address_author_id` ' +
'from `author2` as `e0` ' +
'left join `author_to_friend` as `e2` on `e0`.`id` = `e2`.`author2_1_id` ' +
'left join `author2` as `e1` on `e2`.`author2_2_id` = `e1`.`id` ' +
'left join `address2` as `e3` on `e0`.`id` = `e3`.`author_id` ' +
'where `e0`.`id` = ? ' +
'order by `e1`.`name` asc ' +
'limit ?');
expect(mock.mock.calls[1][0]).toMatch('select `e0`.*, `e1`.`author2_2_id`, `e1`.`author2_1_id`, `e2`.`author_id` as `address_author_id` ' +
'from `author2` as `e0` ' +
'left join `author_to_friend` as `e1` on `e0`.`id` = `e1`.`author2_2_id` ' +
'left join `address2` as `e2` on `e0`.`id` = `e2`.`author_id` ' +
'where `e1`.`author2_1_id` in (?) ' +
'order by `e0`.`name` asc');
orm.em.clear();

const jon2 = await orm.em.findOneOrFail(Author2, { friends: a2.id }, ['friends'], { friends: { name: QueryOrder.ASC } });
expect(mock.mock.calls[2][0]).toMatch('select `e0`.*, `e3`.`author_id` as `address_author_id` ' +
'from `author2` as `e0` ' +
'left join `author_to_friend` as `e1` on `e0`.`id` = `e1`.`author2_1_id` ' +
'left join `author2` as `e2` on `e1`.`author2_2_id` = `e2`.`id` ' +
'left join `address2` as `e3` on `e0`.`id` = `e3`.`author_id` ' +
'where `e1`.`author2_2_id` = ? ' +
'order by `e2`.`name` asc ' +
'limit ?');
expect(mock.mock.calls[3][0]).toMatch('select `e0`.*, `e1`.`author2_2_id`, `e1`.`author2_1_id`, `e2`.`author_id` as `address_author_id` ' +
'from `author2` as `e0` ' +
'left join `author_to_friend` as `e1` on `e0`.`id` = `e1`.`author2_2_id` ' +
'left join `address2` as `e2` on `e0`.`id` = `e2`.`author_id` ' +
'where `e1`.`author2_1_id` in (?) ' +
'order by `e0`.`name` asc');
expect(jon2.id).toBe(author.id);
expect(jon2.friends.isInitialized(true)).toBe(true);
expect(jon2.friends.getIdentifiers()).toEqual([a1.id, a2.id, a3.id, author.id]);
});

// TODO need to rework pivot joins, this was resulting in false positives (wrong alias in the order by clause)
test.skip('self referencing M:N (owner)', async () => {
test('self referencing M:N (owner)', async () => {
const a1 = new Author2('A1', 'a1@wall.st');
const a2 = new Author2('A2', 'a2@wall.st');
const a3 = new Author2('A3', 'a3@wall.st');
Expand All @@ -1360,8 +1391,7 @@ describe('EntityManagerMySql', () => {
expect(jon2.following.getIdentifiers()).toEqual([a1.id, a2.id, a3.id, author.id]);
});

// TODO need to rework pivot joins, this was resulting in false positives (wrong alias in the order by clause)
test.skip('self referencing M:N (inverse)', async () => {
test('self referencing M:N (inverse)', async () => {
const a1 = new Author2('A1', 'a1@wall.st');
const a2 = new Author2('A2', 'a2@wall.st');
const a3 = new Author2('A3', 'a3@wall.st');
Expand Down
5 changes: 2 additions & 3 deletions tests/QueryBuilder.test.ts
View file
Expand Up @@ -1288,12 +1288,11 @@ describe('QueryBuilder', () => {
expect(inspect(node)).toBe(`CriteriaNode { entityName: 'Author2', key: undefined, payload: { foo: 123 } }`);
});

test('getAliasForEntity', async () => {
test('getAliasForJoinPath', async () => {
const node = new CriteriaNode(orm.em.getMetadata(), Author2.name);
node.payload = { foo: 123 };
const qb = orm.em.createQueryBuilder(Author2, 'a');
expect(qb.getAliasForEntity(Author2.name, node)).toBe('a');
expect(qb.getAliasForEntity(Book2.name, node)).toBeUndefined();
expect(qb.getAliasForJoinPath(node.getPath())).toBe('a');
});

afterAll(async () => orm.close(true));
Expand Down

0 comments on commit 7928c50

Please sign in to comment.