feat(knex): allow changing FROM clause using QueryBuilder

[derevnjuk]

Information

Type	Breaking change
Feat	No

Usage example

You can specify the table used in the FROM clause, replacing the current table name if one has already been specified. This is typically used to specify a sub-query expression in SQL.

const qb = em.createQueryBuilder(Book2);
qb.select('*').from(Author2).where({ id: { $gt: 2 } });

console.log(qb.getQuery());
// select `e0`.* from `author2` as `e0` where `e0`.`id` > 2;

You can also use sub-queries in the FROM like this:

const qb1 = em.createQueryBuilder(Book2).where({ id: { $lte: new Date() } }).orderBy({ id: 'DESC' }).limit(10);
const qb2 = em.createQueryBuilder(qb1.clone())
qb2.select('*').orderBy({ id: 'ASC' });

console.log(qb2.getQuery());
// select `e1`.* from (select `e0`.* from `book2` as `e0` where `e0`.`id` <= ? order by `e0`.`id` desc limit ?) as `e1` order by `e1`.`id`;

To set up an alias to refer to a table in a SELECT statement, pass the second argument as follows:

const qb1 = em.createQueryBuilder(Book2, 'b1').where({ id: { $lte: new Date() } }).orderBy({ id: 'DESC' }).limit(10);
const qb2 = em.createQueryBuilder(qb1.clone(), 'b2')
qb2.select('*').orderBy({ id: 'ASC' });

console.log(qb2.getQuery());
// select `b2`.* from (select `b1`.* from `book2` as `b1` where `b1`.`id` <= ? order by `b1`.`id` desc limit ?) as `b2` order by `b2`.`id`;

Further improvements

To give the ability to create a clean query builder without FROM clause, we can change the signature of the createQueryBuilder as follows:

- createQueryBuilder<T>(entityName: EntityName<T>, alias?: string, type?: ConnectionType): QueryBuilder<T> {
+ createQueryBuilder<T>(entityName?: EntityName<T>, alias?: string, type?: ConnectionType): QueryBuilder<T> {

Thus, the example above can look more elegant:

const qb1 = em.createQueryBuilder(Book2, 'b1').where({ id: { $lte: new Date() } }).orderBy({ id: 'DESC' }).limit(10);
const qb2 = em.createQueryBuilder().from(qb.clone(), 'b2')
qb.select('*').orderBy({ id: 'ASC' });

closes #3374

[B4nan]

Amazing, thank you! Will try to review it later today.

To give the ability to create a clean query builder without FROM clause, we can change the signature of the createQueryBuilder as follows:

Definitely, lets do that. We should also validate there is a FROM clause when building the query.

const qb1 = em.createQueryBuilder(Book2, 'b1').where({ id: { $lte: new Date() } }).orderBy({ id: 'DESC' }).limit(10);
const qb2 = em.createQueryBuilder().from(qb.clone(), 'b2')
qb.select('*').orderBy({ id: 'ASC' });

What is qb? Is that third QB instance? Why do you clone it?

Also one thing to keep in mind with this, the root alias will be created when QB instance gets created, but could be changed later via the from() call. We might need to do some cleanup/propagation if the root alias was already used somewhere. Let's add some tests for that to see how it even behaves now.

---

Sidenote: we should add a PR template, what you did looks like a good start :] PR welcome for that as well :]

[derevnjuk]

@B4nan thanks for the feedback)

What is qb? Is that third QB instance?

I suppose this is just a typo. Anyway, it has been fixed by the last commit 3a00280

Why do you clone it?

To avoid any unwanted side-effect while mutation of QueryBuilder. For sure, we have provided for this by calling getKnexQuery() instead of using the link to the instance of QueryBuilder. Yet, I don't think the user should think about it, better to give him the right instructions)

Definitely, lets do that. We should also validate there is a FROM clause when building the query.

Should we handle this in the same PR? I believe it would be better to open a separate one. Wdyt?

Let's add some tests for that to see how it even behaves now.

Yeah, I will try to describe a few more test cases to verify that.

[B4nan]

I'd handle it in this one so we have a single item for this feature in the changelog, it should be rather easy. But it can be a separate PR that targets this one if you want, and we can merge that one first.

[codecov-commenter]

Codecov Report

