MSSQL - strange behavior with limit and already specified order

[arnaud-moncel]

Issue Creation Checklist

• I have read the contribution guidelines

Issue Description

Hi guys 👋, I just want to confirm with you if I found a bug or if I just do something wrong.

Let's say I have 2 models defined like this:

const City = sequelize.define(
  'city',
  {
    id: {
      type: DataTypes.INTEGER,
      primaryKey: true,
      autoIncrement: true,
      allowNull: false,
    },
    name: {
      type: DataTypes.STRING,
      allowNull: false,
    },
  },
  {
    tableName: 'city',
    timestamps: false,
  },
);

const Address = sequelize.define(
  `address`,
  {
    id: {
      type: DataTypes.INTEGER,
      autoIncrement: true,
      primaryKey: true,
      allowNull: false,
    },
    postalCode: {
      type: DataTypes.STRING,
    },
  },
  {
    tableName: 'address',
    timestamps: false,
  },
);

City.hasMany(Address);
Address.belongsTo(City);

I have this findAll query 👇, the goal is pretty simple, I just want to make a leaderboard by the most used city in addresses.

const query = {
  attributes: [
    [col('city.name'), 'city__grouped__'],
    [fn('count', '*'), '__aggregate__'],
  ],
  group: ['city.name'],
  include: [
    {
      association: 'addresses',
      attributes: [],
      include: [],
    },
  ],
  limit: 5,
  order: [[fn('count', '*'), 'DESC']],
  subQuery: false,
  raw: true,
};

This is where you can help me determine if it's a bug or if I did something wrong.
The call to the findAll function result to this query 👇

SELECT
  [city].[name] AS [city__grouped__],
  COUNT(*) AS [__aggregate__]
FROM
  [city] AS [city]
  LEFT OUTER JOIN [address] AS [addresses] ON [city].[id] = [addresses].[cityId]
GROUP BY
  [city].[name]
ORDER BY
  COUNT(*) DESC,
  [city].[id] OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY;

In fact this query result to a SQL error that say:
Column "city.id" is invalid in the ORDER BY clause because it is not contained in either an aggregate function or the GROUP BY clause.

I deep dive in the code and I found something strange here

sequelize/src/dialects/mssql/query-generator.js

Lines 993 to 1006 in abdb896

	if (!options.order || options.order.length === 0 || options.include && orders.subQueryOrder.length === 0) {
	const tablePkFragment = `${this.quoteTable(options.tableAs || model.name)}.${this.quoteIdentifier(model.primaryKeyField)}`;
	if (!options.order || options.order.length === 0) {
	fragment += ` ORDER BY ${tablePkFragment}`;
	} else {
	const orderFieldNames = _.map(options.order, order => order[0]);
	const primaryKeyFieldAlreadyPresent = _.includes(orderFieldNames, model.primaryKeyField);
	if (!primaryKeyFieldAlreadyPresent) {
	fragment += options.order && !isSubQuery ? ', ' : ' ORDER BY ';
	fragment += tablePkFragment;
	}
	}
	}

Sorry about the long message, but my question is: Why [city].[id] is added if I specify an order in my query?

let me know if it not clearer enough.
Thank's for your time 🙏 have a great day.

Issue Template Checklist

Is this issue dialect-specific?

• No. This issue is relevant to Sequelize as a whole.
• Yes. This issue only applies to the following dialect(s): MSSQL
• I don't know.

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don't know how to start, I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

[ephys]

That's weird.

The only line in mssql's query generator that mutates the order option is this one https://github.com/sequelize/sequelize/blob/main/src/dialects/mssql/query-generator.js#L881
I'm not fully convinced it's the culprit. It would help to determine if this issue happens with other dialects as well

[arnaud-moncel]

Hi @ephys 👋, thank you for your response.
I tested it with mysql and postgres and no issue with it.

You are right, orders options are not modified by my linked code.
But when the query is generated, the addLimitAndOffset function (my linked code) add the tablePkFragment next to the order clause.

In fact it add to the existing query

, [city].[id] OFFSET 0 ROWS FETCH NEXT 5 ROWS ONLY;

So I'm try to understand why this tablePkFragment is auto added when I specified an order.
I'm trying to find a use case, but I can't find it.

If we look addLimitAndOffset on the other dialect, mssql is the only one take care of order when i want to set a limit / offset option.

Let me know 😁

[ephys]

Aah, sorry I went too fast again and didn't read carefully enough

I remember seeing the weird behavior in that function too, I added the comment just before it

sequelize/src/dialects/mssql/query-generator.js

Line 991 in abdb896

// TODO: document why this is adding the primary key of the model in ORDER BY

Maybe mssql had (has?) a requirement to specify ORDER BY when using limit/offset. Or it was a decision from the previous maintainers.

Anyway, it seems the reason the clause is added is because this piece of code did not expect subQuery to be false.
If you're willing to open a PR with a test & a fix, we'll try to review it in a timely matter :)

[arnaud-moncel]

You are right, and I just tested it mssql need an order by to be able to use offset ans fetch features.
I'm gonna open a PR to fix this thank's again.

[aniruth37]

@ephys I saw that this has been stale for a while. Raised a PR on v6 - #16776.

[rishabh-arya95]

@ephys This issue is still happening, can anyone review the PR - #16776