search

t6d / smart_properties

Ruby accessors on steroids
MIT License
177 stars 20 forks source link

Proper PropertyCollection caching #29

Closed t6d closed 9 years ago

t6d commented 9 years ago

A PropertyCollection now keeps tracks of its children and updates them whenever a new property is added to a class. This enables faster querying for a particular property. Previously, this query would have caused walking the entire PropertyCollection chain and merging several hashes.

Internally, a PropertyCollection references its properties using strings because these are GCed. Assigning unsafe input to a SmartProperties enabled object could otherwise lead to excisive memory usage because PropertyCollection#key? would have to convert to a symbol which would not be GCed.

benedikt commented 9 years ago

I'm not sure about the argument about storing keys as Strings in this case. With the current implementation the to_sym calls will create the symbols anyways. I'm also not sure if there's ever a case where properties are defined using unsafe data?

And then again, Symbols are GC'ed since Ruby 2.2 anyways…

t6d commented 9 years ago

The problem occurs when you map API input to from JSON.parse to an object that is SmartProperties enabled. Right now SmartProperties do not handle these cases correctly anyways, but this will change with the PR I'm opening up in a few minutes. The next PR will address forwarding unprocessed keyword arguments.

I'm removing keys as strings in SmartProperties 2.0. The public interface has always been returning names as Symbols. The current keys implementation ensures that the public interface stays as is.

t6d commented 9 years ago

To clarify, I'm not worried that you as a developer somehow call property with unsafe data, but the initialize method might be called with such data, and right now this would force me to call to_sym instead of to_s when testing whether a given key is a property. This is why I switched it around. As I said, this will go away with SmartProperties 2.0 which is likely not going to be 1.9.3 compatible.

benedikt commented 9 years ago

Ah, you're right. It does try to find the properties for the given data on initialize, which might very well be unsafe. Disregard all of my comments :)

t6d commented 9 years ago

Thanks for the review!