add support for strong parameters with patch from dpmccabe #1732

Closed
wants to merge 9 commits into
from

Projects

None yet

4 participants

@garysweaver

incorporating this patch from dpmccabe as an optional DSL method for strong_parameters support:
https://gist.github.com/3718571

strong_parameters will be included in Rails 4, so this might help those that are starting to use it early.

@jasperkennis

I had created a similar issue: #1726. Please be aware that the change I made at line 6/7 of strong_admin.rb is required to Devins' fix work with longer class names (anything like ThisIsALongerModelName, because Devins' gist only converts the classname to lowercase, while the params get submitted under this_is_a_longer_model_name.

@garysweaver

@jasperkennis Thanks! Sorry I did a search and missed your ticket before submitting mine. Thanks much to you and Devin McCabe for the patch!

@jasperkennis

Not a problem, thanks a lot for the effort, I wouldn't have had the time to do it properly any time soon.

@macfanatic

I don't like this for a few reasons:

  1. It includes "Patch" in a module name
  2. class_eval is used throughout, would rather see something.class.send :include, ActiveAdmin::NewModuleName
  3. Don't want to overwrite InheritedResource actions of create/update without calling super implementations. Your changes could be implemented in a before_filter ran after the InheritedResources filters (I think) or at the very least write a method that uses the other implementations for rendering, etc.
@garysweaver

Matt,

We pretty much ditched this anyway, because I'm told that latest inherited_resources supports S.P. See:
#1731

Since AA uses inherited_resources, I assume the right S.P. patch would just be to update inherited_resources and use something similar to this in controller block as mentioned by @brendon:

def permitted_params
  params.permit(:blog => [:name, :description])
end

We actually ditched the patch related to this PR long ago and actually use this (for the older AA): http://stackoverflow.com/a/13091012/178651

So, this can probably be closed. Will go ahead and close it now, because I doubt it will be used.

Thanks for the input, though!
Gary

@seanlinsley
Active Admin member

Since in Rails 4 SP is default, we should probably add in a DSL method so this isn't necessary:

controller do
  def permitted_params
    params.permit(:blog => [:name, :description])
  end
end

Instead it could be something like this:

permit_params :blog => [:name, :description]
@garysweaver

you can do more than just permit though, so not sure if you want multiple class methods, and have to add new ones whenever S.P. gets more, e.g.:

params.require(:person).permit(:name, :age)

I agree that it should be more terse though.

@macfanatic

@garysweaver, thanks for the feedback!

@seanlinsley
Active Admin member

How about this syntax then? I'm thinking of allowing either to be used, and the block would be evaluated in the controller context, so you could conditionally change the params. Though I'm not sure if Inherited Resources allows that.

strong_params{ params.require(:person).permit(:name, :age) }
# or
strong_params permit: :foo, require: :bar
@garysweaver

@Daxter block is probably better because you won't be as limited. However, then you aren't far off from:

def permitted_params; params.require(:person).permit(:name, :age); end

It's ugly so a block passed into a class method would be nice, but you complicate things a little, i.e. should it define permitted_params method in the controller? What if you also defined a permitted_params method in the controller block? Should it override that or be called before or after? So, for now I think maybe just do it in the controller block; besides, that is what @dhh et al say in the S.P. docs: encapsulate the permits in a method and use that method name as example. I added similar support for a method of that name in restful_json recently, and also support a few additional methods to define params for create and update separately, for example, though I doubt people usually need that much complexity, typically.

Also- Adam Hawkins came up with a Permitters framework and I've talked with him about finally getting it into a gem. It's like ActiveModel::Serializers is to as_json- an OO version of S.P. However, Permitters also integrate with Cancan- we probably need to decouple that and make it optional. The advantage of having an OO representation of permitters is easier reuse, and you wouldn't need to be defining permits in an AA controller. That is way off-topic, but keep an eye out for it just in case. I use Permitters in restful_json and hoping to extract them and/or to continue to work with Adam on them.

@seanlinsley
Active Admin member

The whole point of my proposed syntax is to avoid having to open up the controller do/end block in AA. If the resource doesn't need to customize the AA controller, this helps keep resource registration files easy to read.

As far as which implementation to go with, for now I was just assuming I could call super. But the general idea is this would supplement, not replace, any other permitted params from other sources.

Can you link to the docs you mentioned?

On the Permitters side of things, where exactly would the whitelist go? SP moved it from the model to the controller; where next?

@garysweaver

In Permitters, the permits, etc. go into the Permitter app/permitters/..., e.g. app/permitters/post_permitter. Adam has a post on his blog about it some time back. Please email me if you have any feedback on them to take that offline. Am hoping to assist soon with that, because I'd like to offer plain S.P. in restful_json, too, and it'd be great to see them in their own gem. And note- I was as big a proponent as anyone not to have tons of files, which is why I was really interested in roar and roar-rails because those put both serialization and permitting together in one place (also because Nick has tried to implement Roy's vision of REST the way it was intended- that's cool), but I've grown to really like that ActiveModel::Serializers is helping with the mess that API (JSON, XML, etc.) views had become. Making both serialization and param authorization separate can be even more flexible. Ok, that was way off-topic, sorry...

I agree with what you are saying about being easier to read by getting it out of the block. I'm just thinking that it is already able to take a block and execute it in the controller to define the method, so having another one is going to add to the maintenance required in AA if it isn't that generic. But having a simple way to do it and then doing it via the controller block makes sense if everyone's on board with trying to keep it up with S.P. can do. Since Rails 4 doesn't have a final 4.0 release yet, and there were significant changes from 3.0->3.1 and I have a feeling it may be a similar thing w/4.1, I'm not sure I'd tack in something yet that might need maintenance. S.P. is fairly simple though, so maybe it would be fine. Not sure.

@seanlinsley
Active Admin member

Thanks, I'll take a look at that blog post.

I don't see this requiring maintenance as it would just be a DSL method that passes the block to the controller.

@garysweaver

BTW- by the doc on S.P. I just meant it's readme here. There is probably a railscast, etc. somewhere also, but what I was talking about was the comment in the first block of code above that method:

private
    # Using a private method to encapsulate the permissible parameters is just a good pattern
    # since you'll be able to reuse the same permit list between create and update. Also, you
    # can specialize this method with per-user checking of permissible attributes.
    def person_params
      params.require(:person).permit(:name, :age)
    end
@garysweaver

As for it not requiring maintenance- you might be right. I don't know. I put S.P. specific stuff in, but I had also spent a lot of time messing with mass assignment security in restful_json, etc. and learned my lesson. (Was added in Rails 3.x and now is gone in Rails 4.x.) So, just keep it in mind.

@garysweaver garysweaver deleted the garysweaver:fix-1731-add-support-for-strong-parameters branch Jan 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment