IndifferentHash improvements by mwpastore · Pull Request #1427 · sinatra/sinatra

mwpastore · 2018-05-02T22:41:24Z

No description provided.

mwpastore · 2018-05-03T00:02:00Z

Hrrm, it doesn't like ActiveSupport's hash extensions, which get pulled in by RABL during testing. I'll look into it.

namusyaka · 2018-05-05T07:06:49Z

@mwpastore Broken test was fixed in #1425. Please rebase these commits.

mwpastore · 2018-05-05T11:13:07Z

@namusyaka Hmm, not quite.

namusyaka · 2018-06-19T14:49:44Z

This is due to the method defined by activesupport, see: https://github.com/rails/rails/blob/ee21e058424b2cb55bf74981c28b1ac0fb98b576/activesupport/lib/active_support/core_ext/hash/keys.rb
By requiring this, transform_keys returns Hash, not Sinatra::IndifferentHash.
The feature is required in rabl test.

We can avoid the failure by patching temporary workaround:

diff --git a/lib/sinatra/indifferent_hash.rb b/lib/sinatra/indifferent_hash.rb
index bf94f4e1..38427f05 100644
--- a/lib/sinatra/indifferent_hash.rb
+++ b/lib/sinatra/indifferent_hash.rb
@@ -164,7 +164,7 @@ module Sinatra
 
       def transform_keys!
         super
-        super(&method(:convert_key))
+        IndifferentHash[super(&method(:convert_key))]
       end
     end
 
diff --git a/test/indifferent_hash_test.rb b/test/indifferent_hash_test.rb
index a22d33ee..0a7ed94b 100644
--- a/test/indifferent_hash_test.rb
+++ b/test/indifferent_hash_test.rb
@@ -4,6 +4,7 @@
 #
 require 'minitest/autorun' unless defined?(Minitest)
 
+require 'active_support/core_ext/hash/conversions'
 require_relative '../lib/sinatra/indifferent_hash'
 
 class TestIndifferentHashBasics < Minitest::Test

However, I don't like the temporary patch. If you have any better solution, please write up or send pull request.

I'm going to re-visit this issue next week.
Thanks,

mwpastore · 2018-06-22T21:05:05Z

Disregard my last comment. Whether we remove ActiveSupport or not, we still need to make IndifferentHash work when active_support/core_ext/hash is loaded. Let me make another run at it.

mwpastore · 2018-06-22T21:37:31Z

@namusyaka

By requiring this, transform_keys returns Hash, not Sinatra::IndifferentHash.

I don't think that's correct. Sinatra::IndifferentHash#transform_keys(!) never calls Hash#transform_keys; it only ever calls Hash#transform_keys!. By design.

irb(main):003:0> require 'active_support/core_ext/hash/conversions'
=> true
irb(main):004:0> foo = Sinatra::IndifferentHash.new
=> {}
irb(main):005:0> foo['bar'] = 'qux'
=> "qux"
irb(main):009:0> foo.transform_keys!(&:upcase).class
=> Sinatra::IndifferentHash
irb(main):008:0> foo.transform_keys(&:upcase).class
=> Sinatra::IndifferentHash

This is without your patch above.

mwpastore · 2018-06-22T23:19:48Z

