Fix handling explicit nil values

[ccdredzik]

Should solve #80

[ccdredzik]

The original premise for this test is #70. It is described as "Ensures that passing a string to a controller that expects an array of strings raises an invalid parameter error". This is not yet what the test does. It doesn't pass the string as the array. It passes the string as the definition of the hash containing an array attribute.

When you actually configure the test to send a string where an array would be, the test fails.

[ifellinaholeonce]

What's the value here in removing that test case tho? If the description of the test was inaccurate, let's address that. However, this was an edge case bug that someone encountered and added the specs for, so let's keep that coverage.

[ccdredzik]

@ifellinaholeonce fair do's. I have assumed that someone just did wrote a rather silly test. I have restored it and it is now failing.

[ccdredzik]

The ParamEvaluator is now receiving the following params:

#<ActionController::Parameters {"filter"=>"state", "controller"=>"fake", "action"=>"nested_array"} permitted: false>
{"state"=>nil}

[ccdredzik]

From my understanding HashParam will happily coerce filter: "state" into filter: { state: nil } which will result in the glorious:

       expected RailsParam::InvalidParameterError, got #<NoMethodError: undefined method `each_with_index' for nil:NilClass> with backtrace:
         # ./lib/rails_param/param_evaluator.rb:51:in `recurse_on_parameter'
         # ./lib/rails_param/param_evaluator.rb:35:in `param!'
         # ./spec/fixtures/controllers.rb:37:in `block in nested_array'
         # ./lib/rails_param/param_evaluator.rb:66:in `recurse'
         # ./lib/rails_param/param_evaluator.rb:59:in `recurse_on_parameter'
         # ./lib/rails_param/param_evaluator.rb:35:in `param!'
         # ./lib/rails_param/param.rb:3:in `param!'
         # ./spec/fixtures/controllers.rb:36:in `nested_array'

[iMacTia]

Yeah we should support an array of hashes as well, that's pretty much the only meaningful scenario for the block. I also agree the shorthand you show above would make sense, the block seems a bit too much in that case

[ccdredzik]

@iMacTia @ifellinaholeonce I have added an awful lot of tests. Can you double check if the assumptions they make are all correct?

[ccdredzik]

@iMacTia @ifellinaholeonce and a last thing for today: In param_spec.rb on line 472 there is a test case as follows:

it 'does not raise exception if array is not required but nested attributes are, and no array is provided' do

Which stands in direct contradiction to what we just established... I kind of want to give up.

[iMacTia]

@ifellinaholeonce I've further discussed this with @ccdredzik offline, I agree there have been some confusion after the refactoring around this point. We agreed on dropping this test because, although the premises are good, it does not work well with hash and array coercion, unfortunately.

We all agree that a nil and missing params should be considered in the same way, so it would make sense to skip the recursive coercion when the parameter is provided, but is nil. Exactly as we do with a missing parameter.

This test can potentially be reintroduced later on, but we'll need to write it using a better payload system, as query-strings clearly don't work as expected and make it hard to reproduce the scenario we wanted to test

[ccdredzik]

I have pushed the implementation to the latest understanding of the contract. Please feel free to review.

[iMacTia]

Quick recap in case something went missing in the long discussion thread:

• This PR follows the rules highlighted here
• This PR introduces a bunch of new tests
• This PR changes 2 existing tests, but we agreed that's the right course of action:
  • Test 1: This test was incorrectly setup to begin with. Due to the way hash coercion works, the payload { filter: 'state' } would become { filter: { state: nil } } in the controller params. The test was then failing due to state being nil, and not because it was a string.
  • Test 2: this was introduced in the refactoring, but breaks the rules highlighted above (nil have always been treated as missing param, we should stick with that).

Big thanks @ccdredzik for the extensive research, patience and commitment on getting this rectified and for highlighting different weak points in the current implementation that should be addressed properly on our way towards v2.0 👏 🙏

[iMacTia]

@ifellinaholeonce I'm happy to get this merged now, would you like to take one final pass and raise any issue from your perspective before that happens?

[ifellinaholeonce]

This looks great to me! Thanks for all the hard work and back forth on this issue @ccdredzik. It is a tricky subject.

@iMacTia thanks for driving the conversation on this. I'm ready for this to be merged. I think we should make a note of this in the release notes tho. I believe this is a breaking change from the last release, around that array with nested required elements.

[iMacTia]

I agree, it should be noted. Though we should also clarify we're effectively reinstating behaviour prior to v1.0 that was unintentionally broken, to justify the minor version bump

[iMacTia]

@ifellinaholeonce here is the release draft, please give me a quick feedback (or feel free to change!) when you have a minute: https://github.com/nicolasblanco/rails_param/releases/tag/untagged-b8a8e53920572a12295e