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

fix: don't call overloaded versions of find functions internally #13951

Merged
merged 5 commits into from Jan 15, 2022

Conversation

cincodenada
Copy link
Contributor

@cincodenada cincodenada commented Jan 15, 2022

Pull Request Checklist

Please make sure to review and check all of these items:

  • Have you added new tests to prevent regressions?
  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • [N/A] Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • [N/A] Did you update the typescript typings accordingly (if applicable)?
  • [N/A] 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

Comments in model.js indicated that we should avoid calling overloaded versions of findAll and findOne internally, but we were in fact doing just that. It appears this behavior was lost in the transition to ES6 classes in #5924. This fixes the code behavior to match the comment (and previous behavior); if this is no longer desired the comments could just be deleted instead.

cincodenada added 4 commits Jan 15, 2022
This was lost in sequelize#5924, but the comments remained, rendering them
contradictory. The tests demonstrate that the commented behavior is no
longer happening.
This aligns our behavior with the comments again
These were all over the place, I went with what seems to be the majority
@fzn0x fzn0x changed the title Don't call overloaded versions of find functions internally, per comments test: don't call overloaded versions of find functions internally, per comments Jan 15, 2022
@fzn0x fzn0x added the type: refactor label Jan 15, 2022
ephys
ephys approved these changes Jan 15, 2022
Copy link
Member

@ephys ephys left a comment

Looks good! I'm not opposed to letting people override Model static methods but I think it's safer not to.

Thanks for the test file name refactor, we're slowly refactoring to a more consistent codebase :)

@ephys ephys changed the title test: don't call overloaded versions of find functions internally, per comments fix: don't call overloaded versions of find functions internally Jan 15, 2022
@ephys ephys merged commit e5d02ef into sequelize:main Jan 15, 2022
42 of 44 checks passed
@ephys ephys added this to To be released in Sequelize v6 release log Jan 15, 2022
@cincodenada
Copy link
Contributor Author

@cincodenada cincodenada commented Jan 15, 2022

Looks good! I'm not opposed to letting people override Model static methods but I think it's safer not to.

Thanks for the test file name refactor, we're slowly refactoring to a more consistent codebase :)

Thanks! And yeah, little bits here and there add up over time, figured I'd do my part 🙂

And my understanding is that folks can still override the static methods if they want (I don't think we can really prevent it) but this makes sure if they do, we at least use the internal versions for internal calls where the callers depend on them behaving in a predictable way. Which should make it harder to break things by overriding them, at least! 😄

Thanks for looking this over and getting it merged (and fixing up my PR title)

@fzn0x
Copy link
Member

@fzn0x fzn0x commented Jan 15, 2022

I was wrong about the commit type, i thought it was all test related, but I think it's better with fix, thanks @ephys

@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from Releasing to To be released in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from To be released to Releasing in Sequelize v6 release log Jan 22, 2022
@sdepold sdepold moved this from Releasing to Released in Sequelize v6 release log Jan 23, 2022
ephys added a commit that referenced this issue Jan 24, 2022
@ephys ephys mentioned this pull request Jan 24, 2022
6 tasks
ephys added a commit that referenced this issue Jan 25, 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 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @v7 type: refactor
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants