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
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@james2m
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 13, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@james2m

james2m Mar 13, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 13, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@james2m

james2m Mar 13, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 16, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@james2m

james2m Mar 16, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 17, 2017

Member

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

Member

dblock commented Mar 17, 2017

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

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 17, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 17, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@james2m

james2m Mar 17, 2017

Contributor

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

Contributor

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.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 17, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Mar 17, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@james2m

james2m Mar 17, 2017

Contributor

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.

Contributor

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 referenced this pull request Mar 21, 2017

Closed

Grape 1.0.0 #1598

@james2m

This comment has been minimized.

Show comment
Hide comment
@james2m

james2m Apr 6, 2017

Contributor

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

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Apr 6, 2017

Member

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

Member

dblock commented Apr 6, 2017

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

Show outdated Hide outdated README.md Outdated
```ruby
declared(params).user == declared(params)['user']
declared(params)[:user] == declared(params)['user']

This comment has been minimized.

@dblock

dblock Apr 6, 2017

Member

This should still work, no?

@dblock

dblock Apr 6, 2017

Member

This should still work, no?

This comment has been minimized.

@james2m

james2m Apr 6, 2017

Contributor

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

@james2m

james2m Apr 6, 2017

Contributor

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

Show outdated Hide outdated README.md Outdated
Show outdated Hide outdated README.md Outdated
params do
build_with Grape::Extensions::Hash::ParamBuilder
optional :color, type: String, default: 'blue', values: ['blue', 'red', 'green']
end

This comment has been minimized.

@dblock

dblock Apr 6, 2017

Member

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

@dblock

dblock Apr 6, 2017

Member

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

Show outdated Hide outdated README.md Outdated
end
```
The various options are documented in [Grape::DSL::Parameters](lib/grape/dsl/parameters.rb)

This comment has been minimized.

@dblock

dblock Apr 6, 2017

Member

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

@dblock

dblock Apr 6, 2017

Member

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

This comment has been minimized.

@james2m

james2m Apr 6, 2017

Contributor

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.

@james2m

james2m Apr 6, 2017

Contributor

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.

This comment has been minimized.

@dblock

dblock Apr 6, 2017

Member

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.

@dblock

dblock Apr 6, 2017

Member

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.

This comment has been minimized.

@james2m

james2m Apr 6, 2017

Contributor

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.

@james2m

james2m Apr 6, 2017

Contributor

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.

This comment has been minimized.

@james2m

james2m Apr 6, 2017

Contributor

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

@james2m

james2m Apr 6, 2017

Contributor

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

This comment has been minimized.

@dblock

dblock Apr 7, 2017

Member

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.

@dblock

dblock Apr 7, 2017

Member

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.

This comment has been minimized.

@james2m

james2m Apr 8, 2017

Contributor

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.

@james2m

james2m Apr 8, 2017

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Apr 6, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@james2m

james2m Apr 6, 2017

Contributor

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

Contributor

james2m commented Apr 6, 2017

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

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Apr 7, 2017

Member

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

Member

namusyaka commented Apr 7, 2017

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|

This comment has been minimized.

@namusyaka

namusyaka Apr 7, 2017

Member

[nits] Prefer Hash#each_key over keys.each

@namusyaka

namusyaka Apr 7, 2017

Member

[nits] Prefer Hash#each_key over keys.each

This comment has been minimized.

@james2m

james2m Apr 8, 2017

Contributor

Good spot.

@james2m

james2m Apr 8, 2017

Contributor

Good spot.

@dblock

This comment has been minimized.

Show comment
Hide comment
@dblock

dblock Apr 9, 2017

Member

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!

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!

@dblock dblock closed this Apr 9, 2017

@tsuwatch tsuwatch referenced this pull request Jul 5, 2017

Merged

Don't require hashie #44

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment