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 #100

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

a-chris
Copy link

@a-chris a-chris commented Sep 7, 2023

Description

fixes #99
As I explained in #99, it would be really useful to automatically permit the validated params. I'm already sharing this PR to collect feedbacks

The idea is to create a hierarchy structure, similar to params that is passed from the upper element to its child and so on. Each child update the hierarchy element it received and since it is passed by reference the hierarchy gets updated step by step, at the end we have the full structure.

Todos

List any remaining work that needs to be done, i.e:

  • Add configuration to enable/disable params permitting
  • Investigate edge cases
  • Documentation

Additional Notes

Let me know what's your idea to make this configurable, I was thinking of an initializer to enable it globally, something like

RailsParam.configure do |config|
    config.permit_params = true
end

@iMacTia
Copy link
Collaborator

iMacTia commented Sep 11, 2023

Thank for taking a stab at this @a-chris.
I had a quick look to provide some early feedback, I'd love to hear from @ifellinaholeonce as well.

Let me know what's your idea to make this configurable, I was thinking of an initializer to enable it globally

This looks great and it's how I've done it in other gems, but we don't currently have configuration support in this gem and it would be out of scope from this PR. So I'd prefer to see that introduced in a separate PR first, and then used here.

The idea is to create a hierarchy structure, similar to params that is passed from the upper element to its child and so on

This is an interesting idea and I can see the implementation was not easy 👏 !
Overall it seems like you're trying to build this structure on the way down, bubble it up to the original caller and then call permit on it.

A possibly easier alternative would be to only pass this information downwards (like breadcrumbs of where you're at in params) and, upon reaching a "leaf", calling params.permit.
This would increase the number of times we call params.permit, but it would not require changing the return type of the methods as the call would happen on the leaves as opposed to the root.

We already do something similar with context for printing error messages; although I'm not sure you could use it as-is given it's a string, I do wonder if it could be combined with the new hierarchy/breadcrumbs information into a single field?

@ifellinaholeonce
Copy link
Collaborator

I like the look of this!

I haven't thought deeply about this yet but could a potential solution to the configurability be a keyword such as permit: true similar to required: true? It might perhaps be a simpler way of adding a config for this behaviour. Excuse me if I am missing some obvious complexity here.

A possibly easier alternative would be to only pass this information downwards (like breadcrumbs of where you're at in params) and, upon reaching a "leaf", calling params.permit.

This seems intriguing to me. And, as you suggested, seems like it would pair well with a small refactor of context.

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 this pull request may close these issues.

automatically permit validated params
4 participants