fix #23640 allow strong parameters to accept nested arrays

[leonardoprg]

This pull request is related to issue

#23640

You guys can test it using this script:

https://gist.github.com/leonardoprg/ae6b3a48b880d5630c52

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[leonardoprg]

r? @arthurnn

[rafaelfranca]

r? @fxn @dhh what do you think about this case?

[leonardoprg]

r?

[leonardoprg]

r? @rafaelfranca

[CarlosSoares]

+1

[pawandubey]

This issue has been open for almost a year now. Still facing this problem and having to write my own code to work around it. @rafaelfranca @fxn

[arthurnn]

This is indeed a common use case of strong parameters.
I agree the API of strong parameters is somehow limited.
@rafaelfranca you think we should keep iterating and adding new features like this on top of it? or put it a blocker on it till we come up with something different/better?

I am ok into us iterating in the current strong parameters we have, to add things like this. and optional types to parameters, for easy casting in the controller.

[dhh]

I think this is just fine. I have more objections on other parts of the type casting spectrum. But let's evaluate on a case by case basis!

[arthurnn]

We will need a nice changelog entry for this change.

Thanks for taking the time .

[arthurnn]

why do we need the same is_a? Array condition here, and in the recursive function? Could we just call it here, and have that condition there?

[leonardoprg]

I'm not sure if I understand your point,
but if we always calls permit_nested_array even object is not a nested array, it will need to permit again the parameter, for example:

if we change the code

allowed_elements = object.flatten.grep(Parameters).map { |el| yield el }.compact
 if object.first.is_a?(Array)
  permit_nested_array(allowed_elements.first, object)
 else
    allowed_elements
  end

for something like

allowed_elements = object.flatten.grep(Parameters).map { |el| yield el }.compact
allowed_elements.contact(allowed_elements, object)

when it's not a nested array, the method will try to permit again the elements,

please let me know your opinion, or if it was a missunderstood it,

Thanks

[rutaka-n]

It will rise error if nested_object is scalar type.
There are nested arrays with scalar types in GeoJSON

[leonardoprg]

@rutaka-n good catch, I fixed the problem and added a test for it.

[leonardoprg]

@arthurnn I addressed the @rutaka-n comment, and I answered your comment,
also edited changelog,

please let me know any other change we need before merge.

Thanks

[meaton-potatoes]

[leonardoprg]

r? @arthurnn

[yangningBU]

Can we get some eyes on this? It's a painful bug to work around. The fix with tests and changelog are already submitted. We just need someone from the rails team to deal with the changelog merge conflict.

[dhh]

@rafaelfranca Any objections?

[matthewd]

Does this change allow previously-rejected values to pass an existing permit pattern, or does it only add behaviour for a previously-invalid pattern form?

[leonardoprg]

@matthewd It just fixes a problem with the nested array pattern, like: [[{ a: :b }], [{ b: :c }]]
it does not add a new pattern.

[leonardoprg]

@rafaelfranca

[matthewd]

@leonardoprg sorry, I missed your previous reply.

I'm not sure how to best clarify my question, but it was an either-or: exactly one of those things is true, because they're opposites.

Does this change allow previously-rejected values to pass an existing permit pattern?

Is there an argument to .permit(A), that would today accept some value X, and would today reject some value Y, and after this change would permit Y?

The opposite would be that every argument A, that after this change permits value Y, previously did not permit any value X.

[rafaelfranca]

If this array had two sections I'd expect both sections to be permitted but only the first one is.

Also, I think this syntax is ambiguous.

Given params.permit(sections: [[[:title]]]), what would be result for the following hashes ?

         sections: [
          [
            {
              title: "romance",
              description: "a romance section"
            }
          ],
          [
            {
              title: "drama",
              description: "a drama section"
            }
          ]
        ],

And

        sections: [
          [
            {
              title: "romance",
              description: "a romance section"
            },
            {
              title: "drama",
              description: "a drama section"
            }
          ]
        ],

[yangningBU]

I would permit both

[sdhull]

I've added a PR for permit! because I don't think this fixes permit! for nested arrays. However if you wanted to roll that fix into this PR that would be reasonable and then we could simply close #32593

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[rails-bot]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.