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

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.

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 For issues and PRs. Things that improve the code readability, maintainability, testability, etc. label Jan 15, 2022
Copy link
Member

@ephys ephys 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! 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
@cincodenada
Copy link
Contributor Author

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

ephys added a commit that referenced this pull request Jan 24, 2022
@ephys ephys mentioned this pull request Jan 24, 2022
6 tasks
ephys added a commit that referenced this pull request Jan 24, 2022
fzn0x pushed a commit that referenced this pull request Jan 24, 2022
ephys added a commit that referenced this pull request Jan 25, 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 📦🚀

maramizo pushed a commit to maramizo/sequelize that referenced this pull request Jun 2, 2022
maramizo pushed a commit to maramizo/sequelize that referenced this pull request Jun 2, 2022
maramizo pushed a commit to maramizo/sequelize that referenced this pull request Jun 2, 2022
aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
aliatsis pushed a commit to creditiq/sequelize that referenced this pull request 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: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants