ruby-grape / ruby-grape/grape

Declared broken for hash params with overlapping names

Open
#2,195 3 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug?
Dominant language
Ruby
Stars
10k
Forks
1.2k
Avg merge
14h 38m
Merged PRs (30d)
92

Description

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?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Reproduce the overlapping-name and unique-name cases in the issue, then inspect lib/grape/dsl/inside_route.rb around the line linked by the reporter and compare it with PR 2103. Check the declared behavior against the documented include_missing and nil-value behavior; done means explicit null Hash parameters retain the intended distinction without regressing missing-parameter handling.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
api, backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.