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

[Fix #51524] Fix strict_loading violations ignored when using pluck #51627

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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
22 changes: 12 additions & 10 deletions activerecord/lib/active_record/associations/association.rb
Expand Up @@ -210,6 +210,18 @@ def create!(attributes = nil, &block)
_create_record(attributes, true, &block)
end

def violates_strict_loading?
return unless find_target?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the find_target? check to ensure that the loaded association adheres to the strict loading rule, which is necessary for passing the newly added test, test_preload_audit_logs_are_strict_loading_with_pluck.

Previously, the violates_strict_loading? method functioned correctly without this check because find_target? was used earlier in the process.

if find_target?
@target = merge_target_lists(find_target, target)
end

def find_target
if violates_strict_loading?
Base.strict_loading_violation!(owner: owner.class, reflection: reflection)
end

I'm not sure whether to open a separate PR for this refactoring since it's a minor change. For now, I've added it as a second commit.


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

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 +263,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 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?
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