Skip to content

Commit

Permalink
[Fix rails#51524] Fix violations ignored when using
Browse files Browse the repository at this point in the history
  • Loading branch information
shes50103 committed Apr 22, 2024
1 parent 82054e8 commit f7e83bc
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 14 deletions.
4 changes: 4 additions & 0 deletions activerecord/CHANGELOG.md
@@ -1,3 +1,7 @@
* Fix `strict_loading` violations ignored when using `pluck`

*Johnson Chan*

* Strict loading using `:n_plus_one_only` does not eagerly load child associations.

With this change, child associations are no longer eagerly loaded, to
Expand Down
28 changes: 14 additions & 14 deletions activerecord/lib/active_record/associations/association.rb
Expand Up @@ -210,6 +210,20 @@ def create!(attributes = nil, &block)
_create_record(attributes, true, &block)
end

def violates_strict_loading?
return if @skip_strict_loading

return unless owner.validation_context.nil?

return reflection.strict_loading? if reflection.options.key?(:strict_loading)

owner.strict_loading? && !owner.strict_loading_n_plus_one_only?
end

def find_target?
!loaded? && (!owner.new_record? || foreign_key_present?) && klass
end

private
# Reader and writer methods call this so that consistent errors are presented
# when the association target class does not exist.
Expand Down Expand Up @@ -251,16 +265,6 @@ def skip_strict_loading(&block)
@skip_strict_loading = skip_strict_loading_was
end

def violates_strict_loading?
return if @skip_strict_loading

return unless owner.validation_context.nil?

return reflection.strict_loading? if reflection.options.key?(:strict_loading)

owner.strict_loading? && !owner.strict_loading_n_plus_one_only?
end

# The scope for this association.
#
# Note that the association_scope is merged into the target_scope only when the
Expand All @@ -287,10 +291,6 @@ def scope_for_create
scope.scope_for_create
end

def find_target?
!loaded? && (!owner.new_record? || foreign_key_present?) && klass
end

# Returns true if there is a foreign key present on the owner which
# references the target. This is used to determine whether we can load
# the target if the owner is currently a new record (and therefore
Expand Down
4 changes: 4 additions & 0 deletions activerecord/lib/active_record/relation/calculations.rb
Expand Up @@ -285,6 +285,10 @@ def calculate(operation, column_name)
#
# See also #ids.
def pluck(*column_names)
if respond_to?(:proxy_association) && proxy_association.violates_strict_loading? && proxy_association.find_target?
Base.strict_loading_violation!(owner: proxy_association.owner.class, reflection: proxy_association.reflection)
end

if @none
if @async
return Promise::Complete.new([])
Expand Down
19 changes: 19 additions & 0 deletions activerecord/test/cases/strict_loading_test.rb
Expand Up @@ -317,6 +317,25 @@ def test_strict_loading_with_has_one_through_does_not_prevent_creation_of_associ
end
end

def test_on_lazy_loading_and_strict_loading_with_pluck
with_strict_loading_by_default(Developer) do
dev = Developer.first

assert_raises ActiveRecord::StrictLoadingViolationError do
dev.audit_logs.pluck(:id)
end
end
end

def test_preload_audit_logs_are_strict_loading_with_pluck
with_strict_loading_by_default(Developer) do
dev = Developer.preload(:audit_logs).first

assert_nothing_raised do
dev.audit_logs.pluck(:id)
end
end
end

def test_preload_audit_logs_are_strict_loading_because_parent_is_strict_loading
developer = Developer.first
Expand Down

0 comments on commit f7e83bc

Please sign in to comment.