The problem is that Sinatra::IndifferentHash is being loaded before active_support/core_ext/hash when the full test suite is run under Ruby <2.5. It doesn't think Hash#transform_keys! is defined, so it doesn't attempt to redefine its behavior. At runtime, however, the method is there (on Sinatra::IndifferentHash's ancestor(s)), so it uses that without the indifferent access tweaks.

Fixing it "correctly" for both the test suite and production use would require some foreknowledge of whether or not active_support/core_ext/hash either has been or will be loaded in the current Ruby process. This issue extends to other Hash methods that active_support/core_ext/hash backports to older Rubies (so it's not restricted to just e.g. active_support/core_ext/hash/keys or active_support/core_ext/hash/conversions).

Here's a list of solutions and workarounds and why they're bad:

Remove the method_defined? checks from Sinatra::IndifferentHash so these methods get defined whether or not they're already defined. This will simply break in the case when Sinatra::IndifferentHash is loaded before active_support/core_ext/hash, because it does the same check there.
REMOVED
If ActiveSupport is present in LOAD_PATH, we can preemptively load active_support/core_ext/hash. We shouldn't force all Sinatra users to use a monkey-patched Hash class in their apps.
If ActiveSupport remains a Sinatra development dependency (i.e. if we don't remove RABL), we can preemptively load active_support/core_ext/hash in the test suite. This is probably the closest thing to a workable solution, but it only solves the test suite case, not the production use case.

Hopefully there's an option I'm overlooking. If not, I think the best approach is a hybrid of 4 and 3 above: we preemptively load active_support/core_ext/hash in the test suite, and print a warning on boot if ActiveSupport is present in LOAD_PATH (but not yet loaded?). The warning should say something to the effect of:

If you plan to load any of ActiveSupport's hash extensions, be sure to do so _before_ loading Sinatra::Application or Sinatra::Base. Otherwise, you may disregard this warning.

Let me know your thoughts.

namusyaka · 2018-06-23T11:41:26Z

The 3 + 4 hybrid plan seems to be good.
I cannot fully predict what changes will be made by activesupport in the future.

Specifically, between ActiveSupport's core extensions to Hash and our own IndifferentHash class.

namusyaka

Looks good to me.

## 2.0.4 / 2018-09-15 * Don't blow up when passing frozen string to `send_file` disposition [#1137](sinatra/sinatra#1137) by Andrew Selder * Fix ubygems LoadError [#1436](sinatra/sinatra#1436) by Pavel Rosick�«ò * Unescape regex captures [#1446](sinatra/sinatra#1446) by Jordan Owens * Slight performance improvements for IndifferentHash [#1427](sinatra/sinatra#1427) by Mike Pastore * Improve development support and documentation and source code by Will Yang, Jake Craige, Grey Baker and Guilherme Goettems Schneider ## 2.0.3 / 2018-06-09 * Fix the backports gem regression [#1442](sinatra/sinatra#1442) by Marc-Andr�«± Lafortune ## 2.0.2 / 2018-06-05 * Escape invalid query parameters [#1432](sinatra/sinatra#1432) by Kunpei Sakai * The patch fixes [CVE-2018-11627](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2018-11627). * Fix undefined method error for `Sinatra::RequiredParams` with hash key [#1431](sinatra/sinatra#1431) by Arpit Chauhan * Add xml content-types to valid html_types for Rack::Protection [#1413](sinatra/sinatra#1413) by Reenan Arbitrario * Encode route parameters using :default_encoding setting [#1412](sinatra/sinatra#1412) by Brian m. Carlson * Fix unpredictable behaviour from Sinatra::ConfigFile [#1244](sinatra/sinatra#1244) by John Hope * Add Sinatra::IndifferentHash#slice [#1405](sinatra/sinatra#1405) by Shota Iguchi * Remove status code 205 from drop body response [#1398](sinatra/sinatra#1398) by Shota Iguchi * Ignore empty captures from params [#1390](sinatra/sinatra#1390) by Shota Iguchi * Improve development support and documentation and source code by Zp Yuan, Andreas Finger, Olle Jonsson, Shota Iguchi, Nikita Bulai and Joshua O'Brien

mwpastore force-pushed the indiff-hash branch from 0933cfc to 23ce754 Compare May 5, 2018 10:41

namusyaka added this to the v2.0.4 milestone Jun 19, 2018

namusyaka self-assigned this Jun 19, 2018

namusyaka added the help wanted label Jun 19, 2018

mwpastore added 3 commits June 22, 2018 16:12

Slight performance improvements for IndifferentHash

92eeec0

Add support for Ruby 2.4's #transform_values(!) to IndifferentHash

453ecd9

Add support for Ruby 2.5's #transform_keys(!) to IndifferentHash

7d8e5be

mwpastore force-pushed the indiff-hash branch from 23ce754 to 7d8e5be Compare June 22, 2018 21:51

Address issues caused by loading sequence

c58452f

Specifically, between ActiveSupport's core extensions to Hash and our own IndifferentHash class.

namusyaka approved these changes Aug 4, 2018

View reviewed changes

namusyaka merged commit 1731bf5 into sinatra:master Aug 4, 2018

mwpastore mentioned this pull request Sep 22, 2018

Please remove the ActiveSupport warning #1476

Closed

evanboho mentioned this pull request Aug 23, 2021

IndifferentHash#transform_keys not transforming keys #1717

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IndifferentHash improvements#1427

IndifferentHash improvements#1427
namusyaka merged 4 commits into
sinatra:masterfrom
mwpastore:indiff-hash

mwpastore commented May 2, 2018

Uh oh!

mwpastore commented May 3, 2018

Uh oh!

namusyaka commented May 5, 2018

Uh oh!

mwpastore commented May 5, 2018

Uh oh!

namusyaka commented Jun 19, 2018

Uh oh!

mwpastore commented Jun 22, 2018

Uh oh!

mwpastore commented Jun 22, 2018

Uh oh!

mwpastore commented Jun 22, 2018 •

edited

Loading

Uh oh!

namusyaka commented Jun 23, 2018

Uh oh!

namusyaka left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mwpastore commented May 2, 2018

Uh oh!

mwpastore commented May 3, 2018

Uh oh!

namusyaka commented May 5, 2018

Uh oh!

mwpastore commented May 5, 2018

Uh oh!

namusyaka commented Jun 19, 2018

Uh oh!

mwpastore commented Jun 22, 2018

Uh oh!

mwpastore commented Jun 22, 2018

Uh oh!

mwpastore commented Jun 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

namusyaka commented Jun 23, 2018

Uh oh!

namusyaka left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mwpastore commented Jun 22, 2018 •

edited

Loading