BodyParser's fallback "_" key in env["router.params"] unnecessary for typical x-www-form-urlencoded bodies?

[timriley]

Hi gang (/cc @jodosha @GustavoCaso)

Today I pushed a branch to hanami/controller with updated specs for the new BodyParser middleware that was introduced here in v1.3.0.beta1.

Making this change raised some new failing specs in hanami/controller. Here's an example:

Failures:

  1) Hanami::Router integration resource calls POST create
     Failure/Error: expect(response.body).to   eq(%({:identity=>{:avatar=>{:image=>\"jodosha.png\"}}}))

       expected: "{:identity=>{:avatar=>{:image=>\"jodosha.png\"}}}"
            got: "{:identity=>{:avatar=>{:image=>\"jodosha.png\"}}, :_=>\"identity[avatar][image]=jodosha.png\"}"

       (compared using ==)
     # ./spec/integration/hanami/controller/routing_spec.rb:52:in `block (3 levels) in <top (required)>'

As you can see, the "router.params" key set on the rack request env is getting a new "_" key set on it as part of the way BodyParser#_symbolize works:

      # @api private
      def _symbolize(body)
        if body.is_a?(Hash)
          Utils::Hash.deep_symbolize(body)
        else
          { FALLBACK_KEY => body }
        end
      end

If the body isn't a hash, it can't be symbolised, so it assigns it in full to a _ key on a new hash.

I presume this is to support e.g. things like JSON arrays that are submitted to an endpoint.

However, this causes some jank to appear for typical x-www-form-urlencoded request bodies. These are all just plain strings, so the body parser treats them in the same way and assigns them to the _ key.

However, later on, rack itself knows how to parse them and make them accessible as a params hash. But because we've assigned a copy of the body to _, we end up with its original string representation sticking around on the params hash. This feels like an unwanted behaviour to me.

Would you agree?

I think a solid fix might actually be to change the way we do the parsing/symbolizing in BodyParser. Right now we have a "null parser" kind of arrangement, where @parsers[media_type(env)] always returns an object with a #parse method, even if there is no explicitly matching parser type. The "null parser" in this case is BodyParser::Parser and its #parse just returns body unmodified.

The problem with this is that BodyParser kind of expects that every body is parseable. But this is not actually the case. I think it might be better to properly recognise when we don't have a parser available for the particular body type, and then in that case do nothing, i.e. don't assign "router.parsed_body" or "router.params" at all on the env hash. This will mean we avoid the situations where we try to symbolize a body that's not appropriate for symboilization, and then we can allow rack to do the right thing later on, if it can (e.g. for the typical x-www-form-urlencoded request bodies).

If you're happy with this approach, I can get a PR together this coming week. I feel it'd be good to do soon because otherwise I think we'll be shipping 1.3.0 with this new middleware behaviour in a way that pollutes people's request params hashes.

[jodosha]

The FALLBACK_KEY was introduced by #86 for a problem spotted in production (#85) for JSON arrays.

---

@timriley I agree with your analysis: we shouldn't attempt to parse a HTTP body if there isn't a matching parser for the current request.

If this change will not produce any breaking change. I'm 👍 for it. Thanks!

[jodosha]

Fixed by #181