Corrected the endpoint parameter name generation.

[Jack12816]

- What is it good for

I've expected bad parameter documentation with grape-swagger when renaming with as: .... The expected behavior is that the original parameter name is used for documentation. Let's look at this example:

optional :address, type: Hash, as: :address_attributes do
  optional :street, type: String
  optional :street_addition, type: String
  optional :city, type: String
end 

I want my API clients to send the address parameter, and in my endpoint definition I want to update the entity (AR, accepts_nested_attributes_for enabled) by using user.update!(declared(params, include_missing: false)). Sure, it's just convenient to rename the parameter for internal use, but that's how it is documented. The resulting Swagger definition looks like this:

This is clearly the internal name that should not be sent nor be documented. This pull request addresses the documentation issue.

- What I did

I just added the original parameter name when a renaming was done which is then used as the parent element name. Additionally, I added a bunch of examples to get this right.

- Open questions

As you might see in the specs I encountered some cases that should behave differently. This boils down to the ability to bypass parameter validation for renamed scalar parameters when the client knows/guesses the internal name. The renaming is implemented right now as a validator which just adds the renamed parameter name into the params hash. When we then use the declared(params, include_missing: false) method in order to get only the declared and validated parameters we can be tricked.

I was wondering why the renaming is not just part of the declared method implementation. This would prevent validation bypassing, while also allows us to ignore parameters we do not expect. So we could ensure the following scenarios:

# (1) Rename a to b, while sending a
optional :a, type: Integer, as: :b
params = { a: 1 }
declared(params, include_missing: false)
# expected => { b: 1 }
# actual   => { b: 1 }

# (2) Rename a to b, while sending nothing
optional :a, type: Integer, as: :b
params = { }
declared(params, include_missing: false)
# expected => { }
# actual   => { }

# (3) Rename a to b, while sending b
optional :a, type: Integer, as: :b, values: [1, 2, 3]
params = { b: '5' }
declared(params, include_missing: false)
# expected => { }
# actual   => { b: '5' } (uncasted, unvalidated)

Is there something I'm missing here? I would like to address this too if the issue is reasonable.

- A picture of a cute animal (not mandatory but encouraged)

[Jack12816]

I spend some time on the parameter renaming just inside the #declared(params) method which works now quite fine. I put it in a derived branch from this at https://github.com/Jack12816/grape/tree/renaming (check the last 2 commits). In this branch, the AsValidator is just a marker and does nothing anymore.

[dblock]

I think you're spot on, and should fix the intended behavior as you described. Your proposed solution is also quite simple. Happy to review on green!

[Jack12816]

I think you're spot on, and should fix the intended behavior as you described. Your proposed solution is also quite simple.

Okay, great. I merged my renaming solution into this branch :)

Happy to review on green!
test / test (ruby-head, true) (pull_request) Failing after 22s — test (ruby-head, true)

This fails on master, too. expected NoMethodError with "undefined method not_exist_method'", got #<NoMethodError: undefined method not_exist_method'

[dblock]

This is excellent work. thank you.

I have some nits. Plus this changes behavior that I believe we need to document in UPGRADING, please?

[Jack12816]

This is excellent work. thank you.

Thanks :)

I have some nits. Plus this changes behavior that I believe we need to document in UPGRADING, please?

I took care of the nits and added an upgrading section. I would not recommend releasing this change as a patch version because it breaks current behavior, even though it's a bug fix in the narrower sense.

[dblock]

Agreed on the minor bump. Let's bump the version in this PR to 1.6 as well, please. Don't forget to re-update the numbers in UPGRADING.

[Jack12816]

Let's bump the version in this PR to 1.6 as well, please. Don't forget to re-update the numbers in UPGRADING.

Did it :)

[dblock]

Merged, thank you!

[Jack12816]

What do you think about a release in the next few days? :)

[dblock]

What do you think about a release in the next few days? :)

I'll get to it sooner than later unless another maintainer can beat me to it.

[Jack12816]

ping @dblock - is there anything blocking a release? :)

[dblock]

Released 1.6.0