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(openapi-fetch): fix headers for FormData #1192

Merged

Conversation

psychedelicious
Copy link
Contributor

@psychedelicious psychedelicious commented Jul 1, 2023

We need to delete the Content-Type header if the serialized body is a FormData. This allows the browser to set the Content-Type and boundary expression correctly.

Resolves #1191

Changes

If you set Content-Type to multipart/form-data explicitly, the browser does not set boundary.

Because FormData gets special handling by the browser, we need to give it special handling too.

Check if the serialized body is an instance of FormData, and if so, delete the Content-Type header.

Resolves #1191

How to Review

I've tested this in my production app but I think you'd need to whip up a simple test project to see for yourself. I'm out of time for that today though, sorry!

One hiccup in the tests - vitest-fetch-mock does not add the form field boundary like the browser does. Ideally we could confirm that the Content-Type for FormData bodies are correct:

// expect requests with FormData to have correct Content-Type with boundary expression
expect((req.headers as Headers).get("Content-Type")).toMatch(/^multipart\/form-data; boundary=.*/);

Instead, we must check if it is null:

// `vitest-fetch-mock` does not add the boundary to the Content-Type header like browsers do, so we expect the header to be null
expect((req.headers as Headers).get("Content-Type")).toBeNull();

edit: Oh, maybe it's just that the browser adds the boundary as the request is processed, and testing for null is correct

@nilsso Please give try this out.

Checklist

  • Unit tests updated
  • README updated
  • [ ] examples/ directory updated (only applicable for openapi-typescript)

@changeset-bot
Copy link

changeset-bot bot commented Jul 1, 2023

🦋 Changeset detected

Latest commit: ff5c633

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openapi-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

We need to delete the `Content-Type` header if the serialized body is a `FormData`. This allows the browser to set the `Content-Type` and boundary expression correctly.

Resolves openapi-ts#1191
@psychedelicious psychedelicious force-pushed the fix/openapi-fetch/fix-headers-formdata branch from 3ef7b99 to 4572aeb Compare July 1, 2023 04:58
@drwpow
Copy link
Contributor

drwpow commented Jul 2, 2023

Thanks for opening this PR! I’m in favor of merging this PR. Just had a minor nitpick on one unneeded change before approving and merging though. Otherwise looks great

`baseHeaders` is still the same object via reference, we can just access it directly.
@drwpow drwpow merged commit 38ee8b4 into openapi-ts:main Jul 3, 2023
@github-actions github-actions bot mentioned this pull request Jul 3, 2023
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/form-data does not allow browser to set boundary expression
2 participants