diff --git a/activerecord/lib/active_record/persistence.rb b/activerecord/lib/active_record/persistence.rb index 7b26ffc5a4..26d2b31f40 100644 --- a/activerecord/lib/active_record/persistence.rb +++ b/activerecord/lib/active_record/persistence.rb @@ -431,9 +431,8 @@ module ActiveRecord 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 @@ module ActiveRecord 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 @@ module ActiveRecord 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 diff --git a/activerecord/test/cases/scoping/default_scoping_test.rb b/activerecord/test/cases/scoping/default_scoping_test.rb index 95a2f4df17..67e06c8a90 100644 --- a/activerecord/test/cases/scoping/default_scoping_test.rb +++ b/activerecord/test/cases/scoping/default_scoping_test.rb @@ -94,6 +94,12 @@ class DefaultScopingTest < ActiveRecord::TestCase 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 @@ class DefaultScopingTest < ActiveRecord::TestCase 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 @@ class DefaultScopingTest < ActiveRecord::TestCase 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 @@ class DefaultScopingTest < ActiveRecord::TestCase 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 @@ class DefaultScopingTest < ActiveRecord::TestCase 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 @@ class DefaultScopingTest < ActiveRecord::TestCase 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 diff --git a/activerecord/test/models/developer.rb b/activerecord/test/models/developer.rb index 60aa74bbca..c84dfe4170 100644 --- a/activerecord/test/models/developer.rb +++ b/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