pluck change from array access to using methods. #299

iberianpig · 2024-02-06T02:28:20Z

Restore ActiveHash::Relation#pluck to execute the specified method on the record.

Background:

Through v3.1.x: Executed method names given as symbols.
In v3.2.x: Change impacted data retrieval by directly referencing attributes[key], affecting compatibility.

#269 broke functionality of gems like enumerize which rely on casting values accessed by column names.

Reproduce

Save following file as issue.rb.
Please switch version with uncomment line of gem "active_hash", "3.2.1" # fail test.
Run in terminal $ ruby issue.rb

#!/usr/bin/env ruby

begin
  require "bundler/inline"
rescue LoadError => e
  $stderr.puts "Bundler version 1.10 or later is required. Please update your Bundler"
  raise e
end

gemfile(true) do
  source "https://rubygems.org"
  gem 'enumerize'
  # gem "active_hash", "3.1.1" # pass test
  # gem "active_hash", "3.2.1" # fail test
  gem "active_hash", github: "active-hash/active_hash", ref: "6c51fa6d" # == master # fail test
  # gem "active_hash", github: "iberianpig/active_hash", branch: "fix/revert-pluck-behavior" # pass test
end

require "active_hash"
require "minitest/autorun"
require "logger"

class Country < ActiveHash::Base
  extend Enumerize
  enumerize :continent, in: { north_america: 1, south_america: 2, europe: 3, asia: 4, africa: 5, oceania: 6 }
  self.data = [
      {
        "id": 1,
        "name": "US",
        "custom_field_1": 1,
        "continent": 1
      },
      {
        "id": 2,
        "name": "Canada",
        "custom_field_2": 2,
        "continent": 1
      }
    ]
end
class BugTest < Minitest::Test
  def test_pluck_with_enumerize
    assert_equal "north_america", Country.find(1).continent  # continent is enumerized
    assert_equal [["US", "north_america"], ["Canada", "north_america"]], Country.all.pluck(:name, :continent)
  end
end

3.1.1

Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Run options: --seed 37444

# Running:

.

Finished in 0.000997s, 1002.6591 runs/s, 2005.3181 assertions/s.

1 runs, 2 assertions, 0 failures, 0 errors, 0 skips

3.2.1

Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
Run options: --seed 14751

# Running:

F

Finished in 0.004705s, 212.5528 runs/s, 425.1055 assertions/s.

  1) Failure:
BugTest#test_pluck_with_enumerize [issue.rb:43]:
--- expected
+++ actual
@@ -1 +1 @@
-[["US", "north_america"], ["Canada", "north_america"]]
+[["US", 1], ["Canada", 1]]


1 runs, 2 assertions, 1 failures, 0 errors, 0 skips

iberianpig · 2024-02-06T03:29:26Z

(CI failed)It looks like Rails 7.1.3 breaks polymorphic associations ... 🤔

flavorjones · 2024-02-07T03:30:24Z

@iberianpig see #300 for the Rails 7.1.3 fix

kbrock · 2024-02-07T03:39:14Z

kicking now that 300 was merged

kbrock · 2024-02-07T03:44:01Z

looking like the tests introduced by 299 are still passing.
We had introduced a few other changes around a symbol or string column access, so maybe the changes in 299 are no longer needed?

- execute method of symbol name specified by pluck for record - Until v3.1.1, it was a specification to execute the method of the symbol name, so the same result is obtained.

