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

Delete Content-Type header for multipart requests #211

Conversation

gydroperit
Copy link
Contributor

@gydroperit gydroperit commented Nov 28, 2019

We need to delete Content-Type header every time we create new instance of Request to properly set boundary in headers and in body.
Fixes #209

@gydroperit gydroperit force-pushed the remove-content-type-for-multipart-request branch 3 times, most recently from 3ea45cc to 1de8453 Compare November 28, 2019 12:41
@sindresorhus
Copy link
Owner

// @rangermeier

@sindresorhus sindresorhus changed the title Delete Content-Type header for multipart requests. Delete Content-Type header for multipart requests Dec 16, 2019
@rangermeier
Copy link
Contributor

Not sure if it's a good idea to always remove Content-Type header for multipart requests. As discussed in #204 (comment) there might be need for setting the content-type when sending multipart.

I think it would be safer to move the new code into its preceding if() statement:

		if (this._options.searchParams) {
			const url = new URL(this.request.url);
			url.search = new URLSearchParams(this._options.searchParams);

			if (((supportsFormData && this._options.body instanceof globals.FormData) || this._options.body instanceof URLSearchParams) && this.request.headers.has('content-type')) {
				this.request.headers.delete('content-type');
			}

			this.request = new globals.Request(new globals.Request(url, this.request), this._options);
		}

@sholladay
Copy link
Collaborator

I think it would be safer to move the new code into its preceding if() statement:

Agreed. At the end of the day, this is a hack, and thus the scope of its impact should be reduced as much as possible so it only takes effect when really necessary. I also think it's worth leaving a code comment here describing why it's being done, perhaps with a link to the original problem.

@sindresorhus
Copy link
Owner

@gydroperit Ping :)

@gydroperit gydroperit force-pushed the remove-content-type-for-multipart-request branch 2 times, most recently from 420e700 to f098c33 Compare February 21, 2020 12:21
@sindresorhus
Copy link
Owner

Tests are failing

@gydroperit gydroperit force-pushed the remove-content-type-for-multipart-request branch from 3092555 to 671a521 Compare February 22, 2020 11:31
@gydroperit
Copy link
Contributor Author

gydroperit commented Feb 22, 2020

Added test cases for request with FormData and searchParams and custom content-type header.
But this

test.failing('FormData with searchParams ("multipart/form-data" parser)', withPage, async (t, page) => {
test case fails because of some strange puppeteer error

 browser  FormData with searchParams ("multipart/form-data" parser)

  node_modules/puppeteer/lib/Connection.js:74

  Rejected promise returned by test. Reason:

  Error {
    message: 'Protocol error (Target.closeTarget): Target closed.',
  }

  node_modules/puppeteer/lib/Connection.js:74:56
  Connection.send (node_modules/puppeteer/lib/Connection.js:73:12)
  Page.close (node_modules/puppeteer/lib/Page.js:1040:38)
  Page.<anonymous> (node_modules/puppeteer/lib/helper.js:112:23)
  withPage `(test/helpers/with-page.js:9:14)`

Can we just delete it?
Also, I added condition for case when content-type is set manual

@sindresorhus
Copy link
Owner

test case fails because of some strange puppeteer error

Try to do some debugging. You could start by Googling the error.

@gydroperit gydroperit force-pushed the remove-content-type-for-multipart-request branch from 671a521 to de75793 Compare February 22, 2020 12:23
@gydroperit
Copy link
Contributor Author

Did it

test/browser.js Show resolved Hide resolved
test/browser.js Show resolved Hide resolved
test/browser.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@gydroperit gydroperit force-pushed the remove-content-type-for-multipart-request branch from de75793 to edd9a51 Compare February 24, 2020 16:27
@gydroperit
Copy link
Contributor Author

Ping

Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

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

LGTM

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.

Multipart boundary mismatch when using search params
4 participants