Parsing empty array as array of [null] in json request

[acapilleri]

Now with the following params:

{ people: [{name: 'angelo', posts:[] }] }

In a normal http post are decoded as:

{ :people => [{:name => 'angelo', :posts => []} ]}

while in json request are decoded as:

{ :people => [{:name => 'angelo', :posts => nil }] }

With this PR , json request and html have the same decoded params
fixes #11023

[egilburg]

👍

I was bitten by this when building inter-service APIs and not understanding why passing arrays (which could be empty or not empty) caused empty ones to be nil on the receiver, leading to errors like nil.map calls. Worked around it by using Array() wrappers on the receiver, but it's nice to fix the root cause.

[egilburg]

Will this solve the case where you have nested arrays with an empty array in the nested one? e.g.

{"person" => [{"name" => "Angelo", "posts" =>[{"title" => "post 1", "comments" => []}]}]}

Looks like "comments" would still end up as nil rather than []

Perhaps this should recursively call deep_munge on each array element, similar to how it handles hashes.

[acapilleri]

I do not touch the logic of recursivity, but I have added only a special base case without any return, infact your test case is green

[lukesarnacki]

Unfortunately this code is wrong and it will replace [nil] and [nil, nil] with nil. The only thing that has to be done is removing line:

hash[k] = nil if v.empty?

[pivotal-xo]

We are also experiencing the same problem. When we submit a request with Content-type: application/json anything key with a value of [] becomes a nil value.

We'll be updating our code to use Array.wrap, but what would it take to get this merged in to a Rails 4.0.1rcX?

[acapilleri]

@pivotal-xo 👍

[ragaskar]

I would also love to see this fixed. Have burnt a few hours on this. Nice to know it's a regression and will eventually be resolved ...

[ragaskar]

Any good way to monkey-patch in this fix? I'm noticing it doesn't get picked up in initializers (loads too late, I suppose), and so I'm having to explicitly handle the nil case in each controller, which is pretty unpleasant.

[joshwlewis]

@ragaskar This patch is targeting edge Rails, where this method has actually moved from where it is in Rails 4.0. To monkey patch against Rails 4.0, putting this somewhere in config/initializers/ works for me:

module ActionDispatch
  class Request < Rack::Request
    def deep_munge(hash)
      hash.each do |k, v|
        case v
        when Array
          v.grep(Hash) { |x| deep_munge(x) }
          if v.empty?
            hash[k] = []
          else
            v.compact!
            hash[k] = nil if v.empty?
          end
        when Hash
          deep_munge(v)
        end
      end
      hash
    end
  end
end

Most likely your initializer was working, it's just that Rails 4.0 expects this method to be at ActionDispatch::Request#deep_munge, where edge Rails has it at ActionDispatch::Request::Utils.deep_munge.

[ragaskar]

@joshwlewis Thanks for that -- dropped it in and it started getting picked up and loaded.

My problem isn't entirely fixed -- rspec request specs are passing an empty rack.input (request body) in the case where my params are foo[]= (and so this fix can't operate on it) ... If there are members of the array it comes through normally. In ActionController::Integration my params are properly set as {:foo => []}, then they get lost in the wild of session request handling. 'Real' requests seem to operate correctly at this point, however, so maybe I'll just give up on testing this behavior.

Relatedly, and somewhat surprisingly, params.require(:foo) raises if foo is []. (Sure, it's blank, but it's truthy). I had to switch to params.permit(:foo)[:foo] which is somewhat unfortunate (and possibly unnecessary?).

[joshwlewis]

@ragaskar Strong parameters won't accept array scalars unless you tell it you want an array. For instance: params.permit(foo: []).

[aroc]

@joshwlewis solution worked fantastic for me! Thanks for sharing!

[acapilleri]

@carlosantoniodasilva there are a reason to not merge this PR?

[carlosantoniodasilva]

@acapilleri I'll need to check with the security folks before taking any action on this one.

[acapilleri]

@carlosantoniodasilva thanks for the clarification

[chancancode]

Hey guys, thank you for submitting this and expressing your concerns about this issue! Speaking for myself, I understand where you are coming from, and I agree we could do better. Unfortunately, this PR does not meet our requirements for an acceptable solution to the problem.

Please head over to #13420 for the background and contribute your ideas. Thanks again! ❤️ 💚 💙 💛 💜

[pivotal-xo]

@chancancode, why did this fail to meet the criteria? We are experience this issue still, which appears to be a pretty easy fix. If your other issue #13420 is solving this problem, could you clarify how it relates to the above issue? I feel like #13420 is fixing a larger problem, where the above would fix our problem immediately.

[acapilleri]

@pivotal-xo I honestly think that decision come from a Rails core Team member, if not I'd like to understand where we are going...

[chancancode]

I already commented over there, but I want to keep @pivotal-xo in the loop too. There's a chance that I mistakenly closed this while trying to funnel related discussions to the centralized ticket. I'll double check and comment here if I have anything to report.

In any case, I want to bring #13157 to your attention :) with that, if you are already taking the appropriate precautions (calling to_s etc) you will be able to disable this. That's a temporary solution definitely, so if you have any suggestions/concerns about potential solutions, do voice your concern over at the other ticket!

Sorry for the confusion I might have caused! 😅

[chancancode]

By the way, the "criteria" I referenced did come from a core team member (@jeremy), you can see the details in my ticket if you are curious. :)

If you just want to focus on solving this immediate problem, I understand too. With the option to turn this off now, you'll be covered either way. (Either something like this get merged, or you get to turn it off and do your own limited protection.)

[imanel]

I might be wrong, but table for this one will probably look like this:

JSON	Hash
{"person":null}	{'person' => nil}
{"person":[]}	{'person' => []}
{"person":[null]}	{'person' => nil}
{"person":[null, null, ...]}	{'person' => nil}
{"person":["foo", null]}	{'person' => ["foo"]}

The reason is that it checks empty first to set as empty array, but second check after compact converts it to nil. This is obviously invalid, as it makes it even more confusing. #12251 is little better in this case so if you absolutely sure that you need to still push it then move to it and leave this one.

[acapilleri]

The reason is that it checks empty first to set as empty array, but second check after compact converts it to nil.

I think you lost the point, the empty array will never compact to nil.
As said in a preview comment this PR simply add a base case to the recursive method/function.
A base case return the same value for the same input, and in our case for the inpunt [] and the value returned is []stopping its recursion node here.

[imanel]

Please review table above and confirm if it's valid - if not I will be happy to update.

[chancancode]

Update: I haven't forgotten about this, but it's holiday season, so it might take a while for me to confirm if we are interested in these types of fixes (and #12251). So bare with me :)

Meanwhile, it would be helpful to run the code and c/d the table posted above (and update accordingly). Having a table like this would definitely help speed up the review (and if the current table is correct then it's most likely a no-go).

I don't have time to do this at the moment, so if someone else can pick that up it'd be awesome 😁

[imanel]

@chancancode here's script that will allow you to tests it. Output is attached and confirms tables for both this pull request and #12251.