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!)

[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 aa512c4d 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 #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!