Replace hashie mash with hash by james2m · Pull Request #1594 · ruby-grape/grape

james2m · 2017-03-13T07:26:17Z

Building on comments in #1514 #1279. Defaults to HashWithIndifferentAccess as @request.params needs to be indifferent for the validation code which mutates it.

It largely achieves the API suggested in #1514 but at the expense of Law of Demeter, I'm open to suggestions. Also happy to make Hash the default params object, but didn't do so as that requires an additional transformation of the params.

dblock · 2017-03-13T11:56:40Z

First, thanks. I closed #1594 in favor of this PR.

Do you think we can reduce the internals of Grape to use a plain Hash and make that the default instead of the activesupport one?

I suspect there's a non-trivial performance penalty in making a Hashie::Mash on top of some other hash in transform_params. I think you have to make an interface where we construct the params object of the right type.

Does Grape::API.extend Grape::Extensions::Hashie::Mash work globally?

I think this is close if we can address the above. Cleanup please, fix the build, document this in README and UPGRADING.

james2m · 2017-03-13T13:18:38Z

@dblock I think we can initiate with either a Mash or AR::HashWithIndifferentAccess in Grape::Request by wrapping it with a shim to tell it what class to use to create the params with;

      module Mash
        def new_request(env)
           Grape::Request.new(env, params_class: Hashie::Mash)
        end
      end

That would mean the interface doesn't change and would work globally.

I started out with the plain Hash which breaks some endpoint specs that test for indifferent access, and therefor presents a breaking change to the Endpoint DSL I also had to enforce a consistent style of key from request, through validation, coercion and the DSL (hence the deep symbolizing module). For those reasons I reverted back to HashWithIndifferentAccess as the default as it's not a breaking change to existing apps and still allows the dropping of Hashie.

I've got a limited window for this, I can give the Hash from Request thru Response a shot, but would still suggest it's better as an option rather than default.

dblock · 2017-03-13T13:38:28Z

I think specs that rely on mash behavior should be rewritten in terms of Hash and possibly duplicated into a Mash version.

I feel pretty strongly not forcing people into another Hash-like class and letting them choose. Either way it should be configurable, so I would write the code in terms of Hash, then potentially switch the default to the activesupport or mash version.

james2m · 2017-03-13T14:30:24Z

Ok, those specs I can move over to Hash behaviour, I can make Hash the default, and update the README. The one cost is going to be in building the Hash from params as they come in with string keys and I'm pretty sure the entire stack uses symbols so it's going to require the keys symboliziing. I don't see any way round that. The deep symbolizer I've included is pretty fast, but there is that cost.

dblock · 2017-03-16T11:45:29Z

Thanks for keeping at it! You should squash these commits. I can look once the build is green.

Signed-off-by: James McCarthy <james@thisishatch.co.uk>

Update Endpoint to use build_with.

james2m · 2017-03-16T14:43:44Z

@dblock The default is now Hash. Hashie and HashWithIndifferentAccess are both options. I ended up adding a build_with option to Parameters.rb as it was much cleaner than extending Grape::API. The commits are all squashed and it should make a lot more sense.

I don't know what's up with Danger failing the build but the tests are all passing. I've gone as far as I can with this and am out of time.

dblock · 2017-03-17T00:32:58Z

Danger is complaining about the missing period at the end of your changelog line.

dblock · 2017-03-17T00:35:02Z

We still need to write clear UPGRADING for this and possibly README changes. Thanks for your help @james2m. It's a sensitive change that is going to affect a lot of people, so I am going to wait till myself or someone else is committed to seeing it through and at least somewhat actively supporting users who're getting problems with it during the next release.

dblock · 2017-03-17T00:35:45Z

For the sake of reducing this PR we probably want to rubocop -a gemfiles as well after appraisals were generated.

james2m · 2017-03-17T01:14:06Z

@dblock I can do the appraisals, I couldn't get Danger to run locally or verbosely so had no feedback from it so thanks for that.

I avoided writing the UPGRADING and README in case there were further implementation changes as I figured it's going to need someone to kick the tyres. My 'out of time' doesn't mean now way never, just got to get on with the project this is going into.

james2m · 2017-03-17T03:57:15Z

            lambda do |val|
              method.call(val).tap do |new_value|
                new_value.each do |item|
-                  Hashie.symbolize_keys!(item) if item.is_a? Hash


@dblock This slipped through the net. From the description above this method is supposed to wrap the result of the method and transform any Hash to be symbolized, that's fine, I can update. There is no direct test of this method, but reading the code it looks to me like the block above should not be iterating with #each but #map to construct a new Array or Set;

method.call(val).tap do |new_value| new_value.map do |item| item.is_a?(Hash) ? symbolize_keys(item) : item end end

Correct?

I can easily update this to symbolize the Hash without using the ActiveSupport method, but wanted to check the existing method is actually doing what's expected.

It's updating the values in place it looks like. If removing it doesn't cause any specs to fail I would start by writing a test that hits it, then change the behavior.

dblock · 2017-03-17T11:49:05Z

I personally vote for this implementation, but I may not be grasping the whole scope. We have to finish the code and the README/UPGRADING, tell the grape mailing list to try this out and ask for comments.

dblock · 2017-03-17T11:52:29Z

If it makes sense, now that everything is Hash-based, and to avoid massive downstream regressions we should probably default the user-facing behavior to the active support version (or any a hash with invariant and method access).

I'm then looking for a 1-liner in README that makes this a plain Hash or a Hashie::Mash (after adding hashie to Gemfile).

james2m · 2017-03-17T15:11:28Z

I'd definitely be inclined to go the ActiveSupport route for it's first outing as that's going to be the least pain for existing users. It'll take me a few days to get the extra tests in and docs updated. I'll chip away at it next week.

james2m · 2017-04-06T11:32:23Z

@dblock Before I make the final changes. We are defaulting to HashWithIndifferentAccess? If so I'll update the README and UPGRADING to reflect. Then the PR is done.

dblock · 2017-04-06T12:14:11Z

Yes, I think that's right. Also cc: @namusyaka

dblock · 2017-04-06T12:15:05Z

+  # Parameter will be a plain Ruby `Hash`:
+  params[:avatar][:filename] # => 'avatar.png'
+  params[:avatar]['avatar']     # => 'image/png'
+  params[:avatar][:tempfile] # => #<File>


The old code works here with HashWithIndifferentAccess default.

dblock · 2017-04-06T13:11:18Z


 ```ruby
-  declared(params).user == declared(params)['user']
+  declared(params)[:user] == declared(params)['user']


This should still work, no?

It's a HashWithIndifferentAccess so I wouldn't expect to be able to access :user as a method only with a key.

dblock · 2017-04-06T13:11:29Z

 ```

+By default Grape 1.x presents the parameters to the endpoint as an
+ActiveSupport::HashWithIndifferentAccess. This is a change from 0.x where a


Quote class name.

dblock · 2017-04-06T13:12:00Z


+By default Grape 1.x presents the parameters to the endpoint as an
+ActiveSupport::HashWithIndifferentAccess. This is a change from 0.x where a
+Hashie::Mash was previously presented.


This and the restore note belongs in UPGRADING, not README. Lets keep it consistent.

It's in the UPGRADING I just figured it's a good point to direct them to it. Can remove the paragraph if you prefer?

dblock · 2017-04-06T13:12:37Z

+params do
+  build_with Grape::Extensions::Hash::ParamBuilder
+  optional :color, type: String, default: 'blue', values: ['blue', 'red', 'green']
+end


Document how to configure this globally with Hash or Hashie::Mash.

dblock · 2017-04-06T13:13:26Z

 end
+
+`params` hash keys  can accesed with `""` or `:`
+`:avatar` and `"avatar"` are considered to be the same.


The old syntax should work by default, so put that back. And is this true when using Hash? What is it storing?

Ruby programmers know the difference so maybe say "as a string" or "as a symbol".

dblock · 2017-04-06T13:13:55Z

