Skip to content

Breaking change for nested params in declared in Grape 1.3.3 #2101

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.

Sign up for GitHub

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

Jump to bottom
Closed
tlconnor opened this issue Sep 11, 2020 · 4 comments · Fixed by #2103
Closed

Breaking change for nested params in declared in Grape 1.3.3 #2101

tlconnor opened this issue Sep 11, 2020 · 4 comments · Fixed by #2103
Labels
bug?

Comments

@tlconnor
Copy link

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
Copy link
Member

dblock commented Sep 11, 2020

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.

@dblock dblock added the bug? label Sep 11, 2020
@tlconnor
Copy link
Author

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
Copy link
Author

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
Copy link
Member

dblock commented Sep 14, 2020

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.

@tlconnor tlconnor mentioned this issue Sep 17, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug?
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Ensure complete declared params structure is present tlconnor/grape
2 participants
@tlconnor @dblock