…n` in Ruby 2.6

iberianpig · 2024-02-07T04:15:48Z

@kbrock Please confirm Reproduce section in this issue. You can reproduce this issue in your terminal.

looking like the tests introduced by 299 are still passing.

I rebased and pushed this branch, then All checks in CI have all passed.
However it's still failing with $ ruby issue.rb .

  1) Failure:
BugTest#test_pluck_with_enumerize [issue.rb:44]:
--- expected
+++ actual
@@ -1 +1 @@
-[["US", "north_america"], ["Canada", "north_america"]]
+[["US", 1], ["Canada", 1]]


1 runs, 2 assertions, 1 failures, 0 errors, 0 skips

kbrock · 2024-02-08T01:11:07Z

@iberianpig Sorry, I was not clear. Let me try again.

Good job. The tests introduced by #269 are still passing.
I said this because I thought this reverted the code introduced in #269 but it turns out this is not a revert, but rather a fix for the regression/bug introduced by #269

Request:

Is there a way to introduce a test (that does not rely upon enumerize) to our code base? I would like to protect us from changing this code and breaking this for you in the future.

iberianpig · 2024-02-09T05:04:51Z

Thank you for your feedback.
I've already add tests for #pluck that it does not rely on enumerize. Could you please take a moment to review them in This PR?

kbrock · 2024-02-09T23:19:27Z

lib/active_hash/relation.rb

-          symbolized_column_names.map { |column_name| record[column_name] }
-        end
+        # `tap with break` can be replaced with yield_self in Ruby 2.5 or then in Ruby 2.6
+        column_names.map { |column_name| all.map(&column_name.to_sym) }.tap { |values| break :zip.to_proc.(*values) }


I think the bug you highlighted is we do not want to call record[column_name] but rather record.public_send(column_name). (or send?)

Think this bug is based around active record overriding array accessor and calling the public method behind the scenes. We use the actual array method. So using the array accessor will not pick up the public attribute method override.

This code has gone through at least 3 iterations:

# first column_names.map { |column_name| all.map(&column_name.to_sym) }.inject(&:zip) #269 all.map do |record| symbolized_column_names.map { |column_name| record[column_name] } end #299 column_names.map { |column_name| all.map(&column_name.to_sym) }.then { :zip.to_proc.(*_1) }

I'm finding the zip and the tap/then a little complicated to mentally parse. (both versions of the zip code). Not sure what the extra traversals buy us.

Does something like this work for you?
(does send/public_send fix your bug?)

all.map { |record| column_names.map { |column_name| record.public_send(column_name) } }

Maybe the zip buys us some performance or something? Please share. Though, the non-zip version seems to be simpler for me.

Thank you for your great feedback.
I initially implemented pluck in a way that was closer to the original code.
However considering your feedback, proceeding as you've recommended seems better, especially from a performance standpoint.

Here are the results of the benchmark script:

Benchmark.bm 25 do |r| r.report "pluck(first)" do 100000.times { CountryWithContinent.pluck_with_first(:id, :name, :continent) } end r.report "pluck(#269)" do 100000.times { CountryWithContinent.pluck_with_269(:id, :name, :continent) } end r.report "pluck(#299)" do 100000.times { CountryWithContinent.pluck_with_299(:id, :name, :continent) } end r.report "pluck(public_send)" do 100000.times { CountryWithContinent.pluck(:id, :name, :continent) } end end

user system total real pluck(first) 2.655054 0.001351 2.656405 ( 2.656560) pluck(#269) 0.919349 0.000053 0.919402 ( 0.919459) pluck(#299) 1.843044 0.004067 1.847111 ( 1.847146) pluck(public_send) 0.853149 0.000000 0.853149 ( 0.863946)

I'll move forward with the public_send approach. This seems like a step in the right direction. Thanks for your suggestion 😄

oh wow. thanks.
Yes, you were definitely following the original code's style. I appreciate that.
After looking at the numbers, I don't feel so bad wanting to go with what I consider a simpler style.

kbrock

This looks great.

Since the pre 269 code uses the method access, this looks like it shouldn't introduce any unintended side effects

iberianpig · 2024-03-26T03:07:07Z

@kbrock When is the release likely to include the contents of this pull request? 😄

@flavorjones

- Support `has_many :through` associations active-hash#296 @flavorjones - Rails 7.1 support active-hash#291 @y-yagi - Rails 7.1: fix sqlite3 issue active-hash#303 @flavorjones - Rails 7.1.3: add missing `has_query_constraints?` active-hash#300 @flavorjones - `Array#pluck` supports methods active-hash#299 @iberianpig - Prefer `safe_constantize` over `constantize` active-hash#297 @flavorjones - Treat `nil` and `blank?` as different values active-hash#295 @kbrock - Fix `#where` for string keys active-hash#292 @usernam3

@m-nakamura145

