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 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

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 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 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 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 ephys commented Jan 21, 2022

Sounds good, I'll look into it :)

@ephys ephys marked this pull request as draft Jan 22, 2022
@ephys
Copy link
Member Author

@ephys ephys commented Jan 25, 2022

Test added

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

@WikiRik WikiRik left a comment

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

@ephys
Copy link
Member Author

@ephys 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 ephys commented Jan 25, 2022

Ok it should be fixed

@ephys ephys requested a review from WikiRik Jan 27, 2022
Copy link
Member

@WikiRik WikiRik left a comment

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

@ephys
Copy link
Member Author

@ephys 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
42 checks passed
@ephys ephys deleted the ephys/clarify-limit-docs branch Jan 27, 2022
@sdepold sdepold added this to To be released in Sequelize v6 release log Jan 29, 2022
@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 29, 2022
@sdepold sdepold moved this from Releasing to Released in Sequelize v6 release log Jan 29, 2022
@github-actions
Copy link
Contributor

@github-actions 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 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this issue Jun 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: docs
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants