fix: minified aliases are now properly referenced in subqueries (v6) #14852

sdepold · 2022-08-07T14:58:04Z

Backport of #14804 to v6 + fix for mssql order duplication

Backport of #14804

WikiRik · 2022-08-07T16:59:06Z

Do we need the linting fixes? They're not in files related to this PR and I would prefer to limit the changes that we do in v6

sdepold · 2022-08-07T17:59:13Z

Do we need the linting fixes? They're not in files related to this PR and I would prefer to limit the changes that we do in v6

Well. I just ran lint --fix and this was the result. I wonder how it could have worked before since I expected the linter to fail if there are things to change

sdepold · 2022-08-07T18:50:55Z

I suppose I can remove the linting commit if we feel strong about it

This reverts commit efab1ea.

sdepold · 2022-08-07T19:01:19Z

@WikiRik better?

WikiRik · 2022-08-07T20:13:03Z

Linter still passes, so I'm fine with this. Thanks!

WikiRik · 2022-08-07T20:14:22Z

@tomquist Any chance you can test this on your project to make sure it works for your use case?

tomquist · 2022-08-08T13:31:14Z

@WikiRik Thanks! I tested it on our project. It all works fine if we enforce a subquery using subQuery:true 🎉.
However I ran into one bug now that when it isn't doing a subquery it doesn't minify the alias only at the place where the order by column is used.
E.g. this doesn't work:

await Foo.findAll({attributes: {include: [[sequelize.literal(`"Foo".my_name`), "order_0"]]}, order: ["order_0", "DESC"]})

The error is:

column _Foo.order_0 does not exist

WikiRik · 2022-08-08T14:45:43Z

@evanrittenhouse could you check this new case out?

tomquist · 2022-08-08T14:49:01Z

I think the issue I mentioned is an existing issue and nothing introduced by this bugfix. I can also open a new issue for the subQuery: false case if you like?

sdepold · 2022-08-10T10:59:36Z

@WikiRik Thanks! I tested it on our project. It all works fine if we enforce a subquery using subQuery:true 🎉. However I ran into one bug now that when it isn't doing a subquery it doesn't minify the alias only at the place where the order by column is used. E.g. this doesn't work:
await Foo.findAll({attributes: {include: [[sequelize.literal(`"Foo".my_name`), "order_0"]]}, order: ["order_0", "DESC"]})
The error is:

column _Foo.order_0 does not exist

Do I understand the code right, that you want to have a query that aliases Foo.myname as order_0 and then you want to sort by order_0?

With this code in place, we are currently generating

SELECT "id", "my_name" AS "_0", "Foo".my_name AS "_1" FROM "Foos" AS "Foo" ORDER BY "Foo"."order_0", "Foo"."DESC";

@tomquist ?

tomquist · 2022-08-10T11:32:59Z

Ah I see some brackets are missing. So the issue is that this:

await Foo.findAll({attributes: {include: [[sequelize.literal(`"Foo".my_name`), "order_0"]]}, order: [["order_0", "DESC"]]})

generates this query:

SELECT "id", "my_name" AS "_0", "Foo".my_name AS "_1" FROM "Foos" AS "Foo" ORDER BY "Foo"."order_0" DESC;

sdepold · 2022-08-10T11:57:15Z

@tomquist what would you expect instead?

tomquist · 2022-08-10T12:00:10Z

I'd expect this:

SELECT "id", "my_name" AS "_0", "Foo".my_name AS "_1" FROM "Foos" AS "Foo" ORDER BY "_1" DESC;

sdepold · 2022-08-10T13:01:27Z

Excellent. Thanks for the clarification

sdepold · 2022-08-10T19:12:22Z

Just pushed a change that might fix the issue. It's funny because I just removed a guard that was specifically targeting subQueries and enabled the logic in general.

sdepold · 2022-08-10T19:14:44Z

@tomquist @WikiRik @evanrittenhouse let's see if this works :)

sdepold · 2022-08-11T18:56:59Z

interesting. so mssql is additionally adding a sorting in it's query generator on top of the abstract order by fragments.

sdepold · 2022-08-11T19:07:42Z

@tomquist can you pleas test the new version? I'll figure out the mssql issue meanwhile

sdepold · 2022-08-12T08:35:03Z

