-
Notifications
You must be signed in to change notification settings - Fork 762
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
Encode form data according to Encoding Object #1500
Conversation
}), | ||
skipEncoding: true | ||
} | ||
req.query[parameter.name] = { |
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.
Since encoding is not required when sending using FormData, encoding has been changed to execute when executing mergeInQueryOrForm, as in the Open API v2.
// Return value example 4: [['color', 'R,100,G,200,B,150']] | ||
// Return value example 5: [['R', '100'], ['G', '200'], ['B', '150']] | ||
// Return value example 6: [['color[R]', '100'], ['color[G]', '200'], ['color[B]', '150']] | ||
function formatKeyValue(key, input, skipEncoding = false) { |
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.
Since the key may change depending on the value, formatValue
has been changed to a method that returns a key-value pair
@@ -35,7 +35,8 @@ const spec = { | |||
}, | |||
encoding: { | |||
industries: { | |||
style: 'form' | |||
style: 'form', | |||
explode: false |
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.
If style is form
, the default value of explode is true
. However, this test seemed to assume the behavior when explode was false
, so I set explode to false
.
@@ -69,7 +70,7 @@ test( | |||
headers: { | |||
'Content-Type': 'application/x-www-form-urlencoded', | |||
}, | |||
body: 'industries=1%2C16' | |||
body: 'industries=1,16' |
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.
Should ,
obey allowReserved
?
Currently it follows the example table, regardless of allowReserved
.
https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#style-examples
@@ -699,7 +699,7 @@ describe('OAS 3.0 - buildRequest w/ `style` & `explode` - query parameters', () | |||
|
|||
expect(req).toEqual({ | |||
method: 'GET', | |||
url: `/users?id=3 id=4 id=5 id=${SAFE_INPUT_RESULT}`, | |||
url: `/users?id=3&id=4&id=5&id=${SAFE_INPUT_RESULT}`, |
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.
There is no example when style is spaceDelimited or pipeDelimited and explode is true, but the same behavior as when style is form is probably correct.
@@ -781,7 +781,7 @@ describe('OAS 3.0 - buildRequest w/ `style` & `explode` - query parameters', () | |||
|
|||
expect(req).toEqual({ | |||
method: 'GET', | |||
url: `/users?id=3 4 5 ${SAFE_INPUT_RESULT}`, | |||
url: `/users?id=3%204%205%20${SAFE_INPUT_RESULT}`, |
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.
According to the example table, when style is spaceDelimited
, the value is delimited by %20
.
@@ -1327,7 +1327,7 @@ describe('OAS 3.0 - buildRequest w/ `style` & `explode` - query parameters', () | |||
|
|||
expect(req).toEqual({ | |||
method: 'GET', | |||
url: `/users?id[role]=admin&id[firstName]=Alex&id[greeting]=${SAFE_INPUT_RESULT}`, | |||
url: `/users?id%5Brole%5D=admin&id%5BfirstName%5D=Alex&id%5Bgreeting%5D=${SAFE_INPUT_RESULT}`, |
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.
encodes [
and ]
according to allowReserved
@abcang Thanks much for this PR. It looks well thought out. A related PR #1527 is inbound that should take care of the |
@@ -63,41 +69,41 @@ export default { | |||
} | |||
} | |||
}, | |||
'/land/content/uploadImage': { |
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.
just fixed the indent
}, | ||
encoding: { | ||
'email[]': { | ||
style: 'form', |
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 interpreted that if no Encoding Object is set, explode
will not be enabled by default and the format will be based on the default value of contentType.
So I thought I had to explicitly specify explode
. (I personally felt that it would be more natural to treat it as explode
by default ...)
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.
These changes will fix the tests that broke after resolving merge conflicts.
@abcang Thanks for the update again! We need one more pass at this, due to the recent dependency reversion/patch of |
Thank you for reviewing and fixing the test! |
Description
Serialize value with respect to
Encoding Object
when the content-type of the request body isapplication/x-www-form-urlencoded
.https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#encoding-object
Also, OpenAPI v3.1 has a specification to serialize similarly when content-type is
multipart/form-data
, so it was implemented at the same time.https://github.com/OAI/OpenAPI-Specification/blob/v3.1.0-dev/versions/3.1.0.md#encodingObject
In addition, a bug that prevented sending an array of files was also fixed.
Motivation and Context
Currently, swagger-client does not work according to the serialization specification of the request body of the form. This pull request is modified to respect the request body serialization settings.
In OpenAPI v3.1, the serialization settings will be respected even when the content-type is
multipart/form-data
. Although the specification has not yet been officially released, I implemented it before release because I felt it was practical.Fixes #1470
How Has This Been Tested?
I connected it to swagger-ui and did a simple operation check. Test cases have been added for important cases.
Screenshots (if appropriate):
Types of changes
package.json
)Checklist: