Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #where for hashes with string keys #292

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

usernam3
Copy link
Contributor

@usernam3 usernam3 commented Oct 24, 2023

✌️ 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
(rdbg) pp record
#<Category:0x000000012a0d7db8 @attributes={:id=>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

Signed-off-by: Stanislav Dobrovolschii <[email protected]>
Signed-off-by: Stanislav Dobrovolschii <[email protected]>
@usernam3 usernam3 marked this pull request as ready for review October 24, 2023 00:08
@kbrock
Copy link
Collaborator

kbrock commented Oct 24, 2023

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
Copy link
Contributor Author

@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
Copy link
Collaborator

kbrock commented Oct 26, 2023

@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.

Copy link
Collaborator

@kbrock kbrock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

@kbrock kbrock merged commit 178595c into active-hash:master Nov 15, 2023
13 checks passed
@kbrock kbrock added the bug label Nov 15, 2023
kbrock added a commit to kbrock/active_hash that referenced this pull request Apr 29, 2024
- 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
kbrock added a commit to kbrock/active_hash that referenced this pull request Apr 29, 2024
- 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
@kbrock kbrock mentioned this pull request Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants