Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace hashie mash with hash #1594

Closed

Conversation

james2m
Copy link
Contributor

@james2m james2m commented Mar 13, 2017

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
Copy link
Member

dblock commented Mar 13, 2017

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
Copy link
Contributor Author

james2m commented Mar 13, 2017

@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
Copy link
Member

dblock commented Mar 13, 2017

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
Copy link
Contributor Author

james2m commented Mar 13, 2017

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.

james2m referenced this pull request in james2m/grape Mar 13, 2017
@dblock
Copy link
Member

dblock commented Mar 16, 2017

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

@james2m james2m force-pushed the replace_hashie-mash_with_hash branch from 862281f to 71a3146 Compare March 16, 2017 14:19
@james2m james2m force-pushed the replace_hashie-mash_with_hash branch from 71a3146 to e713257 Compare March 16, 2017 14:27
@james2m
Copy link
Contributor Author

james2m commented Mar 16, 2017

@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
Copy link
Member

dblock commented Mar 17, 2017

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

@dblock
Copy link
Member

dblock commented Mar 17, 2017

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
Copy link
Member

dblock commented Mar 17, 2017

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

@james2m
Copy link
Contributor Author

james2m commented Mar 17, 2017

@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 james2m force-pushed the replace_hashie-mash_with_hash branch from 7e255f3 to 6c2d614 Compare March 17, 2017 03:23
@@ -162,15 +162,15 @@ def enforce_symbolized_keys(type, method)
lambda do |val|
method.call(val).tap do |new_value|
new_value.each do |item|
Hashie.symbolize_keys!(item) if item.is_a? Hash
Copy link
Contributor Author

@james2m james2m Mar 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

dblock commented Mar 17, 2017

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
Copy link
Member

dblock commented Mar 17, 2017

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
Copy link
Contributor Author

james2m commented Mar 17, 2017

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.

@dblock dblock mentioned this pull request Mar 21, 2017
@james2m
Copy link
Contributor Author

james2m commented Apr 6, 2017

@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
Copy link
Member

dblock commented Apr 6, 2017

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

README.md Outdated
# Parameter will be a plain Ruby `Hash`:
params[:avatar][:filename] # => 'avatar.png'
params[:avatar]['avatar'] # => 'image/png'
params[:avatar][:tempfile] # => #<File>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old code works here with HashWithIndifferentAccess default.


```ruby
declared(params).user == declared(params)['user']
declared(params)[:user] == declared(params)['user']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still work, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.md Outdated
@@ -797,6 +797,22 @@ params do
end
```

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quote class name.

README.md Outdated
@@ -797,6 +797,22 @@ params do
end
```

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

README.md Outdated
end

`params` hash keys can accesed with `""` or `:`
`:avatar` and `"avatar"` are considered to be the same.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

end
```

The various options are documented in [Grape::DSL::Parameters](lib/grape/dsl/parameters.rb)
Copy link
Member

@dblock dblock Apr 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@dblock dblock Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

dblock commented Apr 6, 2017

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
Copy link
Contributor Author

james2m commented Apr 6, 2017

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

@namusyaka
Copy link
Contributor

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

@@ -179,6 +180,20 @@ def enforce_symbolized_keys(type, method)
method
end
end

def symbolize_keys!(hash)
hash.keys.each do |key|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nits] Prefer Hash#each_key over keys.each

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot.

@dblock
Copy link
Member

dblock commented Apr 9, 2017

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants