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

docs: clarify how the limit option works #13985

Merged
merged 13 commits into from Jan 27, 2022
Merged

docs: clarify how the limit option works #13985

merged 13 commits into from Jan 27, 2022

Conversation

ephys
Copy link
Member

@ephys ephys commented Jan 21, 2022

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

I'm attempting to clarify what FindOptions#limit does and its interaction with FindOptions#subQuery

Related to #9605

@ephys ephys added the type: docs For issues and PRs. Things related to documentation, such as changes in the manuals / API reference. label Jan 21, 2022
@ephys ephys self-assigned this Jan 21, 2022
WikiRik
WikiRik previously approved these changes Jan 21, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Looks good. Just to check; do we have tests for these three examples to ensure that's the actual behavior?

@ephys
Copy link
Member Author

ephys commented Jan 21, 2022

We test for limit + include here: https://github.com/sequelize/sequelize/blob/main/test/unit/sql/select.test.js#L281
However we never test limit + include + subQuery: false. But we do it: https://github.com/sequelize/sequelize/blob/main/lib/dialects/abstract/query-generator.js#L1164

There is also an undocumented groupedLimit option, I'm not sure of what it does.
I found this on groupedLimit: #6560

@ephys
Copy link
Member Author

ephys commented Jan 21, 2022

I've removed the third example I added as it's an unrelated limit option (Includable.limit) with a different behavior.

I've also piggybacked this tiny PR to document a weirdness I found in operators.ts (registering operators in the global symbol registry).

@WikiRik
Copy link
Member

WikiRik commented Jan 21, 2022

However we never test limit + include + subQuery: false. But we do it: https://github.com/sequelize/sequelize/blob/main/lib/dialects/abstract/query-generator.js#L1164

Could you add tests for this?

@ephys
Copy link
Member Author

ephys commented Jan 21, 2022

Sounds good, I'll look into it :)

@ephys ephys marked this pull request as draft January 22, 2022 19:25
@ephys
Copy link
Member Author

ephys commented Jan 25, 2022

Test added

@ephys ephys marked this pull request as ready for review January 25, 2022 16:54
@ephys ephys requested a review from WikiRik January 25, 2022 16:54
WikiRik
WikiRik previously approved these changes Jan 25, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Woops, was too early with the approval. Do fix the mssql assertion first

@ephys
Copy link
Member Author

ephys commented Jan 25, 2022

That's weird. Why does mssql produce
ORDER BY [user].[last_name] ASC, [user].[id_user]
instead of
ORDER BY [user].[last_name] ASC

it doesn't seem to be doing that in other queries or when subQuery is true

Ok so MssqlQueryGenerator#addLimitAndOffset returns , [user].[id_user] OFFSET 10 ROWS FETCH NEXT 30 ROWS ONLY which looks like a hack to me

@ephys
Copy link
Member Author

ephys commented Jan 25, 2022

Ok it should be fixed

@ephys ephys requested a review from WikiRik January 27, 2022 14:45
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Looks good, any idea if/when you want to address those TODOs?

@ephys
Copy link
Member Author

ephys commented Jan 27, 2022

Not in this PR but we'll eventually get to it as we migrate to TS

@ephys ephys merged commit 12c18e5 into main Jan 27, 2022
@ephys ephys deleted the ephys/clarify-limit-docs branch January 27, 2022 15:43
sdepold pushed a commit that referenced this pull request Jan 29, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

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

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: docs For issues and PRs. Things related to documentation, such as changes in the manuals / API reference.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants