Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declared broken for hash params with overlapping names #2195

Open
johnk87 opened this issue Oct 18, 2021 · 3 comments
Open

Declared broken for hash params with overlapping names #2195

johnk87 opened this issue Oct 18, 2021 · 3 comments
Labels

Comments

@johnk87
Copy link

johnk87 commented Oct 18, 2021

Upgrading from 1.3.3, noticed several bugs around declared. I think some of them already have issues/PRs (eg. #2112 #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 #2103, my guess is in this line

has_children = route_options_params.keys.any? { |k| k != key && k.start_with?(key) }


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?

@dblock
Copy link
Member

dblock commented Oct 18, 2021

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.

@jcagarcia
Copy link
Contributor

I've just sent a PR for solving this #2372

jcagarcia added a commit to jcagarcia/grape that referenced this issue Nov 17, 2023
dblock added a commit that referenced this issue Nov 17, 2023
fix(#2195): Fix `declared` method for hash params with overlapping names
@jcagarcia
Copy link
Contributor

Solved in #2372 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants