-
Notifications
You must be signed in to change notification settings - Fork 427
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
Fix request building with formData parameters #451
base: master
Are you sure you want to change the base?
Fix request building with formData parameters #451
Conversation
ff9f768
to
6756cc9
Compare
.select { |p| p[:in] == :formData } | ||
.map { |p| [p[:name], example.send(p[:name])] } | ||
Hash[tuples] | ||
form_data_param = parameters.select { |p| p[:in] == :formData }.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That .first
worries me. Doesn't this just work with the 1st form parameter?
{ name: 'f1', in: :formData, type: :string },
{ name: 'f2', in: :formData, type: :string }
Wouldn't f2
be excluded?
The test seemed to test for 2 parameters in FormData, but now it only tests one. Could you return the test to have 2 form parameters to make sure it works with more that 1 form parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sergioisidoro Thanks for your feedback! Yes, that's exactly how it would work. I have explained the reasoning in the "solution" paragraph. body
parameters work the same way.
Another solution would be to make swagger_formatter accept multiple parameters because now it accepts only the first one and logic in these places diverge.
I agree this solution isn't great and is more of a quick fix. Currently, to work around this issue we define the first formData param just for SwaggerFormatter
and then redefine the same schema in separate params to make RequestFactory
work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let me take a bit longer to look into this, and see if I can figure out the historical background behind the body param. My guess is that it is also not supposed to be like that 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the current implementation is actually working for people. #348 is a long thread of people resigned to either having the integration test work, or their generated documentation being correct. I was about to submit pretty much this same PR but happy to see it's already been done.
I also think that conceptually it makes sense to only have a single body parameter supported since you can only send one body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have come up against this issue recently. And I am very keen to solve it, happy to work on it if we get more ideas on the best solution.
+1 :-) |
1 similar comment
+1 :-) |
6756cc9
to
303f68c
Compare
I have run into this issue recently and the workaround here: #348 (comment) worked for me. parameter name: :'', in: :formData, schema: {
type: :object,
properties: {
client_id: { type: :string },
client_secret: { type: :string },
grant_type: { example: 'client_credentials', type: :string },
},
required: [ 'client_id', 'client_secret', 'grant_type' ]
}
response '200', 'Token created' do
example 'application/json', :succesful_token_example, {
'access_token': 'FeXebuMLItU3UPpcXlUgGkxeXoWLgX9CULUQ6FBAFjs',
'token_type': 'Bearer',
'expires_in': 7200,
'scope': 'public',
'created_at': 1677685057
}
let(:'') { { client_id: 'client_id', client_secret: 'client_secret', grant_type: 'client_credentials' } }
run_test!
end |
I never thought I'd say this... but I think I prefer the monkey patch 😞 |
Which solution was the monkey patch? |
Just faced this issue today - I don't think there's currently a good way to document Looking forward to this change :) |
any update on this? |
Problem
Swaggerization part and request building part of
formData
parameters behave differently. With this commitformData
parameters are being filtered out and only the first one's:schema
definition is used inside endpoint description (same as withbody
parameters). However, inrequest_factory
every singleformData
param is used to build a payload.So with parameter definition like this:
An empty
requestBody
definition for that API action is generated (because there is no:schema
key)With this variation:
the correct swagger definition is generated, but the controller receives the
name
parameter that is nested inside theform_data
object. And this does not match the defined schema.Solution
This can be fixed by either changing
request_factory
so it puts firstformData
parameter inside request body (implemented in this PR), or changingswagger_formatter
so that it builds schema definition from multipleformData
parameters. Don't know the reason whyswagger_formatter
was initially implemented this way, so I went with the first option.