Base: 99.92% // Head: 99.89% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (323fd57) compared to base (af8825e).
Patch coverage: 95.74% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3378      +/-   ##
==========================================
- Coverage   99.92%   99.89%   -0.04%     
==========================================
  Files         205      206       +1     
  Lines       12773    12818      +45     
  Branches     2965     2972       +7     
==========================================
+ Hits        12764    12805      +41     
- Misses          9       13       +4     

Impacted Files	Coverage Δ
packages/knex/src/query/QueryBuilder.ts	99.28% <95.34%> (-0.72%)	⬇️
packages/knex/src/AbstractSqlDriver.ts	99.59% <100.00%> (-0.01%)	⬇️
packages/knex/src/SqlEntityManager.ts	100.00% <100.00%> (ø)
packages/knex/src/query/Alias.ts	100.00% <100.00%> (ø)
packages/knex/src/query/QueryBuilderHelper.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[derevnjuk]

@B4nan I have added a few more tests (4e837db). Unfortunately, you are right. We should somehow override aliases once they have been used to build some clauses (e.g. join, where, order by, having, etc).

I'd handle it in this one so we have a single item for this feature in the changelog

This makes sense, I don't mind.

[B4nan]

Or we could just validate this and throw, if you think it would be hard to remap them.

[derevnjuk]

@B4nan this is how one of the options. Anyway, I will check out if we have another option that does not imply a significant reworking of CriteriaNode and etc.

[B4nan]

Any updates on this? As mentioned above, I am fine with validations for the problematic case, we dont need to be too smart in the initial implementation. Would be good to merge this before conflicts start to pop :]

[derevnjuk]

@B4nan Sorry for the delay) It is not possible easily to remap/override aliases (BTW neither knex nor typeorm does this). It may require rewriting the whole logic of QueryBuilder by building clauses at the final stage as follows:
Before:

orderBy(orderBy: QBQueryOrderMap<T> | QBQueryOrderMap<T>[]): this {
  this._orderBy = [];
  Utils.asArray(orderBy).forEach(o => {
    const processed = QueryHelper.processWhere({
      where: o as Dictionary,
      entityName: this.mainAlias.entityName,
      metadata: this.metadata,
      platform: this.platform,
      aliasMap: this.getAliasMap(),
      aliased: !this.type || [QueryType.SELECT, QueryType.COUNT].includes(this.type),
      convertCustomTypes: false,
    })!;
    this._orderBy.push(CriteriaNodeFactory.createNode(this.metadata, this.mainAlias.entityName, processed).process(this));
  });

  return this;
}

getKnexQuery(): Knex.QueryBuilder {
  // ...
  Utils.runIfNotEmpty(() => {
      const queryOrder = this.helper.getQueryOrder(this.type, this._orderBy as FlatQueryOrderMap[], this._populateMap);

      if (queryOrder) {
        return qb.orderByRaw(queryOrder);
      }
  }, this._orderBy);
  // ...
}

After:

orderBy(orderBy: QBQueryOrderMap<T> | QBQueryOrderMap<T>[]): this {
  this._orderBy = Utils.asArray(orderBy);

  return this;
}

// ...

getKnexQuery(): Knex.QueryBuilder {
  // ...
  Utils.runIfNotEmpty(() => {
    const nodes = this._orderBy.map((o) => {
      const processed = QueryHelper.processWhere({
        where: o as Dictionary,
        entityName: this.mainAlias.entityName,
        metadata: this.metadata,
        platform: this.platform,
        aliasMap: this.getAliasMap(),
        aliased: !this.type || [QueryType.SELECT, QueryType.COUNT].includes(this.type),
        convertCustomTypes: false,
      })!;
    
      return CriteriaNodeFactory.createNode(this.metadata, this.mainAlias.entityName, processed).process(this);
    });
    const queryOrder = this.helper.getQueryOrder(this.type, nodes as FlatQueryOrderMap[], this._populateMap);
    
    if (queryOrder) {
      return qb.orderByRaw(queryOrder);
    }
  }, this._orderBy);
  // ...
}

So, I propose to open a separate issue to refactor clause by clause sequentially.

[B4nan]

So, I propose to open a separate issue to refactor clause by clause sequentially.

I was trying to implement that some time ago, its not that simple, as with everything... :]

I am fine with keeping this edge case not supported. This is already a huge improvement, thanks a lot!

[lgtm-com]

This pull request introduces 1 alert when merging 323fd57 into 8e9f6d9 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

[derevnjuk]

@B4nan your comments are resolved including LGTM.com reported issue