Skip to content
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

fix(knex): order by with a formula field should not include as for sub-queries #2929

Merged
merged 1 commit into from
Mar 19, 2022

Conversation

tsangste
Copy link
Contributor

follow on from #2848, noticed for sub-queries it was still adding as to formula field on order by

@@ -505,7 +505,7 @@ export class QueryBuilderHelper {

Utils.splitPrimaryKeys(field).forEach(f => {
const prop = this.getProperty(f, alias);
const noPrefix = (prop && prop.persist === false) || QueryBuilderHelper.isCustomExpression(f);
const noPrefix = (prop && prop.persist === false && !prop.formula) || QueryBuilderHelper.isCustomExpression(f);
Copy link
Contributor Author

@tsangste tsangste Mar 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need advice on this one, I added this because on the test it the order was coming out as e0.priceTaxed and not the formula. It would cause the mapper to not correctly resolve the field as a formula as it loses information that the property belongs to books and not e0.

Copy link
Member

@B4nan B4nan Mar 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

formulas should be marked with persist: false already, i dont mind the explicit check

edit: i see, this is not really about that

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2022

Codecov Report

Merging #2929 (d0af02a) into master (61d1e85) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #2929   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          193       193           
  Lines        11782     11782           
  Branches      2718      2718           
=========================================
  Hits         11782     11782           
Impacted Files Coverage Δ
packages/knex/src/query/QueryBuilder.ts 100.00% <ø> (ø)
packages/knex/src/query/QueryBuilderHelper.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61d1e85...d0af02a. Read the comment docs.

@B4nan B4nan merged commit 74751fb into mikro-orm:master Mar 19, 2022
@tsangste tsangste deleted the sub-query-order-by-formula branch March 19, 2022 10:47
@Lemas97
Copy link

Lemas97 commented Oct 23, 2023

Btw, there is the same issue on group by

B4nan added a commit that referenced this pull request Oct 23, 2023
@B4nan
Copy link
Member

B4nan commented Oct 23, 2023

e27e4b9 should help with that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants