Since v3.5.4 it is no longer possible to call Hashie::Mash[]= more than once with the same key #413
Comments
BRMatt
changed the title
|
Oy vey. Thanks for reporting this. This has been a major thorn in our side. I'll see what I can do to mitigate the problem. If you're digging into it, we'd be happy to accept a PR with a failing test or a fix! |
|
Oh wow when will we get over this saga? :( Thanks for helping out @michaelherold, LMK if you need anything. I'm making myself a note do blog about this once it's over ;) |
Sorry I didn't submit one with this issue, I was a bit tired after investigating this! I might be able to, but I might be missing a bit of context about this feature. Is this only supposed to warn about methods that are defined on the mash or it's subclasses? If the performance overhead was coming from the overhead of |
|
Yeah, it's supposed to throw a warning when you set a key that collides with a built-in method. I think the cache is probably going to be the way to go. I have a failing test and will try to carve out some time to work on it tonight. Otherwise I'll look before I head in to work in the morning. |
When we switched to using `#respond_to?` to detecting whether to log a Mash collision, we started reporting when we were overwriting keys that already exist in the Mash. This is a poor experience because it causes extra warnings (as in hashie#414) or, in the worst case, causes an "undefined method" error (as in hashie#413). This change fixes that problem and benchmarks to ensure we're not appreciably regressing performance. The results of two benchmarks are below: ``` bundle exec ruby benchmark/mash_method_access.rb: Warming up -------------------------------------- before 92.456k i/100ms Calculating ------------------------------------- before 1.290M (± 4.4%) i/s - 6.472M in 5.028183s Pausing here -- run Ruby again to measure the next benchmark... Warming up -------------------------------------- after 92.941k i/100ms Calculating ------------------------------------- after 1.326M (± 5.4%) i/s - 6.692M in 5.060756s Comparison: after: 1326239.2 i/s before: 1289624.0 i/s - same-ish: difference falls within error ``` and ``` within spec/integrations/omniauth, bundle exec rake perf:ips Warming up -------------------------------------- before 1.260k i/100ms Calculating ------------------------------------- before 13.114k (± 4.2%) i/s - 66.780k in 5.101689s Pausing here -- run Ruby again to measure the next benchmark... Warming up -------------------------------------- after 1.299k i/100ms Calculating ------------------------------------- after 13.149k (± 4.0%) i/s - 66.249k in 5.046630s Comparison: after: 13148.9 i/s before: 13113.8 i/s - same-ish: difference falls within error ``` Closes hashie#413 Closes hashie#414
|
I tried using a Set to memoize the method set on initialization. Hashie performance stayed the same, but it caused a 50% performance regression in OmniAuth. I ended up going a different way, as seen in #415. Here are the benchmark results with the Set change, for reference: Mash BenchmarkOmniAuth Benchmark |
When we switched to using `#respond_to?` to detecting whether to log a Mash collision, we started reporting when we were overwriting keys that already exist in the Mash. This is a poor experience because it causes extra warnings (as in hashie#414) or, in the worst case, causes an "undefined method" error (as in hashie#413). This change fixes that problem and benchmarks to ensure we're not appreciably regressing performance. The results of two benchmarks are below: ``` bundle exec ruby benchmark/mash_method_access.rb: Warming up -------------------------------------- before 92.456k i/100ms Calculating ------------------------------------- before 1.290M (± 4.4%) i/s - 6.472M in 5.028183s Pausing here -- run Ruby again to measure the next benchmark... Warming up -------------------------------------- after 92.941k i/100ms Calculating ------------------------------------- after 1.326M (± 5.4%) i/s - 6.692M in 5.060756s Comparison: after: 1326239.2 i/s before: 1289624.0 i/s - same-ish: difference falls within error ``` and ``` within spec/integrations/omniauth, bundle exec rake perf:ips Warming up -------------------------------------- before 1.260k i/100ms Calculating ------------------------------------- before 13.114k (± 4.2%) i/s - 66.780k in 5.101689s Pausing here -- run Ruby again to measure the next benchmark... Warming up -------------------------------------- after 1.299k i/100ms Calculating ------------------------------------- after 13.149k (± 4.0%) i/s - 66.249k in 5.046630s Comparison: after: 13148.9 i/s before: 13113.8 i/s - same-ish: difference falls within error ``` Closes hashie#413 Closes hashie#414
Since 3.5.4 it is no longer possible to assign a value to a key more than once:
produces:
The issue is that fc4f6e2 changed the code that logs warnings to use
respond_to?to check if the call toHashie::Mash[]=would override a method that exists onHashie::Mash, or the thing that's subclassing it.The documentation for
Object#respond_to?says:Unfortunately
Hashie::Mashoverridesrespond_to_missing?to returntrueif a key with that name exists in the hash.When the warning logger is invoked it calls
self#method(method_key)to try and look up details about the method. This raises an error as the method does not exist (Hashie::Mashdoes not actually define methods on the object, it implements dynamic method lookup using#method_missing)This is made worse by the fact that you can't disable warnings globally for all
Hashie::Mashobjects, only subclasses. Unfortunately some fairly prominent users ofHashie::Mashdo not subclass it.I think the previous version of this worked because
methods.include?excluded the "magic" methods that Hashie::Mash will respond to.The text was updated successfully, but these errors were encountered: