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

automatically permit validated params #99

Open
a-chris opened this issue Aug 18, 2023 · 3 comments · May be fixed by #100
Open

automatically permit validated params #99

a-chris opened this issue Aug 18, 2023 · 3 comments · May be fixed by #100

Comments

@a-chris
Copy link

a-chris commented Aug 18, 2023

Basic Info

  • rails_param Version: 1.3.1
  • Ruby Version: 3.2.1
  • Rails Version: 7.0.x

Issue description

The param! methods does not call params.permit over validated parameters, this means that after the validation block we need to manually permit and extract only the required fields.
e.g.

 param! :reaction, Hash, required: true do |r|
      r.param! :note, String, required: false, blank: true
      r.param! :like, :boolean, required: false
      r.param! :love, :boolean, required: false
      r.param! :reject, :boolean, required: false
end

# or with a separate method if you prefer
reaction_params = params.permit(reaction: [:like, :love, :reject, :note])

This is very repetitive and error-prone when deleting/adding new fields and could lead to dangerous errors.

The README is not very clear about this, from a first read I understood that I could use this gem and forget about the params.permit but now I figured out it's not like that.

I propose to automatically permit params (and nested params) based on the fields declared in param! by overriding the params variable or using a new instance variable such as @sanitized_params, @rails_params or whatever you prefer.

I'd like to open a PR if you like this idea, otherwise I will just keep the fork for me.

What do you think?

@iMacTia
Copy link
Collaborator

iMacTia commented Aug 21, 2023

This totally makes sense to me. @ifellinaholeonce what do you think?
The only request from me would be to make this configurable, we can then debate before the next release if we want to this to default to being enabled or disabled (with backwards-compatibility in mind).

I'll be unavailable over the next couple of weeks, but I don't want to be a blocker for this proposal 🙂

@ifellinaholeonce
Copy link
Collaborator

I think this is a great idea 👍 and I think the idea to make this configurable makes a lot of sense. I'm looking forward to it @a-chris. If you also think there is a part of the README that could be improved with this idea would you mind updating that as well?

Coincidentally, I am also offline for the rest of this week but happy to continue this conversation or review a PR next week.

@a-chris
Copy link
Author

a-chris commented Sep 6, 2023

hi guys, I've been busy too (it's summer!) so I managed to start hacking with the code last week but it's being very hard to keep track of the fields to permit.

Since the structure is declared inside blocks, we have no way to know which keys the user wants to validate before actually executing the block. The blocks also acts in a recursive fashion but we are able to collect only the result from the last field of the block, e.g.:

controller.param! :array, Array do |a|
          a.param! :object, Hash do |h|
            h.param! :num, Integer, required: true
            h.param! :float, Float, required: true
          end
        end

if we make the block returns something, we will be able to collect :array, :object and :float results but we can't collect what :num returned.

So, yeah, it's way harder than I expected, I have a semi-working solution that attempt to build a hierachy structure of the validated fields in param! and it was working so good with Hashes until I ran the Array tests 💥 🤣

However I do not give up, I'll keep you updated, in the meantime feel free to share ideas and suggestions 😄

@a-chris a-chris linked a pull request Sep 7, 2023 that will close this issue
3 tasks
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 a pull request may close this issue.

3 participants