fix: issue where application/x-www-form-urlencoded payloads weren't being handled properly #807

domharrington · 2020-06-26T22:58:37Z

🧰 What's being changed?

This resolves a couple bugs with how we're generating HAR definitions:

For application/x-www-form-urlencoded operations, we were storing their payloads inside of postData.text instead of postData.params.
- This required an update to fetch-har to support postData.params. See fix: improper handling of application/x-www-form-urlencoded fetch-har#67 for details.
We were always sending postData regardless if there was actually any data for it. For simple GET requests that just have queryString populated, the postData object is no longer part of the created HAR.
- This required an update to fetch-har to support postData no longer being an expectation. See fix: improper handling of application/x-www-form-urlencoded fetch-har#67 for details.
The HAR definitions that oas-to-har was creating didn't actually validate outside of httpsnippet. This was because that library adds some defaults (like httpVersion) if they're missing. Since oas-to-har should support the HAR spec and not HTTP Snippets handling of it, I've made the decision to:
- Add those missing defaults to generated definitions.
- Update the unit test suite to assert that we're always generating valid HAR definitions when running our other assertions.

🧪 Testing

You can see this broken in action here: http://bin.readme.com/s/5efbb6ac35bcd40024d60488

Load up the local server and the formData example to see how it's handled how: http://localhost:9966/?selected=swagger-files%2Fformdata.json

🗳 Checklist

💡 If answering yes to any of the following, include additional info, before/after links, screenshots, etc. where appropriate!

🆕 I'm adding something new!
🐛 I'm fixing a bug!
📸 I've made some changes to the UI!

I added two test cases: one for json and one for formData. The json
one passes fine! This code in oas-to-har is working as expected:

api-explorer/packages/oas-to-har/src/index.js

Line 198 in 59a5d06

har.postData.text = querystring.stringify(formData.formData);

I narrowed down the issue to here:
https://github.com/Kong/httpsnippet/blob/633b825364ab050f02556cfa837148a1e136fd69/src/index.js#L126-L128
httpsnippet is setting it from the value we set back to ""

I'm not sure if this is something that's changed recently in the way
we're generating hars? This part of httpsnippet hasn't been updated
in 5 years. Gunna defer to Jon for this.

I added two test cases: one for json and one for formData. The json one passes fine! This code in oas-to-har is working as expected: https://github.com/readmeio/api-explorer/blob/59a5d060997321936ae356f2ba3f36e34c81c520/packages/oas-to-har/src/index.js#L198 I narrowed down the issue to here: https://github.com/Kong/httpsnippet/blob/633b825364ab050f02556cfa837148a1e136fd69/src/index.js#L126-L128 httpsnippet is setting it from the value we set back to "" I'm not sure if this is something that's changed recently in the way we're generating hars? This part of httpsnippet hasn't been updated in 5 years. Gunna defer to Jon for this.

erunion · 2020-06-26T23:01:52Z

What's the issue? I made some changes to HAR generation in the SDK work to pass Content-Type headers through so JSON objects are dumped in a snippets as objects, not stringified.

domharrington · 2020-06-26T23:39:52Z

Sorry I should've been clearer. Form data values in the form are not coming through into the code sample!

erunion · 2020-06-30T19:53:51Z

Tests will pass when readmeio/fetch-har#67 is pulled into a new fetch-har release.

erunion · 2020-06-30T22:10:46Z

packages/oas-to-har/__tests__/index.test.js

@@ -1,4 +1,4 @@
-const querystring = require('querystring');
+const validate = require('har-validator');


FYI, most of the changes to this file were to rewrite some tests into using it.each to make them easier to maintain.

domharrington

Code lgtm! Any idea what caused the regression?

domharrington · 2020-06-30T22:12:52Z

packages/oas-to-har/__tests__/index.test.js

