Add support for params.expect

[Linuus]

Fixes: #841

To do

• I have read the contributing guidelines.
• I have added relevant tests.
• I have adjusted relevant documentation.
• I have made sure the individual commits are meaningful.
• I have added relevant lines to the CHANGELOG.

This PR starts adding support for params.expect in Rails which is now recommended over params.require.permit.
See: https://api.rubyonrails.org/classes/ActionController/Parameters.html#method-i-expect

I'm a bit unsure what to do with pundit_params_for though. It's kind of redundant... but also kind of useful as mentioned in our Readme, if you want to fetch based on another "namespace" than the default. However, for expect it would essentially just turn into a wrapper method for PolicyFinder.new(record).param_key, so you can just override the param_key method on the Policy instead? 🤔

[Burgestrand]

This looks as one could expect (:D). I'm also wondering if this is a good time to adjust what kind of API we will permit (:D), since this is an addition we've got time to adjust the API without breaking any backwards-compatibility.

Things I'm thinking of:

• what if our policy API is expected_params_for(action), let the receiver worry about different params for different actions?
• param key, is it worth moving it to pundit.param_key(record) so it delegates to the proper policy finder

We're still rather light on Pundit-usage in our current projects. What about your project, do you use the action-specific expected_params?

[jensljungblad-kisi]

What about your project, do you use the action-specific expected_params?

We do, yeah.

[Linuus]

What about your project, do you use the action-specific expected_params?

We do, yeah.

Well we are not using the expect at all yet as we are waiting for it to be added in Pundit, right? :)

[jensljungblad-kisi]

Correct, I thought the question was if we're using the existing action specific permitted attributes methods.

[Burgestrand]

Ah yes. Sometimes I write a bit för fort!

I haven't forgotten this. Still thinking about if I want to adjust the future API now that there's a chance to do so.

[matthew-puku]

We use permitted_attributes 41 times and permitted_attributes_for_x 3 times.

• what if our policy API is expected_params_for(action), let the receiver worry about different params for different actions?

For us that means that in most cases the methods would look like:

def expected_params_for(_action)
  %i[foo bar]
end

The action-specific cases would take one of two forms:

# 1 Switch
def expected_params_for(action)
  case action.to_sym
  when :create
    %i[foo bar]
  when :update
    [:bar, (:foo if user.admin?)].compact
  else
    %i[bar]
  end
end

# 2 Concat
def expected_params_for(action)
  params = %i[foo]
  params += %i[bar baz] if action.to_sym == :create
  params
end

Things I'd note:

• It's not immediately clear to me whether action would be a symbol or a string.
• We try to avoid custom actions, so for our project the action argument is essentially a bool.
• The methods get gnarly if you have conditions on action and conditions on other stuff (as we usually do).

---

All that to say, I think I prefer copying the existing policy API ala expected_attributes_for_x, but it's not a huge deal either way.

[Burgestrand]

@matthew-puku Thanks, this is great feedback!

[jensljungblad-kisi]

Any plans to proceed with this? Can we do anything to help?

[Burgestrand]

Any plans to proceed with this? Can we do anything to help?

Yeah! I'm on a project now where I'll start using Pundit a bit more again, so that should allow me to try things out. Also christmas holidays 😄 I still have this irk that I want to finish up all issues, including the many-years old ones.

1. I do want to move the responsibility of public_send("expected_params_for_#{action}") into the receiver ¹, e.g. we'll instead be calling expected_params_for_action(action). I don't mind that it'll often be ignored, I could see this implementation be okay in a specific project, if that's the norm:

def expected_params_for_action(_action) = expected_params

I've considered the issue about data type and it'll semantically be whatever the framework you're using is passing, i.e. Rails' action_name.

2. I also want to move the param_key responsibility into Pundit::Context², mainly because it's a place where we can use dependency injection to introduce changes while avoiding breaking backwards-compatibility. If I ever do introduce the concept of "current namespace", then that will live inside the context too and might impact the policy finder, which we delegate to in order to find the param_key ³.

I think it's as simple as just adding the method:

def param_key(record) = policy_finder(record).param_key

Footnotes

1. I think there's a logical issue here in the previous implementation, too. I'm not even sure we should ever "fall back" to a generic permitted parameters; it's safer to always have specific permitted parameters per action, or always have the same permitted parameters for all actions. If you forget a definition or have a typo (e.g. permitted_params_for_updat) we should complain, rather than fall back to a default (permitted_params). Just throwing it out there as an FYI, because we can't change it due to backwards-compatibility anyway and my opinion is that we should delegate this to the receiver anyway. ↩

2. I have but one apprehension on this entire concept of the context, and it's that it might be more difficult to extend. I believe a strong part about Pundit is that it's small and simple to the point where monkey-patching and re-implementing Pundit methods is a good way of approaching certain things. Moving implementation into a class might make that more rigid, so I'm being rather cautious. ↩

3. I believe the PolicyFinder might be doing a bit too much, responsibility-wise. ↩

[Burgestrand]

FYI I ended up rebasing this branch on main, rather than merging it into this one. Feature-wise I think we should be good now, and hopefully all checks pass now that I've updated the GH Actions matrix.

I'll let this marinate while I go to lunch, but I intend to merge it today.

I would be interested to hear your experiences if you're willing to try out running pundit from the main branch, to test expected attributes out.

[Burgestrand]

Thank you! While this has been merged, I'm more than happy to keep options for improving this open. Once it's finally released the API should remain stable, but until then it's a free for all.