grape icon indicating copy to clipboard operation
grape copied to clipboard

Declared broken for hash params with overlapping names

Open johnk87 opened this issue 4 years ago • 3 comments

Upgrading from 1.3.3, noticed several bugs around declared. I think some of them already have issues/PRs (eg. https://github.com/ruby-grape/grape/pull/2112 https://github.com/ruby-grape/grape/pull/2001) but don't see this one:

when passing null to hash params with unique names, the output is consistent (but still incorrect I think, but that's a different issue, see a note at the bottom):

gem 'grape', '1.6.0'

require 'grape'

class API < Grape::API
  format :json

  params do
    optional(:aa, type: Hash)
    optional(:ab, type: Hash)
  end

  put do
    {
      params: params,
      declared: declared(params, include_missing: false)
    }
  end
end

run API

> curl -X PUT http://127.0.0.1:9292 -H 'Content-Type: application/json' -d '{"aa": null, "ab": null}' | jq
{
  "params": {
    "aa": null,
    "ab": null
  },
  "declared": {
    "aa": {},
    "ab": {}
  }
}

but when the param names overlap:

params do
  optional(:aa, type: Hash)
  optional(:aab, type: Hash)
end
> curl -X PUT http://127.0.0.1:9292 -H 'Content-Type: application/json' -d '{"aa": null, "aab": null}' | jq
{
  "params": {
    "aa": null,
    "aab": null
  },
  "declared": {
    "aa": null, # expected {}
    "aab": {}
  }
}

Present since 1.5.0, so it seems to be introduced by https://github.com/ruby-grape/grape/pull/2103, my guess is in this line https://github.com/ruby-grape/grape/blob/43936ac719ee524fd0f8ccd830d1eb50abd721c7/lib/grape/dsl/inside_route.rb#L98


Other than that: Upgrading to >= 1.5.0 says behaviour changes only when params are missing and include_missing=true Upgrading to >= 1.3.3 says that For now on, nil values stay nil values for all types, including arrays, sets and hashes.

so I assume changing nil to {} in the first example when null is passed explicitly (and include_missing isn't true as well) is an unintended behaviour as well?

This also makes it impossible to set nil value for params with type: Hash when using declared. While it can be bypassed by using eg. types: [Hash, anything] (single item triggers another bug where this time nil is changed to []), it's only because the mentioned PR doesn't take types under consideration at all, which is maybe yet another problem?

johnk87 avatar Oct 18 '21 07:10 johnk87

Ouch. These look like real problems. Turn your example(s) into specs and see if you can fix'em? At least PR the specs.

I think the other problem is real and needs to be similarly investigated.

dblock avatar Oct 18 '21 11:10 dblock

I've just sent a PR for solving this https://github.com/ruby-grape/grape/pull/2372

jcagarcia avatar Nov 17 '23 21:11 jcagarcia

Solved in #2372 :)

jcagarcia avatar Nov 17 '23 22:11 jcagarcia