aetherknight / recursive-open-struct

OpenStruct subclass that returns nested hash attributes as RecursiveOpenStructs
Other
276 stars 54 forks source link

Doesn't work for a property `test` #51

Closed 31piy closed 7 years ago

31piy commented 7 years ago

I am using v1.0.2 of RecursiveOpenStruct and found a strange bug with it. This bug can be reproduced using Rails console as follows:

$ rails c
Running via Spring preloader in process 488
Loading development environment (Rails 5.0.2)

[1] pry(main)> str = RecursiveOpenStruct.new({ test: 123 })
=> #<RecursiveOpenStruct test=123>

[2] pry(main)> str.send('test')
ArgumentError: wrong number of arguments (given 0, expected 2..3)
from (pry):2:in `test'

[3] pry(main)> str.test
=> 123

[4] pry(main)> str.send('test')
=> 123

As you can see, the property test cannot be accessed via #send at first, but can be accessed once str.send is invoked. Any thoughts?

aetherknight commented 7 years ago

What is happening is that str.send('test') calls the private Kernel#test because it exists instead of triggering RecursiveOpenStruct#method_missing, which opportunistically creates public getters and setters on the instance's metaclass for the keys in the internal hash.

You should use #public_send instead, in order to call test as if it were a public method. #send ignores the public/private/protected status of methods when looking up which method to execute.

31piy commented 7 years ago

@aetherknight Thanks for your reply. The solution seems to work fine for test, but now, a property method cannot be accessed.

So overall, if my hash contains the properties named after ruby methods, they will create problem?

aetherknight commented 7 years ago

Correct, any existing publicly scoped instance method that ROS defines or that is inherited from Object will already exist and therefore be what is called instead of #method_missing, which is what will perform the lookup on the Hash that ROS maintains internally.

This problem exists with OpenStruct as well, but it is also what lets you call #send or #public_send on the the object (and is why it is generally considered a bad thing for libraries like Rails or RSpec to use mixins to add more methods to Object).

While ROS could declare its own methods when the object is initialized, this could make certain important methods unavailable or behave incorrectly. It would also no longer mimic the behavior of OpenStruct, which it is based upon.

~You should be able to fall back to using subscript notation in order to access values that are not accessible for this reason or when you want to dynamically access values.~ Actually, I just found a bug/discrepency with OpenStruct: the subscript notation doesn't work for ROS with Ruby 2.3.x:

os = OpenStruct.new({'test' => 123, 'method' => 123})
=> #<OpenStruct test=123, method=123>
os[:test]
=> 123
os[:method]
=> 123
os.method
ArgumentError: wrong number of arguments (given 0, expected 1)
from (pry):19:in `method'

ros = RecursiveOpenStruct.new({'test' => 123, 'method' => 123})
# => #<RecursiveOpenStruct test=123, method=123>
ros[:test]
# => 123
ros[:method]
# ArgumentError: wrong number of arguments (given 0, expected 1)
# from .../recursive-open-struct/lib/recursive_open_struct.rb:46:in `method'

I don't have an immediate solution to this problem (ROS's subscript notation uses public_send while OpenStruct just fetches the value from its internal Hash), but I'll think on if there's a clean way to fix this.

My advice is to use ROS if you need the syntactical convenience, but if you're trying to dynamically call accessor methods, I would recommend using a Hash with subscript notation if possible instead because it avoids all of the complexity of method-lookup (and trying to treat method-lookup as if it were a Hash).

aetherknight commented 7 years ago

I just released version 1.0.3, which addresses this last issue (and ensures that the test suite passes in Ruby 2.4.x). It should now be possible to use subscript notation to look up arbitrary values (and have ROS recurse when that value is a nested Hash or an Array (if enabled).

cben commented 7 years ago

For the record, a way to access any element that works both before and after the fix (I'm on project where where backporting makes me care about 1.0.0) is to ask for hash representation:

[21] pry(main)> RecursiveOpenStruct.new({a: {methods: 12, test: 34}}).a_as_a_hash[:methods]
=> 12
[22] pry(main)> RecursiveOpenStruct.new({a: {methods: 12, test: 34}}).a_as_a_hash[:test]
=> 34
[23] pry(main)> RecursiveOpenStruct.new({methods: 12, test: 34}).to_h[:methods]
=> 12
[24] pry(main)> RecursiveOpenStruct.new({methods: 12, test: 34}).to_h[:test]
=> 34

(_as_a_hash is more efficient for nested structs, only to_h works at top level)

Note that once you get the hash, it's a hash all the way down, you won't be able to get subelements with dot notation:

[30] pry(main)> RecursiveOpenStruct.new({methods: {x: 3, y: 4}}).to_h[:methods]
=> {:x=>3, :y=>4}
[31] pry(main)> RecursiveOpenStruct.new({methods: {x: 3, y: 4}}).to_h[:methods][:x]
=> 3
[32] pry(main)> RecursiveOpenStruct.new({methods: {x: 3, y: 4}}).to_h[:methods].x
NoMethodError: undefined method `x' for {:x=>3, :y=>4}:Hash
cben commented 7 years ago

Another weird thing that you've fixed here:

[3] pry(main)> RecursiveOpenStruct::VERSION
=> "1.0.0"
[5] pry(main)> RecursiveOpenStruct.new({a: {x: 3, y: 4}})["table"]
=> {:a=>{:x=>3, :y=>4}}
[6] pry(main)> RecursiveOpenStruct.new({a: {x: 3, y: 4}})[:table]
=> {:a=>{:x=>3, :y=>4}}
[7] pry(main)> RecursiveOpenStruct.new({a: {x: 3, y: 4}}).table
=> nil

This was somehow accessing internal @table, probably via protected attr_reader :table in ostruct.rb. Now it's not:

[2] pry(main)> RecursiveOpenStruct::VERSION
=> "1.0.4"
[3] pry(main)> RecursiveOpenStruct.new({a: {x: 3, y: 4}})["table"]
=> nil
[4] pry(main)> RecursiveOpenStruct.new({a: {x: 3, y: 4}})[:table]
=> nil
[5] pry(main)> RecursiveOpenStruct.new({a: {x: 3, y: 4}}).table
=> nil

[Our project actually used this as a workaround to the old behavior, we'll switch to _as_a_hash, don't bring the bug back ;-)]