aetherknight / recursive-open-struct

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

When using recurse_over_arrays true, adding and populating hash in an array doesn't work in some cases #29

Open abonas opened 9 years ago

abonas commented 9 years ago

Even when defining the object with recurse_over_arrays set to true, certain ways of filling the data in an array don't work. (the nicer , more object oriented ones)

Option A - works obj.spec = {} obj.spec.ports = [{ 'port' => 3000, 'targetPort' => 'http-server', 'protocol' => 'TCP' }]

Option B - doesn't work obj.spec = {} obj.spec.ports = [] obj.spec.ports << { 'port' => 3000, 'targetPort' => 'http-server', 'protocol' => 'TCP' }

Option C - doesn't work obj.spec = {} obj.spec.ports = [] obj.spec.ports[0]= {} obj.spec.ports[0].port = 3000

aetherknight commented 9 years ago

Could you provide examples what the expected results are for each of your Options, and how they break down?

At present, the way recurse_over_arrays works is that the first time you call an accessor method that refers to an Array, it memoizes a copy of that array where each hash within the original array is replaced with an ROS. If you use << to mutate such an array, any hashes already in the original array will be converted to ROS, but any new hashes will only be placed in the memoized array, and they won't be converted into ROSes.

I believe the original purpose of the memoization was to ensure that two accessor method calls will return the same ROS object instead of returning a different one each time.

One way of solving Option B would be to subclass Array for use within ROS (to reimplement methods like <<), but I'm concerned that this will open yet another can of worms.

Another way might be to check the memoized array to see if it contains any hashes that have not been replaced yet.

I also suspect that modifications to the memoized array may not show up under certain circumstances when .to_h is called on the parent ROS object, or when .dup is used to create a deep-duplicate of the object.

abonas commented 9 years ago

@aetherknight, my expectation would be that option B and option C will work without getting a "no method error", so the user can build those structures in a nice object oriented way.

Consider a use case where the user constructs ROS without having a json file, but simply one attribute at a time, just as populating another class in an object oriented way.

aetherknight commented 9 years ago

I made a change to how the getter works so that it will always recompute the array that might contain ROS objects, and tested it against the 3 use-cases mentioned above. However, it does not modify the underlying hash @table such that the parent ROS objects @tables are properly modified. This means that to_hash on a parent ROS object is broken.