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

[fix] various fixes related to weird input data #112

Merged
merged 6 commits into from
Oct 5, 2023

Conversation

allansson
Copy link
Contributor

This PR contains a number of smaller fixes, making the converter more tolerant to deviations from the HAR specification.

  • [fix] param not discarded when name is empty
  • [fix] query items with a null name throws validation error
  • [fix] validation error thrown when GET request has postData

It also adds a small feature where the converter will try to infer the mime type when it is missing, by looking at the rest of the postData content.

Also updated a bunch of test names to make them more descriptive and removed some unused code.

Copy link
Contributor Author

@allansson allansson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a self-review.


if (mimeType === '') {
console.warn(
`[WARN] Post data MIME type is missing and will be inferred from the content (${i})`
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 still warn about this even though I added the inference. I figure that the user might want to check what the converter inferred, to make sure that it was inferred correctly.

@@ -7,11 +7,11 @@ const { createQueryStringIndexes } = require('./utils/indexes')
/*
* [j]: object
*/
function queryString(node, i, assay) {
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 took the opportunity to clean some of the unused parameter up.

}),
`Request postData usage with GET requests is prohibited`
)
console.warn(`[WARN] GET request has postData object (${i})`)
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 think it's pretty safe to simply ignore the postData when it's a GET-request. It is obviously an error in the thing that generated the HAR file.

const spec = makeSpec()
param({ name: 'search' }, spec)
t.deepEqual(spec, new Map().set('search', new Set([{}])))
})

test('value', (t) => {
test('it should set the value of params when a value is given', t => {
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 updated a bunch of these test names, just to make them more descriptive. Didn't have the patience to update them everywhere I made changes though.

@e-fisher e-fisher requested review from a team, going-confetti and 2Steaks and removed request for a team December 2, 2022 08:12
Copy link

@2Steaks 2Steaks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

Copy link

@going-confetti going-confetti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@legander legander requested a review from w1kman June 16, 2023 09:19
Copy link
Contributor

@w1kman w1kman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌🏻


// We only assume it to be JSON if it's an object or array, because
// any other value might just as well be arbitrary text.
if (typeof body === 'object' && body !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd let null be application/json

@tmc
Copy link

tmc commented Sep 8, 2023

any reason this hasn't merged?

@allansson allansson merged commit ae58d58 into master Oct 5, 2023
@allansson allansson deleted the fix/various-fixes-related-to-weird-input-data branch October 5, 2023 15:16
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.

None yet

5 participants