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

Add strict_loading mode to optionally prevent lazy loading #37400

Merged
merged 1 commit into from Feb 20, 2020

Conversation

eileencodes
Copy link
Member

@eileencodes eileencodes commented Oct 8, 2019

Add #strict to any record to prevent lazy loading of associations.
strict will cascade down from the parent record to all the
associations to help you catch any places where you may want to use
preload instead of lazy loading.

Co-authored-by: Aaron Patterson aaron.patterson@gmail.com

cc/ @rafaelfranca @tenderlove @jhawthorn @matthewd

@eileencodes eileencodes changed the title Add strict mode to prevent N+1's Add strict mode to optionally prevent lazy loading Oct 8, 2019
@eileencodes eileencodes added this to the 6.1.0 milestone Oct 8, 2019
@@ -805,10 +805,21 @@ def readonly(value = true)
end

def readonly!(value = true) # :nodoc:
p self.method(:readonly_value).source_location
Copy link
Contributor

@kaspth kaspth Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

Copy link
Member Author

@eileencodes eileencodes Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha oops. This is what I get for opening a PR that I'd been sitting on for months.

Copy link
Member

@rafaelfranca rafaelfranca left a comment

I like the idea. Added some comments about the implementation and it would be important to write documentation as well.

@@ -89,6 +89,7 @@ def update_counters_via_scope(klass, foreign_key, by)
end

def find_target?
raise if @owner.readonly?
Copy link
Member

@rafaelfranca rafaelfranca Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we raise a specific exception here to tell users what this means? BTW, why readonly? should it not be strict?

Copy link
Member

@tenderlove tenderlove Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line can be removed actually (cc @eileencodes). But yes we should add an exception that is specific to this.

@@ -124,8 +124,12 @@ def build_scope
end

scope.merge!(reflection_scope) if reflection.scope
scope.merge!(preload_scope) if preload_scope
scope
scope.merge!(preload_scope) if preload_scope && !preload_scope.empty_scope?
Copy link
Member

@rafaelfranca rafaelfranca Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a two levels if would be easier to understand.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Oct 8, 2019

Also should we add this option also to the association declaration?

belongs_to :foo, strict: true? Right now we kinda of can do that by using a lambda, but maybe it is worth an option.

assert_predicate dev, :strict?
assert dev.audit_logs.all?(&:strict?), "Expected all audit logs to be strict"
end
end
Copy link
Contributor

@dylanahsmith dylanahsmith Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could use more tests for relation methods. For example, doing dev.audit_logs.first in test_raises_if_strict_and_lazy_loading instead of dev.audit_logs.to_a will not result in an exception, which I find surprising. If we are considering the relation itself to be strict, then I would expect that at least relation methods that use the records if they are loaded? to raise if the relation hasn't been loaded.

Copy link
Contributor

@dylanahsmith dylanahsmith Oct 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want the following to handle those methods on the collection proxy

diff --git a/activerecord/lib/active_record/associations/collection_association.rb b/activerecord/lib/active_record/associations/collection_association.rb
index f1b470f97e..958eba114d 100644
--- a/activerecord/lib/active_record/associations/collection_association.rb
+++ b/activerecord/lib/active_record/associations/collection_association.rb
@@ -306,6 +306,7 @@ def null_scope?
 
       def find_from_target?
         loaded? ||
+          owner.strict? ||
           owner.new_record? ||
           target.any? { |record| record.new_record? || record.changed? }
       end
diff --git a/activerecord/test/cases/strict_test.rb b/activerecord/test/cases/strict_test.rb
index 806810484f..5264521992 100644
--- a/activerecord/test/cases/strict_test.rb
+++ b/activerecord/test/cases/strict_test.rb
@@ -21,6 +21,15 @@ def test_raises_if_strict_and_lazy_loading
     end
   end
 
+  def test_raises_on_unloaded_relation_methods_if_strict
+    dev = Developer.strict.first
+    assert_predicate dev, :strict?
+
+    assert_raises ActiveRecord::StrictViolationError do
+      dev.audit_logs.first
+    end
+  end
+
   def test_preload_audit_logs_are_strict_because_parent_is_strict
     developer = Developer.first
 

@matthewd
Copy link
Member

@matthewd matthewd commented Oct 9, 2019

