Condition explicitly uses attribute values and not public_send

[flavorjones]

fix: Condition explicitly uses attribute values and not public_send

Using public_send caused an issue when attribute names shadowed method names.

Closes #280

[flavorjones]

@kbrock When you get some time, would love any feedback you have on this potential fix. (No rush, I know you've been busy!)

[kbrock]

Wow, great work/find.

The good old column name conflicts with Object namespace trick.

I currently have one concern (as you can see from the comments):

Will this break custom accessor methods?

ActiveRecord#read_attribute() has a pretty serious implementation that detects custom accessor methods. Think their [] calls read_attribute()

We have tests to handle custom attribute accessor methods, so if those passed with this test, I guess that means this code change is good?

You know, I don't think rails would work with this case either. Isn't that the whole reason why Object#id changed to Object#object_id?

Having said that, I'm leaning towards merging this.

@pfeiffer Let us know if you can see anything wrong with this.

[flavorjones]

Yeeaaaah, @kbrock good catch, this does break custom accessor methods. Gonna give this a good think.

[flavorjones]

OK, taking a step back for a moment ... the code that calls public_send was added in aa512c4 as part of #268. Before that commit, where did not support arbitrary method definitions.

Here are the tests I'm running:

  describe "colliding methods" do
    klass = Class.new(ActiveHash::Base) do
      self.data = [
        {
          id: 1,
          name: "Aaa",
          display: true,
        },
        {
          id: 2,
          name: "Bbb",
          display: false,
        },
      ]

      def name
        self[:name] + "!"
      end

      def not_a_real_attribute
        self[:name]
      end
    end

    it "should handle attributes named after existing methods" do
      expect(klass.where(display: true).length).to eq(1)
    end

    it "should handle redefined attributes" do
      expect(klass.where(name: 'Bbb!').length).to eq(1)
    end

    it "should handle arbitrary methods" do
      expect(klass.where(not_a_real_attribute: 'Bbb').length).to eq(1)
    end
  end

On e58f3ff (before the Relation overhaul was merged), the results are:

Failures:

  1) ActiveHash::Relation colliding methods should handle redefined attributes
     Failure/Error: expect(klass.where(name: 'Bbb!').length).to eq(1)
     
       expected: 1
            got: 0
     
       (compared using ==)
     # ./spec/active_hash/relation_spec.rb:83:in `block (3 levels) in <top (required)>'

  2) ActiveHash::Relation colliding methods should handle arbitrary methods
     Failure/Error: expect(klass.where(not_a_real_attribute: 'Bbb').length).to eq(1)
     
       expected: 1
            got: 0
     
       (compared using ==)
     # ./spec/active_hash/relation_spec.rb:87:in `block (3 levels) in <top (required)>'

and after #268 is merged:

Failures:

  1) ActiveHash::Relation colliding methods should handle attributes named after existing methods
     Failure/Error: expect(klass.where(display: true).length).to eq(1)
     
       expected: 1
            got: 0
     
       (compared using ==)
     # ./spec/active_hash/relation_spec.rb:79:in `block (3 levels) in <top (required)>'

So my thinking is that it's OK to break support for arbitrary custom accessor methods, since:

• it was only introduced (not intentionally) in v3.2.0, and presents the bug in 3.2.0: Regression in .where? #280
• Active Record doesn't support custom accessor methods with where, meaning that potential migrations may break behavior relied upon in Active Hash

I'd like to suggest that we merge this PR and cut a release with a clear CHANGELOG description.

[kbrock]

So I ran the same test against active record.
This works great.

Thanks for making the read_attribute changes.

[pfeiffer]

Hey, a bit late to this PR and the request for comments. I think using read_attribute makes sense here, so great change!