-test('should output a har format', () => {
-  expect(oasToHar(oas)).toStrictEqual({
+expect.extend({
+  toBeAValidHAR(har) {


I'm tempted to rip this out into its own package.

domharrington · 2020-06-30T22:13:17Z

packages/oas-to-har/__tests__/index.test.js

-    it('should use example if no value', () => {
-      expect(
-        oasToHar(oas, {
+    it.each([


domharrington · 2020-06-30T22:15:40Z

packages/oas-to-har/__tests__/index.test.js

@@ -1157,7 +1048,7 @@ describe('requestBody', () => {
            },
          },
        }).log.entries[0].request.postData.text
-      ).toBe(querystring.stringify({ a: 'value' }));
+      ).toBe(JSON.stringify({ a: 'value' }));


Is this correct? How come it's change from querystring to JSON?

We were only ever using the querystring module for compiling postData.text for formData, but since aren't using that for that anymore we no longer need to pull that module in. Since the other cases within the library that are compiling postData.text use JSON.stringify() I moved this instance over as well.

That said, this test is still not quite right because it's looking for postData.text for a formData case, but since the test is skipped right now I'm not going to worry about it right now.

erunion · 2020-06-30T22:23:32Z

@domharrington Yeah it seems like it was a result of me adding setting postData.mimeType to application/x-www-form-urlencoded with the node-simple work in https://github.com/readmeio/api-explorer/pull/792/files#diff-528f99451eda35186e98d5f772a60fd1R182, and we didn't have sufficient tests for formData anywhere else.

…eing handled properly (#807) * test: added failing test case for formData issue I added two test cases: one for json and one for formData. The json one passes fine! This code in oas-to-har is working as expected: https://github.com/readmeio/api-explorer/blob/59a5d060997321936ae356f2ba3f36e34c81c520/packages/oas-to-har/src/index.js#L198 I narrowed down the issue to here: https://github.com/Kong/httpsnippet/blob/633b825364ab050f02556cfa837148a1e136fd69/src/index.js#L126-L128 httpsnippet is setting it from the value we set back to "" I'm not sure if this is something that's changed recently in the way we're generating hars? This part of httpsnippet hasn't been updated in 5 years. Gunna defer to Jon for this. * fix: correctly preparing `x-www-form-urlencoded` forms as params * fix: oas-to-har will now output valid HAR definitions * chore(deps): upgrading fetch-har to 3.0.0 * docs: adding an example for our handling of formData Co-authored-by: Jon Ursenbach <jon@ursenba.ch>

* fix: issue where application/x-www-form-urlencoded payloads weren't being handled properly (#807) * test: added failing test case for formData issue I added two test cases: one for json and one for formData. The json one passes fine! This code in oas-to-har is working as expected: https://github.com/readmeio/api-explorer/blob/59a5d060997321936ae356f2ba3f36e34c81c520/packages/oas-to-har/src/index.js#L198 I narrowed down the issue to here: https://github.com/Kong/httpsnippet/blob/633b825364ab050f02556cfa837148a1e136fd69/src/index.js#L126-L128 httpsnippet is setting it from the value we set back to "" I'm not sure if this is something that's changed recently in the way we're generating hars? This part of httpsnippet hasn't been updated in 5 years. Gunna defer to Jon for this. * fix: correctly preparing `x-www-form-urlencoded` forms as params * fix: oas-to-har will now output valid HAR definitions * chore(deps): upgrading fetch-har to 3.0.0 * docs: adding an example for our handling of formData Co-authored-by: Jon Ursenbach <jon@ursenba.ch> * build: updating dist files * v6.12.1 Co-authored-by: domharrington <domharrington@users.noreply.github.com>

domharrington assigned erunion Jun 26, 2020

erunion added area:explorer type:bug Something isn't working labels Jun 29, 2020

erunion added 2 commits June 29, 2020 15:29

fix: correctly preparing x-www-form-urlencoded forms as params

88647e8

fix: oas-to-har will now output valid HAR definitions

598c118

erunion changed the title ~~test: added failing test case for formData issue~~ fix: issue where application/x-www-form-urlencoded payloads weren't being handled properly Jun 30, 2020

erunion added the type:refactor Issues about tackling technical debt label Jun 30, 2020

erunion mentioned this pull request Jun 30, 2020

fix: improper handling of application/x-www-form-urlencoded readmeio/fetch-har#67

Merged

erunion added 2 commits June 30, 2020 14:43

chore(deps): upgrading fetch-har to 3.0.0

1725d2a

docs: adding an example for our handling of formData

b79f1ec

erunion marked this pull request as ready for review June 30, 2020 22:05

erunion requested a review from kanadgupta June 30, 2020 22:06

erunion reviewed Jun 30, 2020

View reviewed changes

domharrington commented Jun 30, 2020

View reviewed changes

erunion merged commit c08ca55 into master Jun 30, 2020

erunion deleted the bug/formdata-code-samples branch June 30, 2020 23:39

erunion mentioned this pull request Jul 1, 2020

test: loading in our new decoupled jest-expect-har package #809

Merged

3 tasks

erunion mentioned this pull request Jul 1, 2020

hotfix: application/x-www-form-urlencoded bug #818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: issue where application/x-www-form-urlencoded payloads weren't being handled properly #807

fix: issue where application/x-www-form-urlencoded payloads weren't being handled properly #807

domharrington commented Jun 26, 2020 •

edited by erunion

Loading

erunion commented Jun 26, 2020

domharrington commented Jun 26, 2020

erunion commented Jun 30, 2020

erunion Jun 30, 2020

domharrington left a comment

domharrington Jun 30, 2020

erunion Jun 30, 2020

domharrington Jun 30, 2020

domharrington Jun 30, 2020

erunion Jun 30, 2020 •

edited

Loading

erunion commented Jun 30, 2020

		@@ -1,4 +1,4 @@
		const querystring = require('querystring');
		const validate = require('har-validator');

fix: issue where application/x-www-form-urlencoded payloads weren't being handled properly #807

fix: issue where application/x-www-form-urlencoded payloads weren't being handled properly #807

Conversation

domharrington commented Jun 26, 2020 • edited by erunion Loading

🧰 What's being changed?

🧪 Testing

🗳 Checklist

erunion commented Jun 26, 2020

domharrington commented Jun 26, 2020

erunion commented Jun 30, 2020

erunion Jun 30, 2020

Choose a reason for hiding this comment

domharrington left a comment

Choose a reason for hiding this comment

domharrington Jun 30, 2020

Choose a reason for hiding this comment

erunion Jun 30, 2020

Choose a reason for hiding this comment

domharrington Jun 30, 2020

Choose a reason for hiding this comment

domharrington Jun 30, 2020

Choose a reason for hiding this comment

erunion Jun 30, 2020 • edited Loading

Choose a reason for hiding this comment

erunion commented Jun 30, 2020

domharrington commented Jun 26, 2020 •

edited by erunion

Loading

erunion Jun 30, 2020 •

edited

Loading