Breaking change for nested params in declared in Grape 1.3.3

[tlconnor]

A PR was merged back in April and included in Grape 1.3.3 that has changed the way declared works.
#2043

Before this PR was merged, the declared helper would guarantee that the entire params structure would be recreated, including deeply nested parameters. Any leaf params not provided would have nil values.

With this merged PR, any missing Hash params would be evaluated as {}, even if they has deeper nested params defined.

Here is an example from endpoint_spec.rb. The params are defined as:

subject.params do
  requires :first
  optional :second
  optional :third, default: 'third-default'
  optional :nested, type: Hash do
    optional :fourth
    optional :fifth
    optional :nested_two, type: Hash do
      optional :sixth
      optional :nested_three, type: Hash do
        optional :seventh
      end
    end
    optional :nested_arr, type: Array do
      optional :eighth
    end
  end
  optional :arr, type: Array do
    optional :nineth
  end
end

Given a test that calls get '/declared?first=present', the result of declared(params) would be:

In Grape 1.3.2:

{
  "first": "present",
  "second": null,
  "third": "third-default",
  "nested": {
    "fourth": null,
    "fifth": null,
    "nested_two": {
      "sixth": null,
      "nested_three": {
        "seventh": null
      }
    }
  },
  "nested_arr": []
}

In Grape 1.3.3:

{
  "first": "present",
  "second": null,
  "third": "third-default",
  "nested": {},
  "arr": []
}

It is not clear which of these is the correct behavior. The merged PR #2043 would suggest that the latter is correct, however it is actually very useful to have the previous behavior from 1.3.2.

The README isn't clear about what the behavior should be for deeply nested params.

[dblock]

Ouch, this keeps biting us, cc: @kadotami and @dnesteryuk.

Naively reading our own README it feels like if you say include_missing: true, we should get the entire structure. Do you want to attempt a change/fix for this and see whether we are breaking any specs? Reading #2043 it feels that we simply didn't consider this, but I could be wrong.

[tlconnor]

My interpretation is that you should get the whole (aka declared) structure, and there are certainly benefits to that behavior. The specs before #2043 were ambiguous - there was no spec that definitively asserted or refuted the behavior.

If we can get a definitive design decision from the maintainers of Grape one way or the other I would be happy to submit a PR to update the README / code / specs.

[tlconnor]

Just for a bit of additional context, we have many use cases that looks something like this:

params do
  optional :filter, type: Hash do
    optional :widgets, type: Hash do
      optional :name, type: String
    end
    optional :gadgets, type: Hash do
      optional :name, type: String
    end
  end
end

In the API body, we would be able to go:

filter = declared(params)[:filter]
filter_by_widget_name if filter[:widgets][:name].present?

In Grape 1.3.2 this would work fine.
In Grape 1.3.3 this would fail with a undefined method []' for nil:NilClasserror due tofilter[:widgets]beingnil`.

If we were to preserve the existing behavior, the API code would need to change to something like:

filter = declared(params)[:filter]
filter_by_widget_name if filter.dig(:widgets, :name).present?

[dblock]

My interpretation is that you should get the whole (aka declared) structure, and there are certainly benefits to that behavior. The specs before #2043 were ambiguous - there was no spec that definitively asserted or refuted the behavior.

If we can get a definitive design decision from the maintainers of Grape one way or the other I would be happy to submit a PR to update the README / code / specs.

I vote for expanding deep as you suggest. If others can add a +1 that'd be great.