Is "strict" a sufficiently communicative name? I worry that User.where(admin: true).strict reads a little too easily as a stricter form of condition checking, or something. Is e.g. "strict_loading" too wordy?

Sure would be nice if I hadn't left #32136 to wilt for [checks calendar] a year. 😐

@bogdan
Copy link
Contributor

@bogdan bogdan commented Oct 9, 2019

Can you please make sure that Relation#strict_value is either false or true but never nil. nil creates confusion on other relation values like Relation#lock_value or Relation#readonly_value and I don't think we want to continue that design issue.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Oct 10, 2019

nil is falsy so there is no problem with returning nil. It is following the Rails guidelines for boolean logic.

@bogdan
Copy link
Contributor

@bogdan bogdan commented Oct 11, 2019

Problems with ‘nil’ start when you start using ‘*_value’ directly in tests for example or advanced activerecord code. It is just an ambiguity no one wants to deal with.

@lsylvester
Copy link
Contributor

@lsylvester lsylvester commented Nov 5, 2019

I don't think that this currently works with records that are eager_loaded in a single query. This test fails for me:

  def test_eager_load_audit_logs_are_strict_because_parent_is_strict
    developer = Developer.first

    3.times do
      AuditLog.create(developer: developer, message: "I am message")
    end

    dev = Developer.eager_load(:audit_logs).strict.first

    assert_predicate dev, :strict?
    assert dev.audit_logs.all?(&:strict?), "Expected all audit logs to be strict"
  end

#=> StrictTest#test_preload_audit_logs_are_strict_because_parent_is_strict [./rails/activerecord/test/cases/strict_test.rb:34]:
    Expected all audit logs to be strict

@rails-bot
Copy link

@rails-bot rails-bot bot commented Feb 3, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Feb 3, 2020
@louim
Copy link
Contributor

@louim louim commented Feb 6, 2020

Hey! This seems like a really useful concept and I would be sad to see this PR disappear in the stale void. 🚀

@rails-bot rails-bot bot removed the stale label Feb 6, 2020
@eileencodes eileencodes force-pushed the add-strict-mode branch 2 times, most recently from de5f6bf to 539ed23 Compare Feb 18, 2020
@eileencodes
Copy link
Member Author

@eileencodes eileencodes commented Feb 18, 2020

Ok we've gotten the extra noted cases working and added some new tests. I cleaned up my messy code. 😬

I like @rafaelfranca's suggestion but that feels like a follow-up PR - maybe even a good first issue for someone to tackle.

I changed the name from strict to strict_loading.

@eileencodes eileencodes changed the title Add strict mode to optionally prevent lazy loading Add strict_loading mode to optionally prevent lazy loading Feb 18, 2020
Add `#strict_loading` to any record to prevent lazy loading of associations.
`strict_loading` will cascade down from the parent record to all the
associations to help you catch any places where you may want to use
`preload` instead of lazy loading. This is useful for preventing N+1's.

Co-authored-by: Aaron Patterson <aaron.patterson@gmail.com>
@eileencodes eileencodes merged commit ac9b11a into rails:master Feb 20, 2020
2 checks passed
@eileencodes eileencodes deleted the add-strict-mode branch Feb 20, 2020
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this issue Jun 1, 2020
…:Base.strict_loading_by_default=`.

This will allow to enable/disable strict_loading mode by default for a model.
The configuration's value is inheritable by subclasses, but they can override that value and
it will not impact parent class:

```ruby
class Developer < ApplicationRecord
  self.strict_loading_by_default = true

  has_many :projects
end

dev = Developer.first
dev.projects.first
\# => ActiveRecord::StrictLoadingViolationError Exception: Developer is marked as strict_loading and Project cannot be lazily loaded.
```

What is great about this feature that it could help users to nip N+1 queries in
the bud, especially for fresh applications, by setting
`ActiveRecord::Base.strict_loading_by_default = true` / `config.active_record.strict_loading_by_default = true`.
That is also a great way to prevent new N+1 queries in the existing applications
after all the N+1 queries are eliminated.
(See https://guides.rubyonrails.org/v6.0/active_record_querying.html#eager-loading-associations,
https://github.com/seejohnrun/prelude for details on how to fight against N+1 queries).

Related to rails#37400, rails#38541
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants