Prevent warnings and errors when setting Mash keys more than once #415

Merged
merged 2 commits into from Feb 24, 2017

Conversation

Projects
None yet
4 participants
@michaelherold
Contributor

michaelherold commented Feb 24, 2017

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 #414) or, in the worst case, causes an
"undefined method" error (as in #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 #413
Closes #414

@michaelherold

This comment has been minimized.

Show comment
Hide comment
@michaelherold

michaelherold Feb 24, 2017

Contributor

@BRMatt and @jrmhaig could you please check to see if this solves the two problems you encountered?

Contributor

michaelherold commented Feb 24, 2017

@BRMatt and @jrmhaig could you please check to see if this solves the two problems you encountered?

Don't log when overwriting Mash keys
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 #414) or, in the worst case, causes an
"undefined method" error (as in #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 #413
Closes #414
@jrmhaig

This comment has been minimized.

Show comment
Hide comment
@jrmhaig

jrmhaig Feb 24, 2017

Yes, this fixes #414. Thanks.

jrmhaig commented Feb 24, 2017

Yes, this fixes #414. Thanks.

@michaelherold michaelherold changed the title from Fix double set to Prevent warnings and errors when setting Mash keys more than once Feb 24, 2017

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Feb 24, 2017

Contributor

You're my hero @michaelherold!

Contributor

dblock commented Feb 24, 2017

You're my hero @michaelherold!

@dblock dblock merged commit e73f01b into intridea:master Feb 24, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@michaelherold michaelherold deleted the michaelherold:fix-double-set branch Feb 24, 2017

@michaelherold michaelherold referenced this pull request in omniauth/omniauth Feb 24, 2017

Closed

Performance Issue with version 1.5.0 #886

@BRMatt

This comment has been minimized.

Show comment
Hide comment
@BRMatt

BRMatt Feb 24, 2017

Thanks for the fix @michaelherold! 💙❤️💛💜💚💙❤️💛💜💚

BRMatt commented Feb 24, 2017

Thanks for the fix @michaelherold! 💙❤️💛💜💚💙❤️💛💜💚

@michaelherold

This comment has been minimized.

Show comment
Hide comment
@michaelherold

michaelherold Feb 24, 2017

Contributor

Awesome, I'm glad it works for both of you. I'll get a new version out today! 🏆

Contributor

michaelherold commented Feb 24, 2017

Awesome, I'm glad it works for both of you. I'll get a new version out today! 🏆

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Mar 20, 2017

taca
Update ruby-hashie to 3.5.5.
# Change Log

## [3.5.5] - 2017-02-24

[3.5.5]: intridea/hashie@v3.5.4...v3.5.5

### Added

* [#326](intridea/hashie#326): Added `Hashie::Extensions::Mash::KeepOriginalKeys` to give Mashes the ability to keep the original structure given to it - [@michaelherold](https://github.com/michaelherold).

### Fixed

* [#415](intridea/hashie#415): Fixed Mash logging keys multiple times which lead to a bad user experience or, in some cases, errors - [@michaelherold](https://github.com/michaelherold).

## [3.5.4] - 2017-02-22

[3.5.4]: intridea/hashie@v3.5.3...v3.5.4

### Added

* [#412](intridea/hashie#412): Added a Hashie::Extensions::Mash::SymbolizeKeys extension that overrides the default stringification behavior for keys - [@michaelherold](https://github.com/michaelherold).

### Fixed

* [#409](intridea/hashie#409): Fixed Railtie detection for projects where Rails is defined but Railties are not availble - [@CallumD](https://github.com/callumd).
* [#411](intridea/hashie#411): Fixed a performance regression from 3.4.3 that caused a 10x slowdown in OmniAuth - [@michaelherold](https://github.com/michaelherold).

## [3.5.3] - 2017-02-11

[3.5.3]: intridea/hashie@v3.5.2...v3.5.3

### Fixed

* [#402](intridea/hashie#402): Use a Railtie to set Hashie.logger on rails boot - [@matthewrudy](https://github.com/matthewrudy).
* [#406](intridea/hashie#406): Ensure that subclasses that disable warnings propagate that setting to grandchild classes - [@michaelherold](https://github.com/michaelherold).
* Your contribution here.

## [3.5.2] - 2017-02-10

[3.5.2]: intridea/hashie@v3.5.1...v3.5.2

### Added

* [#395](intridea/hashie#395): Add the ability to disable warnings in Mash subclasses - [@michaelherold](https://github.com/michaelherold).
* [#400](intridea/hashie#400): Fix Hashie.logger load and set the Hashie logger to the Rails logger in a Rails environment - [@michaelherold](https://github.com/michaelherold).

### Fixed

* [#396](intridea/hashie#396): Fix for specs in #381: Incorrect use of shared context meant example was not being run - [@biinari](https://github.com/biinari).
* [#399](intridea/hashie#399): Fix passing Pathname object to Hashie::Mesh.load() - [@albb0920](https://github.com/albb0920).

### Miscellanous

* [#397](intridea/hashie#397): Add the integration specs harness into the main test tasks - [@michaelherold](https://github.com/michaelherold).

## [3.5.1] - 2017-01-31

* [#392](intridea/hashie#392): Fix for #391: Require all dependencies of Hashie::Mash - [@dblock](https://github.com/dblock).

[3.5.1]: intridea/hashie@v3.5.0...v3.5.1

## [3.5.0] - 2017-01-31

* [#386](intridea/hashie#386): Fix for #385: Make `deep_merge` always `deep_dup` nested hashes before merging them in so that there are no shared references between the two hashes being merged. - [@mltsy](https://github.com/mltsy).
* [#389](intridea/hashie#389): Support Ruby 2.4.0 - [@camelmasa](https://github.com/camelmasa).

[3.5.0]: intridea/hashie@v3.4.6...v3.5.0

### Added

* [#381](intridea/hashie#381): Add a logging layer that lets us report potential issues to our users. As the first logged issue, report when a `Hashie::Mash` is attempting to overwrite a built-in method, since that is one of our number one questions - [@michaelherold](https://github.com/michaelherold).

### Changed

* [#384](intridea/hashie#384): Updated to CodeClimate 1.x - [@boffbowsh](https://github.com/boffbowsh).

### Fixed

* [#369](intridea/hashie#369): If a translation for a property exists when using IndifferentAccess and IgnoreUndeclared, use the translation to find the property - [@whitethunder](https://github.com/whitethunder).
* [#376](intridea/hashie#376): Leave string index unchanged if it can't be converted to integer for Array#dig - [@sazor](https://github.com/sazor).
* [#377](intridea/hashie#377): Dont use Rubygems to check ruby version - [@sazor](https://github.com/sazor).
* [#378](intridea/hashie#378): Deep find all searches inside all nested hashes - [@sazor](https://github.com/sazor).
* [#380](intridea/hashie#380): Evaluate procs default values of Dash in object initialization - [@sazor](https://github.com/sazor).

### Miscellanous

* [#387](intridea/hashie#387): Fix builds failing due to Rake 11 having a breaking change - [@michaelherold](https://github.com/michaelherold).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment