-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Allow one to set strict_loading_mode
globally
#51339
base: main
Are you sure you want to change the base?
Conversation
45826ab
to
3a0a713
Compare
0d22cc2
to
6d47dc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I had a few suggestions below.
.vscode/settings.json
Outdated
"[ruby]": { | ||
"editor.defaultFormatter": "Shopify.ruby-lsp" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this file, configs should be kept local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah, sorry!
@@ -86,6 +86,20 @@ def test_strict_loading_n_plus_one_only_mode_with_belongs_to | |||
end | |||
end | |||
|
|||
def test_default_mode_is_all | |||
developer = Developer.first | |||
assert_predicate(developer, :strict_loading_all?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_predicate(developer, :strict_loading_all?) | |
assert_predicate developer, :strict_loading_all? |
self.table_name = "developers" | ||
end.new | ||
|
||
assert_predicate(developer, :strict_loading_n_plus_one_only?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_predicate(developer, :strict_loading_n_plus_one_only?) | |
assert_predicate developer, :strict_loading_n_plus_one_only? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as an FYI, this code passed the linter. Would you like me to change the Rubocop rules, or the code syntax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code syntax. The linter doesn't enforce any specific direction on this, so both would pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
guides/source/configuring.md
Outdated
@@ -1226,6 +1226,10 @@ changed to `:log` to send violations to the logger instead of raising. | |||
Is a boolean value that either enables or disables strict_loading mode by | |||
default. Defaults to `false`. | |||
|
|||
#### `config.active_record.strict_loading_mode` | |||
|
|||
Sets the mode in which strict loading is reported. Deaults to `:all`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sets the mode in which strict loading is reported. Deaults to `:all`. | |
Sets the mode in which strict loading is reported. Defaults to `:all`. It can be | |
changed to `:n_plus_one_only` to only report when loading associations that will | |
lead to an N + 1 query. |
activerecord/CHANGELOG.md
Outdated
@@ -1,3 +1,7 @@ | |||
* Allow one to define `strict_loading_mode`, either globally or within a model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Allow one to define `strict_loading_mode`, either globally or within a model | |
* Allow to configure `strict_loading_mode` globally or within a model. | |
Defaults to `:all`, can be changed to `:n_plus_one_only`. |
6d47dc2
to
d14d8b4
Compare
@carlosantoniodasilva Thanks for the quick review! I added |
@@ -699,6 +700,11 @@ def strict_loading!(value = true, mode: :all) | |||
|
|||
attr_reader :strict_loading_mode | |||
|
|||
# Returns +true+ if the record uses strict_loading with +:all+ mode enabled. | |||
def strict_loading_all? | |||
@strict_loading_mode == :all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should not this only return true if strict_loading
is true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was following the design of the method right below:
rails/activerecord/lib/active_record/core.rb
Lines 708 to 711 in d14d8b4
# Returns +true+ if the record uses strict_loading with +:n_plus_one_only+ mode enabled. | |
def strict_loading_n_plus_one_only? | |
@strict_loading_mode == :n_plus_one_only | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change both if you'd like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 gentle bump in case you want the method body changed here!
Per the comment at the top of that file, it is not. |
448f2dd
to
a217959
Compare
Made all the requested changes. I think the current CI failure is something flakey:
I also can't see a way to restart the build, I guess because of admin permissions. |
a217959
to
ab86eaf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just one tiny thing on the changelog, if you don't mind updating (since it's also conflicting with main now). Thanks!
activerecord/CHANGELOG.md
Outdated
@@ -1,3 +1,8 @@ | |||
* Allowed to configure `strict_loading_mode` globally or within a model. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: we don't really use past tense in the changelog:
* Allowed to configure `strict_loading_mode` globally or within a model. | |
* Allow to configure `strict_loading_mode` globally or within a model. |
a0b5dcb
to
13e4a55
Compare
@carlosantoniodasilva Changed! 🫡 |
Should I not have edited the CHANGELOG? I’m not sure of the process; was a core maintainer supposed to update that? I really don’t want to keep resolving conflicts… |
13e4a55
to
11249c0
Compare
11249c0
to
cb59878
Compare
@carlosantoniodasilva I just removed the CHANGELOG change. Someone else can add those notes if they want. 🤷♂️ |
cb59878
to
51fb31e
Compare
https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#updating-the-changelog This is for better or worse, par for the course. 🙏 |
51fb31e
to
8323eef
Compare
It just stinks because I was really hoping to get this in to 7.2. CHANGELOG entry re-added. |
It's not uncommon for a committer to rebase the changelog conflict before merging, so I wouldn't worry about constantly trying to keep it up to date while waiting for a review. Especially since having any changelog is better than no changelog. |
but my understanding is the review is complete…? No one has requested any changes, but I continue to rebase conflicts and hope to be free. |
Carlos gave you his blessing here, but he is really busy now I think: I will also drop a link in the discord to get it some more eyeballs. Thanks for your patience! |
Sorry I noticed there are some relevant errors in CI, can you check?
I think they are all due to the duplicate method definition. |
51fb31e
to
c514f25
Compare
@zzak my bad, fixed that |
c514f25
to
750b2d4
Compare
# Returns +true+ if the record uses strict_loading with +:all+ mode enabled. | ||
def strict_loading_all? | ||
@strict_loading_mode == :all | ||
end | ||
|
||
# Returns +true+ if the record uses strict_loading with +:n_plus_one_only+ mode enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this method moved?
It looks unchanged, so if you could rebase this out for a cleaner diff 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the ~2.5 months I've been pushing this PR along, someone else added the exact same method. I've not been tremendously great at rebasing, so every time I have to fix a conflict I rush through it in the vain hope that I can catch a merge. But, I have fixed it.
@@ -98,6 +98,20 @@ def test_strict_loading_n_plus_one_only_mode_does_not_eager_load_child_associati | |||
end | |||
end | |||
|
|||
def test_default_mode_is_all | |||
developer = Developer.first | |||
assert_predicate developer, :strict_loading_all? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert_predicate developer, :strict_loading_all? | |
assert_predicate(developer, :strict_loading_all?) |
We don't enforce this on rubocop because it would result in a large cosmetic change which we try to avoid. I think an exception was made to enforce double quotes. There is also no .rubocop_todo.yml
to enforce this progressively, so we're left with solving this in code review. 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized this is the exact opposite style suggestion made earlier: #51339 (comment)
I accepted yours, which was my original style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, ugh. L112 also has the parenthetical-less style which was suggested: https://github.com/rails/rails/pull/51339/files#diff-0f8c8b854a62005e61f7855cf989a0be4836f591cef662baddff17194642f55fR112
I've gone back to removing the parens here, too. It's two previous commenters making this suggestion, so 🤷🏻♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I may have misread that, it was early 🙈
My impression was that some maintainers actively avoid using Seattle.rb style (no-parens), but let's wait for another look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fwiw this entire file is paren-less 🤷🏻♂️
e5f1cf9
to
ea1895c
Compare
ea1895c
to
1ab2865
Compare
Motivation / Background
I was reading through this summary of Rails' strict loading and came across this paragraph:
In my app, we're setting
strict_loading!(mode: :n_plus_one_only)
on individual records; this paragraph made me realize it didn't have to be this way.This Pull Request has been created because currently,
strict_loading_mode
is always set to:all
. It may be preferable for users to have:n_plus_one_only
on an individual model, or even on the entire app.Detail
This Pull Request adds a new class_attribute
:strict_loading_mode
, defaulted to:all
. If it's set to:n_plus_one_only
, that mode is used by default when doing strict loading checks.Additional information
n/a
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]