src/dialects/mssql/query-generator.js

        if (!options.order || !options.order.length) {
          fragment += ` ORDER BY ${tablePkFragment}`;
        } else {
-          const orderFieldNames = _.map(options.order, order => order[0]);
-          const primaryKeyFieldAlreadyPresent = _.includes(orderFieldNames, model.primaryKeyField);
+          const orderFieldNames = (options.order || []).map(order => {


Do we have a function that is doing this already? Taking a name like thing and returns its string content?

sdepold · 2022-08-12T08:37:55Z

src/dialects/mssql/query-generator.js

+        let primaryKey = model.primaryKeyField;
+
+        const tablePkFragment = `${this.quoteTable(options.tableAs || model.name)}.${this.quoteIdentifier(primaryKey)}`;
+        const aliasedAttribute = (options.attributes || []).find(attr => Array.isArray(attr)


This is a copy from the generic query generator. I was about to refactor this function but then wondered if _getAliasForField should somehow take care of this logic. I'm still not 100% sure what we are even doing here but I supposed, that this piece of code combined with getAliasForField is basically catering for 1) model definition based aliases (using the field property?) and 2) aliases that are introduced during the query calls (e.g. findAll).

Maybe instead of doing this manually, we should have a bit of code inside of _getAliasForField that is checking the options and extracts the call alias from them?

sdepold · 2022-08-12T08:39:22Z

I'm a little bit lost on why MSSQL is having additional logic wrt order but it's probably there for good reasons. Anyway, because of this, it becomes a bit more convoluted and messy. We could clean up MSSQL but I wonder if that should happen in v7 only?

tomquist · 2022-08-12T08:43:34Z

@sdepold Just tested with af1a638, and it made all our tests pass 🥳 Thanks a lot!

sdepold · 2022-08-12T08:45:55Z

@sdepold Just tested with af1a638, and it made all our tests pass 🥳 Thanks a lot!

Sweet. Thanks for the feedback!

sdepold · 2022-08-12T08:49:08Z

src/dialects/mssql/query-generator.js

+          });
+          const primaryKeyFieldAlreadyPresent = orderFieldNames.some(
+            fieldName => fieldName === (primaryKey.col || primaryKey)
+          );

          if (!primaryKeyFieldAlreadyPresent) {
            fragment += options.order && !isSubQuery ? ', ' : ' ORDER BY ';


It's a bit crazy that this is done manually. I was tempted to somehow inject this idea into the incoming order options so that this.getQueryOrders would do the heavy lifting for us

sdepold · 2022-08-12T08:49:44Z

@WikiRik @evanrittenhouse ready for review

WikiRik · 2022-08-14T08:58:22Z

Let's review #14882 first and then get back to this one

sdepold · 2022-08-18T09:21:24Z

@WikiRik @evanrittenhouse can you review this guy?

WikiRik

Yeah, it's not the prettiest but it seems to work for v6 so we should be good

sdepold · 2022-08-18T11:00:20Z

Thanks Rik!

github-actions · 2022-08-18T11:43:49Z

🎉 This PR is included in version 6.21.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

pascuflow · 2022-09-27T22:32:03Z

Was this the only change from 6.20.1? Am getting "No context available. ns.run() or ns.bind() must be called first." error when using transactions.

ephys · 2022-09-28T06:37:24Z

Sounds like a continuation local storage issue, but we haven't touched that part of the code in recent releases

bminer · 2022-12-08T19:40:12Z

This PR seems to be causing an issue for me. See #15422

sdepold added 2 commits August 7, 2022 16:56

fix: minified aliases are now properly referenced in subqueries

136da31

Backport of #14804

fix: linting mistakes

efab1ea

sdepold requested a review from evanrittenhouse August 7, 2022 14:58

change syntax

9daa885

WikiRik mentioned this pull request Aug 7, 2022

Impossible to do literal order by with subQuery and minifyAliases enabled #14723

Closed

6 tasks

Revert "fix: linting mistakes"

a3ced4f

This reverts commit efab1ea.

fix: respect aliases without subQuery enabled

fb5c666

sdepold added 3 commits August 10, 2022 21:36

fix: unit tests

5a44589

fix: tmp

2d9fa3e

fix: should we disable this for mssql hm

edabe75

sdepold added 2 commits August 12, 2022 04:25

fix: adjust conditions for manual mssql ordering

40bb106

fix: don't add manual pk order if already exist

69fdeb1

sdepold added 2 commits August 12, 2022 05:34

fix: field name extraction

f33712c

remove unused file

af1a638

sdepold commented Aug 12, 2022

View reviewed changes

sdepold requested a review from WikiRik August 12, 2022 08:49

sdepold mentioned this pull request Aug 14, 2022

fix: use aliases if subQuery is not used (v7) #14882

Merged

WikiRik approved these changes Aug 18, 2022

View reviewed changes

sdepold requested a review from WikiRik August 18, 2022 10:59

sdepold merged commit 5a257bc into v6 Aug 18, 2022

sdepold deleted the order-by-minify-aliases-v6 branch August 18, 2022 11:00

github-actions bot added the released label Aug 18, 2022

sdepold mentioned this pull request Aug 18, 2022

How can we optimise work on paid / priority features/hot fixes? sequelize/meetings#26

Open

sdepold added the funded label Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: minified aliases are now properly referenced in subqueries (v6) #14852

fix: minified aliases are now properly referenced in subqueries (v6) #14852

sdepold commented Aug 7, 2022 •

edited

WikiRik commented Aug 7, 2022

sdepold commented Aug 7, 2022

sdepold commented Aug 7, 2022

sdepold commented Aug 7, 2022

WikiRik commented Aug 7, 2022

WikiRik commented Aug 7, 2022

tomquist commented Aug 8, 2022 •

edited

WikiRik commented Aug 8, 2022

tomquist commented Aug 8, 2022

sdepold commented Aug 10, 2022 •

edited

tomquist commented Aug 10, 2022

sdepold commented Aug 10, 2022

tomquist commented Aug 10, 2022

sdepold commented Aug 10, 2022

sdepold commented Aug 10, 2022

sdepold commented Aug 10, 2022

sdepold commented Aug 11, 2022

sdepold commented Aug 11, 2022

sdepold Aug 12, 2022

sdepold Aug 12, 2022

sdepold commented Aug 12, 2022

tomquist commented Aug 12, 2022

sdepold commented Aug 12, 2022

sdepold Aug 12, 2022

sdepold commented Aug 12, 2022 •

edited

WikiRik commented Aug 14, 2022

sdepold commented Aug 18, 2022

WikiRik left a comment

sdepold commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

pascuflow commented Sep 27, 2022

ephys commented Sep 28, 2022

bminer commented Dec 8, 2022

fix: minified aliases are now properly referenced in subqueries (v6) #14852

fix: minified aliases are now properly referenced in subqueries (v6) #14852

Conversation

sdepold commented Aug 7, 2022 • edited

WikiRik commented Aug 7, 2022

sdepold commented Aug 7, 2022

sdepold commented Aug 7, 2022

sdepold commented Aug 7, 2022

WikiRik commented Aug 7, 2022

WikiRik commented Aug 7, 2022

tomquist commented Aug 8, 2022 • edited

WikiRik commented Aug 8, 2022

tomquist commented Aug 8, 2022

sdepold commented Aug 10, 2022 • edited

tomquist commented Aug 10, 2022

sdepold commented Aug 10, 2022

tomquist commented Aug 10, 2022

sdepold commented Aug 10, 2022

sdepold commented Aug 10, 2022

sdepold commented Aug 10, 2022

sdepold commented Aug 11, 2022

sdepold commented Aug 11, 2022

sdepold Aug 12, 2022

Choose a reason for hiding this comment

sdepold Aug 12, 2022

Choose a reason for hiding this comment

sdepold commented Aug 12, 2022

tomquist commented Aug 12, 2022

sdepold commented Aug 12, 2022

sdepold Aug 12, 2022

Choose a reason for hiding this comment

sdepold commented Aug 12, 2022 • edited

WikiRik commented Aug 14, 2022

sdepold commented Aug 18, 2022

WikiRik left a comment

Choose a reason for hiding this comment

sdepold commented Aug 18, 2022

github-actions bot commented Aug 18, 2022

pascuflow commented Sep 27, 2022

ephys commented Sep 28, 2022

bminer commented Dec 8, 2022

sdepold commented Aug 7, 2022 •

edited

tomquist commented Aug 8, 2022 •

edited

sdepold commented Aug 10, 2022 •

edited

sdepold commented Aug 12, 2022 •

edited