- Ruby 3.3 support active-hash#298 @m-nakamura145 - Support `has_many :through` associations active-hash#296 @flavorjones - Rails 7.1 support active-hash#291 @y-yagi - Rails 7.1: fix sqlite3 issue active-hash#303 @flavorjones - Rails 7.1.3: add missing `has_query_constraints?` active-hash#300 @flavorjones - `Array#pluck` supports methods active-hash#299 @iberianpig - Prefer `safe_constantize` over `constantize` active-hash#297 @flavorjones - Treat `nil` and `blank?` as different values active-hash#295 @kbrock - Fix `#where` for string keys active-hash#292 @usernam3

iberianpig force-pushed the fix/revert-pluck-behavior branch from c447ce6 to 66c5234 Compare February 6, 2024 03:07

iberianpig marked this pull request as ready for review February 6, 2024 03:27

kbrock closed this Feb 7, 2024

kbrock reopened this Feb 7, 2024

iberianpig added 2 commits February 7, 2024 12:54

fix: pluck method of ActiveHash::Relation

de884bc

- execute method of symbol name specified by pluck for record - Until v3.1.1, it was a specification to execute the method of the symbol name, so the same result is obtained.

fix: use yield_self(ruby 2.5) instead of then(ruby 2.6)

8ea4256

iberianpig force-pushed the fix/revert-pluck-behavior branch from 66c5234 to 8ea4256 Compare February 7, 2024 03:54

fix: use tap with break instead of yield_self in Ruby 2.5 or `the…

28a3ea7

…n` in Ruby 2.6

test: add spec for pluck method with enum like behavior

9341e7a

kbrock reviewed Feb 9, 2024

View reviewed changes

refactor: use public_send instead of zip and to_proc in pluck

d14f25e

iberianpig force-pushed the fix/revert-pluck-behavior branch from 33f0960 to d14f25e Compare February 10, 2024 08:21

kbrock changed the title ~~Revert pluck behavior~~ pluck change from array access to using methods. Feb 12, 2024

kbrock approved these changes Feb 12, 2024

View reviewed changes

kbrock merged commit 195b1e9 into active-hash:master Feb 12, 2024
16 checks passed

kbrock mentioned this pull request Apr 29, 2024

Release 3.3.0 #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pluck change from array access to using methods. #299

pluck change from array access to using methods. #299

iberianpig commented Feb 6, 2024 •

edited

Loading

iberianpig commented Feb 6, 2024 •

edited

Loading

flavorjones commented Feb 7, 2024

kbrock commented Feb 7, 2024

kbrock commented Feb 7, 2024

iberianpig commented Feb 7, 2024 •

edited

Loading

kbrock commented Feb 8, 2024 •

edited

Loading

iberianpig commented Feb 9, 2024

kbrock Feb 9, 2024 •

edited

Loading

kbrock Feb 9, 2024

iberianpig Feb 10, 2024 •

edited

Loading

kbrock Feb 12, 2024

kbrock left a comment

iberianpig commented Mar 26, 2024

pluck change from array access to using methods. #299

pluck change from array access to using methods. #299

Conversation

iberianpig commented Feb 6, 2024 • edited Loading

Background:

Reproduce

3.1.1

3.2.1

iberianpig commented Feb 6, 2024 • edited Loading

flavorjones commented Feb 7, 2024

kbrock commented Feb 7, 2024

kbrock commented Feb 7, 2024

iberianpig commented Feb 7, 2024 • edited Loading

kbrock commented Feb 8, 2024 • edited Loading

iberianpig commented Feb 9, 2024

kbrock Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

kbrock Feb 9, 2024

Choose a reason for hiding this comment

iberianpig Feb 10, 2024 • edited Loading

Choose a reason for hiding this comment

kbrock Feb 12, 2024

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

iberianpig commented Mar 26, 2024

iberianpig commented Feb 6, 2024 •

edited

Loading

iberianpig commented Feb 6, 2024 •

edited

Loading

iberianpig commented Feb 7, 2024 •

edited

Loading

kbrock commented Feb 8, 2024 •

edited

Loading

kbrock Feb 9, 2024 •

edited

Loading

iberianpig Feb 10, 2024 •

edited

Loading