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

[sdepold]

backport of #14852

please note that particularly the second commit is very odd since it's unclear to me why we are doing so much additional manual work for mssql.

during my attempts on refactoring I stumbled across this test code:

`SELECT * FROM (
               SELECT [user].[id_user] AS [id], [user].[id_user] AS [subquery_order_0], [project_user].[user_id] AS [project_user.userId], [project_user].[project_id] AS [project_user.projectId]
               FROM [users] AS [user]
               INNER JOIN [project_users] AS [project_user]
                 ON [user].[id_user] = [project_user].[user_id]
                 AND [project_user].[project_id] = 1
               WHERE [user].[age] >= 21
               ORDER BY [subquery_order_0] ASC${current.dialect.name === 'mssql' ? ', [user].[id_user]' : ''}${sql.addLimitAndOffset({ limit: 3, order: ['last_name', 'ASC'] })}
            ) AS sub`,

Please note the order by instructions which imho order the results twice based on the same property. (subqueryorder0 === id_user)

Similarly I was wondering for other query where we order by lastName first and userId second, why we even automatically add an order by primaryKey in the first place. I get it if you don't have any order in place, but if the user decides to sort by last name, why would we adjust this to order by the pk secondarily?

[sdepold]

cc @sushantdhiman @lerit

[WikiRik]

We're talking about this test right?

sequelize/test/unit/sql/select.test.js

Lines 196 to 242 in 3cc8c5e

	testsql({
	table: User.getTableName(),
	model: User,
	attributes: [
	['id_user', 'id'],
	],
	order: [
	['id_user', 'ASC'],
	],
	where: {
	age: {
	[Op.gte]: 21,
	},
	},
	groupedLimit: {
	limit: 3,
	on: User.Projects,
	values: [
	1,
	5,
	],
	},
	}, {
	default: `SELECT [user].* FROM (${
	[
	`SELECT * FROM (
	SELECT [user].[id_user] AS [id], [user].[id_user] AS [subquery_order_0], [project_user].[user_id] AS [project_user.userId], [project_user].[project_id] AS [project_user.projectId]
	FROM [users] AS [user]
	INNER JOIN [project_users] AS [project_user]
	ON [user].[id_user] = [project_user].[user_id]
	AND [project_user].[project_id] = 1
	WHERE [user].[age] >= 21
	ORDER BY [subquery_order_0] ASC${current.dialect.name === 'mssql' ? ', [user].[id_user]' : ''}${sql.addLimitAndOffset({ limit: 3, order: ['last_name', 'ASC'] })}
	) AS sub`,
	`SELECT * FROM (
	SELECT [user].[id_user] AS [id], [user].[id_user] AS [subquery_order_0], [project_user].[user_id] AS [project_user.userId], [project_user].[project_id] AS [project_user.projectId]
	FROM [users] AS [user]
	INNER JOIN [project_users] AS [project_user]
	ON [user].[id_user] = [project_user].[user_id]
	AND [project_user].[project_id] = 5
	WHERE [user].[age] >= 21
	ORDER BY [subquery_order_0] ASC${current.dialect.name === 'mssql' ? ', [user].[id_user]' : ''}${sql.addLimitAndOffset({ limit: 3, order: ['last_name', 'ASC'] })}
	) AS sub`,
	].join(current.dialect.supports['UNION ALL'] ? ' UNION ALL ' : ' UNION ')
	}) AS [user] ORDER BY [subquery_order_0] ASC;`,
	});
	}());

We've added this test in #6560 and @ephys worked on this in #13985 (she added the TODO in the mssql query-generator).

[evanrittenhouse]

I do wonder why MSSQL requires so much work as well. I wonder if it's worth a larger look

[WikiRik]

I do wonder why MSSQL requires so much work as well. I wonder if it's worth a larger look

I think it is, but after we have migrated it to TypeScript (possibly even after the first release of v7). We'll probably also notice some things during that conversion and having stricter typing will probably help us as well

[sdepold]

We're talking about this test right?

sequelize/test/unit/sql/select.test.js

Lines 196 to 242 in 3cc8c5e

	testsql({
	table: User.getTableName(),
	model: User,
	attributes: [
	['id_user', 'id'],
	],
	order: [
	['id_user', 'ASC'],
	],
	where: {
	age: {
	[Op.gte]: 21,
	},
	},
	groupedLimit: {
	limit: 3,
	on: User.Projects,
	values: [
	1,
	5,
	],
	},
	}, {
	default: `SELECT [user].* FROM (${
	[
	`SELECT * FROM (
	SELECT [user].[id_user] AS [id], [user].[id_user] AS [subquery_order_0], [project_user].[user_id] AS [project_user.userId], [project_user].[project_id] AS [project_user.projectId]
	FROM [users] AS [user]
	INNER JOIN [project_users] AS [project_user]
	ON [user].[id_user] = [project_user].[user_id]
	AND [project_user].[project_id] = 1
	WHERE [user].[age] >= 21
	ORDER BY [subquery_order_0] ASC${current.dialect.name === 'mssql' ? ', [user].[id_user]' : ''}${sql.addLimitAndOffset({ limit: 3, order: ['last_name', 'ASC'] })}
	) AS sub`,
	`SELECT * FROM (
	SELECT [user].[id_user] AS [id], [user].[id_user] AS [subquery_order_0], [project_user].[user_id] AS [project_user.userId], [project_user].[project_id] AS [project_user.projectId]
	FROM [users] AS [user]
	INNER JOIN [project_users] AS [project_user]
	ON [user].[id_user] = [project_user].[user_id]
	AND [project_user].[project_id] = 5
	WHERE [user].[age] >= 21
	ORDER BY [subquery_order_0] ASC${current.dialect.name === 'mssql' ? ', [user].[id_user]' : ''}${sql.addLimitAndOffset({ limit: 3, order: ['last_name', 'ASC'] })}
	) AS sub`,
	].join(current.dialect.supports['UNION ALL'] ? ' UNION ALL ' : ' UNION ')
	}) AS [user] ORDER BY [subquery_order_0] ASC;`,
	});
	}());

We've added this test in #6560 and @ephys worked on this in #13985 (she added the TODO in the mssql query-generator).

give this input and the fact that we might want to refactor mssql once ts migration is completed, I'd say this PR is good to go. It's a but ugly but should roughly work

[evanrittenhouse]

Approved since we have consensus about cleaning up later - thanks for picking this up @sdepold!

[WikiRik]

Two comments of possible additional tests and one question. But I like the new function

[sdepold]

@WikiRik @evanrittenhouse Ready maybe :)

[sdepold]

Sooo.

Without casting the primaryKey to Col:

, [LoginLog].[id] OFFSET 0 ROWS FETCH NEXT 10 ROWS ONLY

With casting primaryKey to Col:

, [LoginLog].[id] OFFSET 0 ROWS FETCH NEXT 10 ROWS ONLY

is it supposed to make any difference?

[WikiRik]

I wasn't sure, but now we know the query does not change we should be good to remove it. Thanks for checking it out!

[sdepold]

@WikiRik do you think we should remove the col casting in v6 too?

[WikiRik]

No, let's leave it for now

[github-actions]

🎉 This PR is included in version 7.0.0-alpha.16 🎉

The release is available on:

• npm package (@release-v7 dist-tag)
• GitHub release

Your semantic-release bot 📦🚀