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

Calling pluck on a strict_loading! association will execute the N+1 instead of raising ActiveRecord::StrictLoadingViolationError #51524

Open
dombesz opened this issue Apr 9, 2024 · 3 comments · May be fixed by #51627

Comments

@dombesz
Copy link

dombesz commented Apr 9, 2024

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "rails"
  # Uncomment the following line to test it against the fix provided at https://github.com/rails/rails/pull/50389
  # gem "rails", github: "joshuay03/rails", branch: "strict-loading-not-raising-on-nested-has-many-with-query-methods"

  gem "sqlite3"
end

require "active_record"
require "minitest/autorun"
require "logger"

# This connection will do for database-independent bug reports.
ActiveRecord::Base.establish_connection(adapter: "sqlite3", database: ":memory:")
ActiveRecord::Base.logger = Logger.new(STDOUT)

ActiveRecord::Schema.define do
  create_table :posts, force: true do |t|
  end

  create_table :comments, force: true do |t|
    t.integer :post_id
  end
end

class Post < ActiveRecord::Base
  has_many :comments
end

class Comment < ActiveRecord::Base
  belongs_to :post
end

class BugTest < Minitest::Test
  def test_association_stuff
    post = Post.create!
    post.comments << Comment.create!

    posts = Post.all.strict_loading!

    assert_raises(ActiveRecord::StrictLoadingViolationError) do
      posts.first.comments.to_a
    end

    assert_raises(ActiveRecord::StrictLoadingViolationError) do
      posts.first.comments.pluck(:id)
    end
  end
end

Expected behavior

The second assertion should also pass. Calling posts.first.comments.pluck(:id) should also raise an ActiveRecord::StrictLoadingViolationError just as the to_a does.

Actual behavior

Calling posts.first.comments.pluck(:id) does not raise an error and calls N+1 queries instead

System configuration

Rails version:
7.1.3.2
Ruby version:
3.2.2

There is a similar issue raised #49520 and I also referenced the provided fix in #50389 in the POC and the issue still remains. Hence raising this issue as separate.

@dombesz dombesz changed the title Calling pluck on an association of Post.all.strict_loading! will execute the N+1 instead of raising ActiveRecord::StrictLoadingViolationError Calling pluck on a strict_loading! association will execute the N+1 instead of raising ActiveRecord::StrictLoadingViolationError Apr 9, 2024
@nikhil-dronamraju
Copy link

Taking a look at this

@shes50103
Copy link
Contributor

Hey @nikhil-dronamraju I've been reviewing the issue and realized I hadn't shouted - apologies if you've already started looking into this 🙏

I'm nearly ready to submit my PR to address the fix. However, if you've already prepared a PR for the same issue, just let me know, and I'll hold off on mine.

shes50103 added a commit to shes50103/rails that referenced this issue Apr 22, 2024
… execute the N+1 instead of raising ActiveRecord::StrictLoadingViolationError
shes50103 added a commit to shes50103/rails that referenced this issue Apr 22, 2024
shes50103 added a commit to shes50103/rails that referenced this issue Apr 22, 2024
@nikhil-dronamraju
Copy link

Oh yeah, you've got it! I'll take a look at the PR too, I wasn't able to finish investigating. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants