Merge pull request rails#42517 from nvasilevski/nilable-default-scope-with-all-queries

Fix update & destroy queries when default_scope is nillable with all_…

Author: rafaelfranca

activerecord/lib/active_record/persistence.rb

@@ -431,9 +431,8 @@ def _insert_record(values) # :nodoc:
       def _update_record(values, constraints) # :nodoc:
         constraints = constraints.map { |name, value| predicate_builder[name, value] }
 
-        if default_scopes?(all_queries: true)
-          constraints << default_scoped(all_queries: true).where_clause.ast
-        end
+        default_constraint = build_default_constraint
+        constraints << default_constraint if default_constraint
 
         if current_scope = self.global_current_scope
           constraints << current_scope.where_clause.ast
@@ -449,9 +448,8 @@ def _update_record(values, constraints) # :nodoc:
       def _delete_record(constraints) # :nodoc:
         constraints = constraints.map { |name, value| predicate_builder[name, value] }
 
-        if default_scopes?(all_queries: true)
-          constraints << default_scoped(all_queries: true).where_clause.ast
-        end
+        default_constraint = build_default_constraint
+        constraints << default_constraint if default_constraint
 
         if current_scope = self.global_current_scope
           constraints << current_scope.where_clause.ast
@@ -479,6 +477,16 @@ def instantiate_instance_of(klass, attributes, column_types = {}, &block)
         def discriminate_class_for_record(record)
           self
         end
+
+        # Called by +_update_record+ and +_delete_record+
+        # to build `where` clause from default scopes.
+        # Skips empty scopes.
+        def build_default_constraint
+          return unless default_scopes?(all_queries: true)
+
+          default_where_clause = default_scoped(all_queries: true).where_clause
+          default_where_clause.ast unless default_where_clause.empty?
+        end
     end
 
     # Returns true if this object hasn't been saved yet -- that is, a record

activerecord/test/cases/scoping/default_scoping_test.rb

@@ -94,6 +94,12 @@ def test_default_scope_with_all_queries_runs_on_create
     assert_match(/mentor_id/, create_sql)
   end
 
+  def test_nilable_default_scope_with_all_queries_runs_on_create
+    create_sql = capture_sql { DeveloperWithDefaultNilableMentorScopeAllQueries.create!(name: "Nikita") }.first
+
+    assert_no_match(/AND$/, create_sql)
+  end
+
   def test_default_scope_runs_on_select
     Mentor.create!
     DeveloperwithDefaultMentorScopeNot.create!(name: "Eileen")
@@ -110,6 +116,13 @@ def test_default_scope_with_all_queries_runs_on_select
     assert_match(/mentor_id/, select_sql)
   end
 
+  def test_nilable_default_scope_with_all_queries_runs_on_select
+    DeveloperWithDefaultNilableMentorScopeAllQueries.create!(name: "Nikita")
+    select_sql = capture_sql { DeveloperWithDefaultNilableMentorScopeAllQueries.find_by(name: "Nikita") }.first
+
+    assert_no_match(/AND$/, select_sql)
+  end
+
   def test_default_scope_doesnt_run_on_update
     Mentor.create!
     dev = DeveloperwithDefaultMentorScopeNot.create!(name: "Eileen")
@@ -126,6 +139,13 @@ def test_default_scope_with_all_queries_runs_on_update
     assert_match(/mentor_id/, update_sql)
   end
 
+  def test_nilable_default_scope_with_all_queries_runs_on_update
+    dev = DeveloperWithDefaultNilableMentorScopeAllQueries.create!(name: "Nikita")
+    update_sql = capture_sql { dev.update!(name: "Not Nikita") }.first
+
+    assert_no_match(/AND$/, update_sql)
+  end
+
   def test_default_scope_doesnt_run_on_update_columns
     Mentor.create!
     dev = DeveloperwithDefaultMentorScopeNot.create!(name: "Eileen")
@@ -142,6 +162,13 @@ def test_default_scope_with_all_queries_runs_on_update_columns
     assert_match(/mentor_id/, update_sql)
   end
 
+  def test_nilable_default_scope_with_all_queries_runs_on_update_columns
+    dev = DeveloperWithDefaultNilableMentorScopeAllQueries.create!(name: "Nikita")
+    update_sql = capture_sql { dev.update_columns(name: "Not Nikita") }.first
+
+    assert_no_match(/AND$/, update_sql)
+  end
+
   def test_default_scope_doesnt_run_on_destroy
     Mentor.create!
     dev = DeveloperwithDefaultMentorScopeNot.create!(name: "Eileen")
@@ -158,6 +185,13 @@ def test_default_scope_with_all_queries_runs_on_destroy
     assert_match(/mentor_id/, destroy_sql)
   end
 
+  def test_nilable_default_scope_with_all_queries_runs_on_destroy
+    dev = DeveloperWithDefaultNilableMentorScopeAllQueries.create!(name: "Nikita")
+    destroy_sql = capture_sql { dev.destroy }.first
+
+    assert_no_match(/AND$/, destroy_sql)
+  end
+
   def test_default_scope_doesnt_run_on_reload
     Mentor.create!
     dev = DeveloperwithDefaultMentorScopeNot.create!(name: "Eileen")
@@ -174,6 +208,13 @@ def test_default_scope_with_all_queries_runs_on_reload
     assert_match(/mentor_id/, reload_sql)
   end
 
+  def test_nilable_default_scope_with_all_queries_runs_on_destroy
+    dev = DeveloperWithDefaultNilableMentorScopeAllQueries.create!(name: "Nikita")
+    reload_sql = capture_sql { dev.reload }.first
+
+    assert_no_match(/AND$/, reload_sql)
+  end
+
   def test_default_scope_with_all_queries_doesnt_run_on_destroy_when_unscoped
     dev = DeveloperWithDefaultMentorScopeAllQueries.create!(name: "Eileen", mentor_id: 2)
     reload_sql = capture_sql { dev.reload({ unscoped: true }) }.first

activerecord/test/models/developer.rb

@@ -162,6 +162,12 @@ class DeveloperWithDefaultMentorScopeAllQueries < ActiveRecord::Base
   default_scope -> { where(mentor_id: 1) }, all_queries: true
 end
 
+class DeveloperWithDefaultNilableMentorScopeAllQueries < ActiveRecord::Base
+  self.table_name = "developers"
+  firm_id = nil # Could be something like Current.mentor_id
+  default_scope -> { where(firm_id: firm_id) if firm_id }, all_queries: true
+end
+
 class DeveloperWithIncludes < ActiveRecord::Base
   self.table_name = "developers"
   has_many :audit_logs, foreign_key: :developer_id