+end
+```
+
+The various options are documented in [Grape::DSL::Parameters](lib/grape/dsl/parameters.rb)


Doesn't explain how to do this globally or for an entire API. I have a 500 endpoints, I don't think I can do this everywhere :)

Can you look at the implementation in dsl/parameters. This is actually my first outing with Grape so I wasn't 100% sure about the inheritable parameters so I'd appreciate some eyes on that.

Looks OK to me, just need a way to do this globally. I'm not in love with build_with as a name, but I can't come up with better. Alternatives ideas may be builde or just with?

Either way we need something where I can do this for an entire API.

So currently I've just got build_with defined in lib/grape/dsl/parameters.rb which is fine for the syntax in the local params block. Can you suggest how & where you would like this in Global? I'm on very limited time for this so a big sign post would help.

@dblock @namusyaka There are a lot of getter/setters in DSL::Settings, right now I do not have the time to read through all the code and decipher which one to use and how they interact with each other so I'm going to need direction.

I too am not delighted with build_with but couldn't come up with anything better and it kind of made sense in the context of the params block. I don't think it much sense in a global context so very receptive to suggestions of what to call it and where to put it.

I think I'd want a mixin, so something like

class API < Grape::API include Grape::SomethingSomething::Params::Hashie::Mash end

One way it could work is by overriding params, but hopefully there's a cleaner way.

I couldn't find a clean way via composition as you're mixing into API, but Request creates the params when Endpoint is mounted in API so going that route I ended up passing the class to instantiate through Endpoint and onto Request. Which pretty convoluted given we have a mechanism for sharing parameters across API and Endpoint via the Settings mixin.

dblock · 2017-04-06T13:15:48Z

You should squash these commits so we can look at a nice single change. I can do it when merging, but lets make this PR discoverable and easy to read for anyone who wants to look at what changed for something so important.

james2m · 2017-04-06T13:32:47Z

I'll squash again when we're done with review comments.

namusyaka · 2017-04-07T15:55:10Z

I was late to the party. I agree that HashWithIndifferentAccess is default.
@james2m Thank you so much for your great work!

namusyaka · 2017-04-07T16:00:49Z

        end
+
+        def symbolize_keys!(hash)
+          hash.keys.each do |key|


[nits] Prefer Hash#each_key over keys.each

dblock · 2017-04-09T21:10:04Z

Replacing this with #1610, which includes a way to use ParamBuilder globally for an entire API or a namespace. And a bunch of README, UPGRADING, spec and require cleanup.

Thanks @james2m, lets finish this!

dblock mentioned this pull request Mar 13, 2017

Replace Hashie::Mash with ActiveSupport::HashWithIndifferentAccess #1514

Closed

james2m referenced this pull request in james2m/grape Mar 13, 2017

Upgraded RuboCop to 0.35.1.

6aa1204

Ignore .ruby-version, .ruby-gemset and byebug_history.

a908eb7

james2m force-pushed the replace_hashie-mash_with_hash branch from 862281f to 71a3146 Compare March 16, 2017 14:19

msroot and others added 15 commits March 16, 2017 21:26

replace Hashie::Mash with ActiveSupport::HashWithIndifferentAccess

1afdc91

Move hashie from gemspec to Gemfile Development dependency.

162f28c

Replace references to ActiveSupport::HashWithIndifferentAccess.

57f4545

Deep symbolize module for converting a Hash.

1787cb7

Create DeepMergeableHash.

71052c8

Add parameter builder extensions.

790443b

Signed-off-by: James McCarthy <james@thisishatch.co.uk>

Update coerce_spec for Hashie::Mash.

d9e8ed0

Update endpoint_spec to test setting param builder.

bbb88e3

Update request_spec to expect a symbolized Hash.

eabe213

Update Endpoint to use build_with.

Add build_with to the parameters dsl to select the builder.

2a21427

Use build_with in Endpoint to pass to Grape::Request.

e54e2d0

Plug the parameter builder extensions into Grape::Request.

2201d15

Ensure deep symbolization of coerced params in Hash's.

85e02c7

Switch Grape::Request to use Hash as default.

d7c1daa

Add build_parameters_with to Grape::Middleware::Globals

2491071

james2m force-pushed the replace_hashie-mash_with_hash branch from 71a3146 to e713257 Compare March 16, 2017 14:27

James McCarthy added 2 commits March 17, 2017 10:22

Change README.md to reference Hash not HashWithIndifferentAccess

dc07f38

Update CHANGELOG.md with PR 1594.

6c2d614

james2m force-pushed the replace_hashie-mash_with_hash branch from 7e255f3 to 6c2d614 Compare March 17, 2017 03:23

james2m commented Mar 17, 2017

View reviewed changes

dblock mentioned this pull request Mar 21, 2017

Grape 1.0.0 #1598

Closed

Ensure CustomTypeCoercer works with plain Hash.

abae16c

dblock reviewed Apr 6, 2017

View reviewed changes

James McCarthy added 2 commits April 6, 2017 20:06

Switch the default params to HashWithIndifferentAccess.

247869b

Document the changes to params in README and UPGRADING.

6e5964b

dblock reviewed Apr 6, 2017

View reviewed changes

Some adjustments to README.md.

2ddaa42

namusyaka reviewed Apr 7, 2017

View reviewed changes

dblock mentioned this pull request Apr 9, 2017

Replace Hashie::Mash with ActiveSupport::HashWithIndifferentAccess. #1610

Merged

dblock closed this Apr 9, 2017

tsuwatch mentioned this pull request Jul 5, 2017

Don't require hashie ruby-grape/grape-rabl#44

Merged

dblock mentioned this pull request Nov 1, 2018

Don't rely on Hashie::Mash (compatibility with hashie 3.3.0) #737

Closed

Uh oh!

Uh oh!

Conversation

james2m commented Mar 13, 2017

Uh oh!

dblock commented Mar 13, 2017

Uh oh!

james2m commented Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dblock commented Mar 13, 2017

Uh oh!

james2m commented Mar 13, 2017

Uh oh!

dblock commented Mar 16, 2017

Uh oh!

james2m commented Mar 16, 2017

Uh oh!

dblock commented Mar 17, 2017

Uh oh!

dblock commented Mar 17, 2017

Uh oh!

dblock commented Mar 17, 2017

Uh oh!

james2m commented Mar 17, 2017

Uh oh!

james2m Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblock commented Mar 17, 2017

Uh oh!

dblock commented Mar 17, 2017

Uh oh!

james2m commented Mar 17, 2017

Uh oh!

james2m commented Apr 6, 2017

Uh oh!

dblock commented Apr 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblock Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblock Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblock commented Apr 6, 2017

Uh oh!

james2m commented Apr 6, 2017

Uh oh!

james2m commented Mar 13, 2017 •

edited

Loading

james2m Mar 17, 2017 •

edited

Loading

dblock Apr 6, 2017 •

edited

Loading

dblock Apr 7, 2017 •

edited

Loading