Skip to content
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

Merged
merged 6 commits into from May 29, 2020
Merged

Encode form data according to Encoding Object #1500

merged 6 commits into from May 29, 2020

Conversation

abcang
Copy link
Contributor

@abcang abcang commented Mar 22, 2020

Description

Serialize value with respect to Encoding Object when the content-type of the request body is application/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

  • No code changes (changes to documentation, CI, metadata, etc)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

}),
skipEncoding: true
}
req.query[parameter.name] = {
Copy link
Contributor Author

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) {
Copy link
Contributor Author

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

@abcang abcang Mar 22, 2020

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.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#encoding-object

@@ -69,7 +70,7 @@ test(
headers: {
'Content-Type': 'application/x-www-form-urlencoded',
},
body: 'industries=1%2C16'
body: 'industries=1,16'
Copy link
Contributor Author

@abcang abcang Mar 22, 2020

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}`,
Copy link
Contributor Author

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.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#style-examples

@@ -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}`,
Copy link
Contributor Author

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.

https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#style-examples

@@ -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}`,
Copy link
Contributor Author

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

@tim-lai tim-lai self-assigned this May 6, 2020
@tim-lai
Copy link
Contributor

tim-lai commented May 13, 2020

@abcang Thanks much for this PR. It looks well thought out. A related PR #1527 is inbound that should take care of the multipart case for both OAS2 and OAS3.0 while maintaining the existing urlecoded case. However, I am not sure if it directly address the title's application/x-www-form-urlencoded serialization case. If #1527 does not resolve your issue, please make any updates as needed, and I would be happy to take another look.

@@ -63,41 +69,41 @@ export default {
}
}
},
'/land/content/uploadImage': {
Copy link
Contributor Author

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',
Copy link
Contributor Author

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.

https://github.com/OAI/OpenAPI-Specification/blob/v3.1.0-dev/versions/3.1.0.md#encoding-object

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 ...)

@tim-lai
Copy link
Contributor

tim-lai commented May 18, 2020

@abcang Update: Merged and released #1527 only ensured maintain of existing support of OAS3 x-www-form-urlencoded. After some additional QA, implementation of OAS3 x-www-form-urlencoded itself does need improvement. We should account for explode=true, explode=false, explode (default).

@frantuma frantuma added this to the M2 milestone May 25, 2020
Copy link
Contributor

@tim-lai tim-lai left a 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.

test/http-multipart.js Outdated Show resolved Hide resolved
test/http-multipart.js Outdated Show resolved Hide resolved
test/http-multipart.js Outdated Show resolved Hide resolved
@tim-lai
Copy link
Contributor

tim-lai commented May 29, 2020

@abcang Thanks for the update again! We need one more pass at this, due to the recent dependency reversion/patch of form-data. I've resolved the merge conflicts and requested changes to fix the tests that broke as a result.

test/http-multipart.js Outdated Show resolved Hide resolved
test/http-multipart.js Outdated Show resolved Hide resolved
@tim-lai tim-lai merged commit 651e857 into swagger-api:master May 29, 2020
@tim-lai tim-lai mentioned this pull request May 29, 2020
10 tasks
@abcang abcang deleted the encode_form_data branch May 30, 2020 03:46
@abcang
Copy link
Contributor Author

abcang commented May 30, 2020

Thank you for reviewing and fixing the test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encoding style not respected for requestBody
3 participants