8000 Add support for params.expect by Linuus · Pull Request #855 · varvet/pundit · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add support for params.expect #855

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Add support for params.expect #855

wants to merge 3 commits into from

Conversation

Linuus
Copy link
Collaborator
@Linuus Linuus commented Mar 27, 2025 •
edited
Loading

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? 🤔

Linuus
Linuus commented Mar 27, 2025
View reviewed changes
spec/authorization_spec.rb
@@ -273,6 +273,91 @@ def to_params(*args, **kwargs, &block)
end
end

if Gem::Version.new(ActionPack::VERSION::STRING) >= Gem::Version.new("8.0.0")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looks like JRUBY doesn't support this version? I don't know if this is a good approach though :)

@Linuus Linuus self-assigned this Mar 28, 2025
@Linuus Linuus requested a review from Burgestrand March 28, 2025 13:47
@Burgestrand
Copy link
Member

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

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

We do, yeah.

@Linuus
Copy link
Collaborator Author
Linuus commented May 1, 2025

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

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

@Burgestrand
Copy link
Member

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
Copy link
matthew-puku commented Jun 5, 2025 •
edited
Loading

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

@matthew-puku Thanks, this is great feedback!

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

@Burgestrand Burgestrand Awaiting requested review from Burgestrand

Assignees

@Linuus Linuus

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support new params expect method to permit parameters
4 participants
@Linuus @Burgestrand @jensljungblad-kisi @matthew-puku
0