rails icon indicating copy to clipboard operation
rails copied to clipboard

Confusing behavior using SQL functions (eg: pluck, count) on associations of unpersisted objects

Open anchit-desai opened this issue 1 year ago • 5 comments

Hi, this is my first time here, sorry if I make any mistakes :)

Steps to reproduce

Let's say I have a model Region and a model Location. A region has_many locations, and a location belongs_to a region. Consider the following code:

region = Region.new
location = Location.create!
region.locations = [location]

At this point, region is not persisted and I understand why functions like pluck, count, and order (there are probably more) don't work as expected on region.locations.

Expected Behavior

I think these functions (count, pluck, etc.) should either

  • give the expected value (e.g. region.locations.count should return 1 and region.locations.pluck(:id) should return [location.id]) OR
  • raise an error

Actual Behavior

The actual behavior does not seem intuitive to me.

region.locations.count # gives 0
region.locations.pluck(:id) # gives []

Thanks for considering!

Reproduction script

# frozen_string_literal: true

require "bundler/inline"

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

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  gem "rails", github: "rails/rails", branch: "main"
  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_count
    post = Post.new
    post.comments << Comment.create!

    # this fails
    assert_equal 1, post.comments.count
  end

  def test_pluck
    post = Post.new
    post.comments << Comment.create!

    # this fails
    assert_equal [Comment.first.id], post.comments.pluck(:id)
  end
end

System configuration

Rails version: 5.2.5

Ruby version: 2.7.2

anchit-desai avatar Jun 16 '22 23:06 anchit-desai

I think because they are a AR methods which returns result from sql, not from the object?

vishalzambre avatar Jun 17 '22 11:06 vishalzambre

@vishalzambre I understand the reason, just saying that they could be modified for a more intuitive response in this case.

anchit-desai avatar Jun 17 '22 17:06 anchit-desai

they could be modified for a more intuitive response in this case.

I'm curious in what way? Because

give the expected value (e.g. region.locations.count should return 1 and region.locations.pluck(:id) should return [location.id])

From my point of view it really shouldn't as my expectation is that pluck() makes a database query based on relation I've built, which in this case should query location table but also scoped per region foreign key. And as long as region.id is empty, the query turns into select id from locations where region_id IS NULL and returns an empty array as shown in your example

Also doing something like

region = Region.new(id: 25)
location = Location.create!(region_id: 25)
region.locations = [location]

region.locations.pluck(:id) # => [some_id]

should be possible as now relation is using id attribute even though region is not persisted

I'd say that correct way of getting data in this case would be to use .map and .size as I believe this would be an implicit indication that you want to get data based on in-memory objects without performing queries, so:

region.locations.size
region.locations.map(&:id)

nvasilevski avatar Jun 17 '22 18:06 nvasilevski

That's a nice example @nvasilevski (Region.new(id: 25))! I agree that the right way to get data in this case is .map and .size. I also totally understand why the output with .pluck and .count is what it is.

I think I just expect a little more from an ORM function than just executing the SQL but that could just be me :smile:

Another option is to pass an optional argument to pluck, indicating whether we want special behavior in this case, e.g. pluck(pure: true) if want to just run SQL or pluck(pure: false) if we want special behavior when the object is not persisted.

I think the advantage of this over using map is that we can pluck when the object is persisted (pluck usually being more performant), and map when it is not. Essentially what we're doing is region.persisted? ? region.locations.pluck(:id) : region.locations.map(&:id), but if we don't know if region is persisted each time we want to do an operation on it, it might be a lot to type each time.

Anyway, I'm just sharing my thoughts and if no one agrees, I'm happy to close this issue :)

anchit-desai avatar Jun 17 '22 20:06 anchit-desai

This issue has been automatically marked as stale because it has not been commented on for at least three months. The resources of the Rails team are limited, and so we are asking for your help. If you can still reproduce this error on the 7-0-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open. Thank you for all your contributions.

rails-bot[bot] avatar Sep 16 '22 20:09 rails-bot[bot]