fix(mssql): return no result when limit is zero

[gmasclet]

Pull Request Checklist

• [x] Have you added new tests to prevent regressions?
• [ ] ~If a documentation update is necessary, have you opened a PR to the documentation repository?~
• [ ] ~Did you update the typescript typings accordingly (if applicable)?~
• [x] Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
• [x] Does the name of your PR follow our conventions?

Description Of Change

This is a naive attempt to fix https://github.com/sequelize/sequelize/issues/15959.

Today, the MSSQL variant of addLimitAndOffset() outputs nothing when the limit parameter is zero. Therefore, no pagination is applied and Sequelize returns all results. I would instead expect Sequelize to return no result, as it does with other dialects.

This fix doesn't work: FETCH NEXT 0 ROWS ONLY is considered as invalid by MS SQL Server as The number of rows provided for a FETCH clause must be greater then zero.

So I think a correct solution would be to have a condition higher in the call stack that bypasses SQL generation/execution altogether when limit is zero, and instead directly returns an empty set. Not sure where to add such a condition, any guidance/advice on that is welcome.

Todos

• [ ] Implement a fix that works
• [ ] Revert the changes in packages/core/src/dialects/mssql/query-generator.js
• [ ] Add more tests?

[evanrittenhouse]

LGTM, please just fix the failing tests :)

[gmasclet]

Thanks for your review :)

sadly, fixing the test is not straigthforward, as the generated SQL query is considered as invalid by MS SQL Server. See details in the description of this pull request.

At this point, I'm not sure about the best solution. Here are the options I consider:

• Bypassing SQL generation/execution altogether when the limit parameter is zero, and instead directly return an empty set. I see two risks:

  • Such a bypass would probably not be dialect specific, while this issue is specific to MS SQL.
  • Maybe we never want to bypass SQL generation/execution even when it's going to yield an empty set, because it may also have side-effects in some edge cases.

• Keeping the "fix" I've implemented in this PR. So Sequelize lets the MS SQL error bubble up. In that case, I've to fix the test so that it expects an error (so the test has to be specific to the MS SQL dialect). I note this is a breaking change. Not sure it's really better than silently ignoring the limit option (the benefit is that it makes it clear that this edge case is not handled by Sequelize).

• Closing this pull request and the bug report as "won't fix", at least as long as MS SQL doesn't accept the FETCH NEXT 0 ROWS ONLY syntax.

Let me know what you think.

[WikiRik]

Sorry for the delayed response but I think we want MSSQL to throw an error saying a limit of 0 is not supported. Silently ignoring options is something we're reducing in v7 and beyond.