Allow one to set `strict_loading_mode` globally #51339

gjtorikian · 2024-03-17T16:45:34Z

Motivation / Background

I was reading through this summary of Rails' strict loading and came across this paragraph:

If you’re very brave, you can opt your entire application into strict loading. (Oddly, there doesn’t seem to be an equivalent of :n_plus_one_only here. I can’t imagine using this.)

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:

This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

carlosantoniodasilva

Thanks, I had a few suggestions below.

carlosantoniodasilva · 2024-03-18T14:28:41Z

.vscode/settings.json

+    "[ruby]": {
+        "editor.defaultFormatter": "Shopify.ruby-lsp"
+    }
+}


Please remove this file, configs should be kept local.

Gah, sorry!

carlosantoniodasilva · 2024-03-18T14:29:04Z

activerecord/test/cases/strict_loading_test.rb

@@ -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?)


Suggested change

assert_predicate(developer, :strict_loading_all?)

assert_predicate developer, :strict_loading_all?

carlosantoniodasilva · 2024-03-18T14:29:12Z

activerecord/test/cases/strict_loading_test.rb

+      self.table_name = "developers"
+    end.new
+
+    assert_predicate(developer, :strict_loading_n_plus_one_only?)


Suggested change

assert_predicate(developer, :strict_loading_n_plus_one_only?)

assert_predicate developer, :strict_loading_n_plus_one_only?

Just as an FYI, this code passed the linter. Would you like me to change the Rubocop rules, or the code syntax?

The code syntax. The linter doesn't enforce any specific direction on this, so both would pass.

Well, yes, I know both would pass. I'm saying there's no consistency across the repo for parentheticals:

I'm suggesting that the linter should catch this.

carlosantoniodasilva · 2024-03-18T14:29:19Z

guides/source/configuring.md

@@ -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`.


Suggested change

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.

carlosantoniodasilva · 2024-03-18T14:38:35Z

activerecord/CHANGELOG.md

@@ -1,3 +1,7 @@
+*   Allow one to define `strict_loading_mode`, either globally or within a model


Suggested change

* 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`.

gjtorikian · 2024-03-18T14:45:32Z

@carlosantoniodasilva Thanks for the quick review! I added .vscode to .gitignore; please let me know if that's acceptable, otherwise I'll revert that change. I also left a reply re: linting.

rafaelfranca · 2024-03-18T21:32:17Z

activerecord/lib/active_record/core.rb

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


Should not this only return true if strict_loading is true?

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

I can change both if you'd like.

👋 gentle bump in case you want the method body changed here!

matthewd · 2024-03-19T04:44:58Z

I added .vscode to .gitignore; please let me know if that's acceptable

Per the comment at the top of that file, it is not.

gjtorikian · 2024-03-19T17:44:41Z

Made all the requested changes. I think the current CI failure is something flakey:

Using trilogy
rake aborted!
ActiveRecord::ConnectionNotEstablished: Connection refused - trilogy_connect - unable to connect to mysql:3306 (ActiveRecord::ConnectionNotEstablished)

I also can't see a way to restart the build, I guess because of admin permissions.

carlosantoniodasilva

LGTM, just one tiny thing on the changelog, if you don't mind updating (since it's also conflicting with main now). Thanks!

carlosantoniodasilva · 2024-04-09T14:38:40Z

activerecord/CHANGELOG.md

@@ -1,3 +1,8 @@
+*   Allowed to configure `strict_loading_mode` globally or within a model.


Minor: we don't really use past tense in the changelog:

Suggested change

* Allowed to configure `strict_loading_mode` globally or within a model.

* Allow to configure `strict_loading_mode` globally or within a model.

gjtorikian · 2024-04-09T15:48:12Z

@carlosantoniodasilva Changed! 🫡

gjtorikian · 2024-04-23T14:38:16Z

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…

gjtorikian · 2024-05-09T12:52:36Z

@carlosantoniodasilva I just removed the CHANGELOG change. Someone else can add those notes if they want. 🤷‍♂️

rails-bot bot added the activerecord label Mar 17, 2024

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch from 45826ab to 3a0a713 Compare March 17, 2024 18:52

rails-bot bot added the docs label Mar 17, 2024

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch 3 times, most recently from 0d22cc2 to 6d47dc2 Compare March 18, 2024 12:29

carlosantoniodasilva reviewed Mar 18, 2024

View reviewed changes

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch from 6d47dc2 to d14d8b4 Compare March 18, 2024 14:44

rafaelfranca reviewed Mar 18, 2024

View reviewed changes

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch 3 times, most recently from 448f2dd to a217959 Compare March 19, 2024 12:52

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch from a217959 to ab86eaf Compare March 28, 2024 21:52

carlosantoniodasilva reviewed Apr 9, 2024

View reviewed changes

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch 3 times, most recently from a0b5dcb to 13e4a55 Compare April 9, 2024 15:47

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch from 13e4a55 to 11249c0 Compare April 23, 2024 18:04

Allow one to set strict_loading_mode globally

cb59878

gjtorikian force-pushed the enable-n_plus_one_only-toggling branch from 11249c0 to cb59878 Compare May 9, 2024 12:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow one to set `strict_loading_mode` globally #51339

Allow one to set `strict_loading_mode` globally #51339

gjtorikian commented Mar 17, 2024 •

edited

carlosantoniodasilva left a comment

carlosantoniodasilva Mar 18, 2024

gjtorikian Mar 18, 2024

carlosantoniodasilva Mar 18, 2024

carlosantoniodasilva Mar 18, 2024

gjtorikian Mar 18, 2024

rafaelfranca Mar 18, 2024

gjtorikian Mar 19, 2024

carlosantoniodasilva Mar 18, 2024

carlosantoniodasilva Mar 18, 2024

gjtorikian commented Mar 18, 2024

rafaelfranca Mar 18, 2024

gjtorikian Mar 19, 2024

gjtorikian Mar 19, 2024

gjtorikian Mar 28, 2024

matthewd commented Mar 19, 2024

gjtorikian commented Mar 19, 2024

carlosantoniodasilva left a comment •

edited

carlosantoniodasilva Apr 9, 2024

gjtorikian commented Apr 9, 2024

gjtorikian commented Apr 23, 2024 •

edited

gjtorikian commented May 9, 2024

	assert_predicate(developer, :strict_loading_all?)
	assert_predicate developer, :strict_loading_all?

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

		@@ -1,3 +1,7 @@
		* Allow one to define `strict_loading_mode`, either globally or within a model

	* 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`.

	# 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

		@@ -1,3 +1,8 @@
		* Allowed to configure `strict_loading_mode` globally or within a model.

Allow one to set strict_loading_mode globally #51339

Are you sure you want to change the base?

Allow one to set strict_loading_mode globally #51339

Conversation

gjtorikian commented Mar 17, 2024 • edited

Motivation / Background

Detail

Additional information

Checklist

carlosantoniodasilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjtorikian commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewd commented Mar 19, 2024

gjtorikian commented Mar 19, 2024

carlosantoniodasilva left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjtorikian commented Apr 9, 2024

gjtorikian commented Apr 23, 2024 • edited

gjtorikian commented May 9, 2024

Allow one to set `strict_loading_mode` globally #51339

Allow one to set `strict_loading_mode` globally #51339

gjtorikian commented Mar 17, 2024 •

edited

carlosantoniodasilva left a comment •

edited

gjtorikian commented Apr 23, 2024 •

edited