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: multipart/form-data body interpolation #3142

Merged

Conversation

lohxt1
Copy link
Collaborator

@lohxt1 lohxt1 commented Sep 19, 2024

changes:

~ the formData body will be available in json format during the pre-request script (req.getBody()).
json body is interpolated after the pre-request step, and the formData body is then constructed from the json data.

Screenshot 2024-09-19 at 12 37 05 PM

~ if a constructed formData body is directly set in the pre-request script, the interpolation and the construction of formData body are skipped.

Screenshot 2024-09-19 at 12 31 24 PM

@helloanoop helloanoop marked this pull request as ready for review September 20, 2024 11:14
@helloanoop
Copy link
Contributor

@Its-treason Can you review this if you have some time over the weekend?
The implementation looks good to me.
We are fine to introduce the breaking change where req.getBody() returns an object instead of FormData

Copy link
Member

@Its-treason Its-treason left a comment

Choose a reason for hiding this comment

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

I added some notes. But feature itself seems good.

@@ -195,6 +181,14 @@ const runSingleRequest = async function (
request.data = qs.stringify(request.data);
}

if (request?.headers?.['content-type'] === 'multipart/form-data') {
Copy link
Member

Choose a reason for hiding this comment

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

These two if-statements can be merged.

// reference: https://github.com/axios/axios/issues/1006#issuecomment-320165427
const form = new FormData();
forOwn(datas, (value, key) => {
if (typeof value == 'object') {
Copy link
Member

Choose a reason for hiding this comment

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

I would personally invert the if to: typeof value === 'string' and return after form.append like this:

if (typeof data === 'string') {
  form.append(key, value);
  return;
}

// Code for file here.

To make the code more readable

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

makes sense, will update the logic

@@ -15,7 +15,7 @@
"bypassProxy": ""
},
"scripts": {
"moduleWhitelist": ["crypto", "buffer"],
"moduleWhitelist": ["crypto", "buffer", "form-data"],
Copy link
Member

Choose a reason for hiding this comment

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

We could add form-data to the default libraries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

since form-data dep needs to be added to both vm2 and quickjs, i'l create a seperate pr for just that

@helloanoop helloanoop merged commit ed20ecc into usebruno:main Sep 23, 2024
2 checks passed
@helloanoop
Copy link
Contributor

Merged. Thanks @lohxt1 for working on this!
Thank you @Its-treason for your review!

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

Successfully merging this pull request may close these issues.

3 participants