active-hash / active_hash

A readonly ActiveRecord-esque base class that lets you use a hash, a Yaml file or a custom file as the datasource
MIT License
1.21k stars 179 forks source link

Fix #where for hashes with string keys #292

Closed usernam3 closed 1 year ago

usernam3 commented 1 year ago

✌️ Hey active_hash maintainers ✌️ Thank you for such great gem that makes development with ruby even more productive 🙇

I noticed that the latest version (v3.2.1) has undocumented side-effect which affect the behaviour of one of the core features - search. To be specific the #where for hash with string keys stopped working (example: ActiveHashModel.where("id" => 1)). It seems that transition from Object#public_send to ActiveHash::Base#read_attribute (in #281) isn't backward-compatible as those methods have a bit different expectation towards arguments. Object#public_send - converts arguments to symbols, read_attribute - not (while attributes internally are stored with symbolized keys).

Small demo ```ruby (rdbg) pp record #1, :name=>"qwerty"}> (rdbg) pp record.read_attribute("id") nil (rdbg) pp record.public_send(:id) 1 (rdbg) pp record.attributes["id"] nil (rdbg) pp record.attributes[:id] 1 (rdbg) pp record.id 1 ```
kbrock commented 1 year ago

Thank you for the bug request.

I'm thinking we should move the conversion lower in the stack. It looks like read_attribute should work with string or symbol while _read_attribute only works with String.

Think moving the to_s to _read_attribute is the simplest way. (others chime in if you want to have separate methods defined with the string and id conversion in it)

usernam3 commented 1 year ago

@kbrock Cool idea, thanks for suggesting it, it definitely makes things consistent.

Conversions are inside the _read_attribute now, initially I split it into two separate methods

like:

    def _read_attribute(key)
      attributes[key.to_s]
    end

    def read_attribute(key)
      attributes[key.to_sym]
    end

But didn't find a reason to keep them both, as attributes[key.to_s] doesn't make much sense (?or does it?) as attributes have keys symbolized.

kbrock commented 1 year ago

@usernam3 thanks for the change.

vm = ActiveRecordModel.first
vm.read_attribute(:name)
# => "abc"
vm.read_attribute("name")
# => "abc"
vm._read_attribute(:name)
# => nil
vm._read_attribute("name")
# => "abc"

Yes, technically read_attribute does some conversion and _read_attribute is more skeletal, but this works.