The Wayback Machine - https://web.archive.org/web/20210106155326/https://github.com/ruby-grape/grape/issues/2094
Skip to content

/ grape

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

Jump to bottom

Issue with the order of as: and default: options on param declarations #2094

Open
schinery opened this issue Aug 6, 2020 · 4 comments
Open

Issue with the order of as: and default: options on param declarations #2094

schinery opened this issue Aug 6, 2020 · 4 comments
Labels
bug?

Comments

@schinery
Copy link

@schinery schinery commented Aug 6, 2020

Am fairly new to Grape, so apologies if this has come up elsewhere already but doing a quick scan I couldn't see anything covering this issue I was having with using both as and default param options.

If I use the following to set an endDate param with a default value and convert it to end_date then the default value behaviour works as expected.

params do
  optional :endDate, type: Date, default: (Date.current + 50.days).end_of_month, as: :end_date
end

Would result in the default param value being set when no param was sent:

(byebug) params
{"end_date"=>Wed, 30 Sep 2020}

However if I use following for the same request, and specify the as option first, the default value is never set on the params, even when using declared (although not sure this would make a difference anyway).

params do
  optional :endDate, as: :end_date, type: Date, default: (Date.current + 50.days).end_of_month
end

Would result in:

(byebug) params
{}
(byebug) declared(params)
{"end_date"=>nil}

I'm assuming this might be a bug with how the param options are being parsed by Grape but wanted to also make sure I wasn't doing something dumb.
@schinery
Copy link
Author

@schinery schinery commented Aug 6, 2020

As a side note, if there is a better way to convert all incoming camelcase params to snake case without having to use the as option when declaring a param I'd love to know 👍
@dblock
Copy link
Member

@dblock dblock commented Aug 6, 2020 •
edited

This looks like a possible bug indeed, try turning it into a spec?

Regarding camelcase, I would try swapping the parameter builder to a Hashie extension that underscores all keys treats all camel case keys indifferently with underscore keys. I believe Rash does that, but it's built on top of Mash and I wouldn't use that because that's crazy. My recommendation would be to write a Hashie extension that only does that. I'd be interested in seeing the results, including both a contribution to hashie and documentation in Grape/blog post for how to accomplish this.

Btw, you probably want a lambda to evaluate on every request, default: -> { (Date.current + 50.days).end_of_month }.
@schinery
Copy link
Author

@schinery schinery commented Aug 6, 2020 •
edited

Hey @dblock, I can certainly try putting a spec together for this when I get some time to, same as the Hashie extension.

Regarding the default: -> { (Date.current + 50.days).end_of_month }, I had mine set as this originally, but this produced "default": { } in the Swagger JSON so I thought I had set it up wrong and removed the lambda. The Swagger JSON looked like:

{
  "in": "query",
  "name": "endDate",
  "description": "End date of query",
  "type": "string",
  "format": "date",
  "default": { },
  "required": false
}

Which then sets in the Swagger UI (using https://petstore.swagger.io) the default value to Default value : OrderedMap {}

If this is another bug I can put this in another ticket if that helps keep things separated?
@dblock
Copy link
Member

@dblock dblock commented Aug 9, 2020

If this is another bug I can put this in another ticket if that helps keep things separated?

That's a grape-swagger issue, so you should probably open a bug there. That said, it's not supposed to interpret a dynamic default value into some kind of documentation. Not sure what one would expect here.

For your API, I suspect you don't want Date.current to be some value every time your server starts/grape loads, but actually change on every request, hence you need a ->.
@dblock dblock added the bug? label Aug 9, 2020
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
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
@schinery @dblock
You can’t perform that action at this time.