-
-
Notifications
You must be signed in to change notification settings - Fork 497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(knex): allow changing FROM
clause using QueryBuilder
#3378
feat(knex): allow changing FROM
clause using QueryBuilder
#3378
Conversation
Amazing, thank you! Will try to review it later today.
Definitely, lets do that. We should also validate there is a FROM clause when building the query.
What is 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 Sidenote: we should add a PR template, what you did looks like a good start :] PR welcome for that as well :] |
50e5fdc
to
3a00280
Compare
@B4nan thanks for the feedback)
I suppose this is just a typo. Anyway, it has been fixed by the last commit 3a00280
To avoid any unwanted side-effect while mutation of
Should we handle this in the same PR? I believe it would be better to open a separate one. Wdyt?
Yeah, I will try to describe a few more test cases to verify that. |
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 ReportBase: 99.92% // Head: 99.89% // Decreases project coverage by
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
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. |
@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).
This makes sense, I don't mind. |
Or we could just validate this and throw, if you think it would be hard to remap them. |
@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 |
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 :] |
c19510a
to
7ed2d62
Compare
@B4nan Sorry for the delay) It is not possible easily to remap/override aliases (BTW neither 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. |
7ed2d62
to
323fd57
Compare
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! |
This pull request introduces 1 alert when merging 323fd57 into 8e9f6d9 - view on LGTM.com new alerts:
|
323fd57
to
02d70ca
Compare
@B4nan your comments are resolved including LGTM.com reported issue |
Information
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.You can also use sub-queries in the
FROM
like this:To set up an alias to refer to a table in a
SELECT
statement, pass the second argument as follows:Further improvements
To give the ability to create a clean query builder without
FROM
clause, we can change the signature of thecreateQueryBuilder
as follows:Thus, the example above can look more elegant:
closes #3374