Do not set content-type on body-less requests #1826

goce-cz · 2024-08-07T10:58:25Z

Changes

Resolves #1694

Important: This PR touches the same code as #1825 and in-fact includes the same fix by itself. I recommend merging #1825 first.

This PR prevents Content-Type header from being set on requests with no body. This is required to ensure compatibility with strict server impls. such as fastify.

How to Review

Consider checking the included tests and docs to understand the desired behavior.
The change in the implementation is rather simple.

Checklist

Unit tests updated
docs/ updated (if necessary)
pnpm run update:examples run (only applicable for openapi-typescript)

changeset-bot · 2024-08-07T10:58:28Z

🦋 Changeset detected

Latest commit: 1812aff

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

This PR includes changesets to release 2 packages

Name	Type
openapi-fetch	Minor
openapi-react-query	Major

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

goce-cz · 2024-08-07T11:01:48Z

docs/openapi-fetch/api.md

+or when instantiating the client. Setting `Content-Type` to `null` will omit the header, however the native fetch API
+will likely set it to `text/plain` in this case.


Not sure we really need to mention the null thing anymore, when this behaves properly now. 🤷‍♂️

Opinions?

Yeah just remove it 🔪

BTW Just a thought... I wonder whether allowing the bodySerializer to return {contentType: "...", body: ...} wouldn't be the best way to allow transport customization 🤔

goce-cz · 2024-08-07T11:02:34Z

packages/openapi-fetch/test/index.test.ts

+      const BODIES = [{ prop: "a" }, {}, "", "str", null, false, 0, 1, new Date("2024-08-07T09:52:00.836Z")] as const;
+      const METHOD_BODY_COMBINATIONS = BODY_ACCEPTING_METHODS.flatMap(([method]) =>
+        BODIES.map((body) => [method, body] as const),
+      );


@kerwanp Guessing from #1825 (comment) you won't like this 🤣

Happy to ditch the methods, but I'd rather keep the various bodies as this was broken until #1825.

Aha I'm not against this when behavior have to be different between cases, the pattern you use looks good to me! The methods constants could be moved at the top of the file to be reused in other tests.

Until they are actually reused in other tests, they IMO make more sense when they are collocated with the test itself. It's just less scrolling to understand how they are used. Anyway this is just my opinion I've grown over the years, nothing I would fight for.

I guess we are in agreement that this file already does too much and deserves some larger refactoring and splitting. Let's maybe open a separate issue and discuss a bit what the desired structure could look like?

kerwanp

Great work! Same thing as the other pull request, I'll let @drwpow review this

kerwanp · 2024-08-09T23:58:08Z

packages/openapi-fetch/test/index.test.ts

+          // the fetch implementation won't allow sending a body without content-type,
+          // so it invents one up and sends it, hopefully this will be consistent across
+          // local environments and won't make the tests flaky
+          expect(contentType).toBe("text/plain;charset=UTF-8");


Yeah this one is a bit scary, it's a potential IWOMM scenario.
It seems the test ensure that the default content-type is basically removed?

Maybe we can simply do something like this?

Suggested change

expect(contentType).toBe("text/plain;charset=UTF-8");

expect(contentType).not.toBe("application/json");

Yeah, that sounds better. Good idea.

kerwanp · 2024-08-10T00:02:47Z

packages/openapi-fetch/test/index.test.ts

+      const BODIES = [{ prop: "a" }, {}, "", "str", null, false, 0, 1, new Date("2024-08-07T09:52:00.836Z")] as const;
+      const METHOD_BODY_COMBINATIONS = BODY_ACCEPTING_METHODS.flatMap(([method]) =>
+        BODIES.map((body) => [method, body] as const),
+      );


Aha I'm not against this when behavior have to be different between cases, the pattern you use looks good to me! The methods constants could be moved at the top of the file to be reused in other tests.

kerwanp · 2024-08-10T00:04:31Z

packages/openapi-fetch/test/index.test.ts

@@ -859,6 +855,138 @@ describe("client", () => {
      });
    });

+    describe("content-type", () => {
+      const BODY_ACCEPTING_METHODS = [["PUT"], ["POST"], ["DELETE"], ["OPTIONS"], ["PATCH"]] as const;


nit: Is there a reason to have two dims arrays here?

This is what the each method officially expects. It's a list of param sets where each set can consist of multiple params. The API supports a flat array, where [x] is treated as [[x]], but I don't really fancy these magical interfaces and prefer to be explicit. Imagine what happens when x is actually an array?

drwpow · 2024-08-12T05:02:13Z

See Martin’s comments/questions on tests. The implementation seems to solve the original issue. But in order to approve/merge we’ll need a patch changeset (see comment). Thanks for contributing!

goce-cz · 2024-08-14T11:38:04Z

But in order to approve/merge we’ll need a patch changeset (see comment).

I'll address this once I'm back from my vacation. Thanks!

goce-cz · 2024-08-28T08:43:55Z

@drwpow should this really be a patch release? IIRC, in the issue, you mentioned that you would release it as a breaking change, just to be sure.

drwpow · 2024-08-29T17:31:23Z

@drwpow should this really be a patch release? IIRC, in the issue, you mentioned that you would release it as a breaking change, just to be sure.

So I did 😄! Thank you, yes this should be a minor (“breaking”) release.

goce-cz · 2024-08-30T07:58:27Z

minor (“breaking”)

Unfamiliar combination 😄 , but here you go.

drwpow · 2024-08-30T13:46:56Z

minor (“breaking”)

Unfamiliar combination 😄 , but here you go.

Thanks! This is actually following standard semver. Pre-1.0, the minor almost always signals breaking changes. Technically you can choose to either put features in minor releases or patches, but I choose the latter so a minor predictably means some breaking change.

goce-cz · 2024-08-30T14:08:23Z

Pre-1.0, the minor almost always signals breaking changes.

This didn't make sense to me, until I stopped looking at (and releasing 🤦 ) a wrong package. (openapi-typescript v7).

Rebased and hopefully ready for merging.

drwpow

Great addition and tests, thank you!

drwpow · 2024-08-30T16:19:27Z

docs/openapi-fetch/api.md

+`Content-Type` is omitted, allowing the browser to set it automatically with the correct message part boundary.
+
+You can also set `Content-Type` manually through `headers` object either in the fetch options,
+or when instantiating the client.


I forget if I said this already, but thanks for adding this! Little docs improvements go a long way and are always welcome ❤️

bkis · 2024-09-20T12:57:46Z

The documentation states

For convenience, openapi-fetch sets Content-Type to application/json automatically for any request that provides value for the body parameter. When the bodySerializer returns an instance of FormData, Content-Type is omitted, allowing the browser to set it automatically with the correct message part boundary.

You can also set Content-Type manually through headers object either in the fetch options, or when instantiating the client.

It seems like I actually can't set the header manually anymore. I have a fetch options object

{
    headers: {
      'Content-Type': 'application/x-www-form-urlencoded',
    },
    bodySerializer: (body: Record<string, string | number | boolean | undefined | null>) => {
      return new URLSearchParams(
        Object.entries(body).map(([key, value]) => [String(key), String(value)])
      ).toString();
    },
  }

...but the Content-Type header is omitted. This happens since these changes were made. When returning a FormData object in the bodySerializer, the API I target doesn't understand the request.
It does work with the above options including the header, though. How can I force this header to be set?

EDIT: Weirdly, this seems to happen since 0.9.x. Sorry – it may not be because of this PR as it was merged and released with 0.12.0, right?

goce-cz requested a review from a team as a code owner August 7, 2024 10:58

goce-cz commented Aug 7, 2024

View reviewed changes

kerwanp reviewed Aug 10, 2024

View reviewed changes

kerwanp added the openapi-fetch Relevant to the openapi-fetch library label Aug 10, 2024

goce-cz force-pushed the 1694-do-not-set-content-type-on-undefined-body branch from e2779cd to 502cc70 Compare August 30, 2024 07:57

goce-cz added 5 commits August 30, 2024 15:50

Do not set content-type on body-less requests

80e2b93

Add docs

880b288

Remove commented code

7e78435

Implement CR suggestions

15e89bf

Add change-set

fdeecfe

goce-cz force-pushed the 1694-do-not-set-content-type-on-undefined-body branch from 502cc70 to fdeecfe Compare August 30, 2024 13:50

goce-cz added 2 commits August 30, 2024 15:59

Fix change-set

ef8a69e

Post-rebase fix

1812aff

drwpow approved these changes Aug 30, 2024

View reviewed changes

drwpow merged commit b893c44 into openapi-ts:main Aug 30, 2024
8 checks passed

github-actions bot mentioned this pull request Aug 30, 2024

[ci] release #1891

Merged

goce-cz deleted the 1694-do-not-set-content-type-on-undefined-body branch August 30, 2024 19:14

organich mentioned this pull request Sep 20, 2024

[Snyk] Upgrade openapi-fetch from 0.11.3 to 0.12.0 organich/openverse#4

Open

gpichot mentioned this pull request Oct 22, 2024

Content-Type of type multipart must include a boundary parameter #1956

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not set content-type on body-less requests #1826

Do not set content-type on body-less requests #1826

goce-cz commented Aug 7, 2024

changeset-bot bot commented Aug 7, 2024 •

edited

Loading

goce-cz Aug 7, 2024

drwpow Aug 10, 2024

goce-cz Aug 28, 2024

goce-cz Aug 7, 2024

kerwanp Aug 10, 2024

goce-cz Aug 14, 2024

kerwanp left a comment

kerwanp Aug 9, 2024

goce-cz Aug 14, 2024

kerwanp Aug 10, 2024

kerwanp Aug 10, 2024

goce-cz Aug 14, 2024

drwpow commented Aug 12, 2024

goce-cz commented Aug 14, 2024

goce-cz commented Aug 28, 2024

drwpow commented Aug 29, 2024

goce-cz commented Aug 30, 2024

drwpow commented Aug 30, 2024

goce-cz commented Aug 30, 2024

drwpow left a comment

drwpow Aug 30, 2024

bkis commented Sep 20, 2024 •

edited

Loading

		or when instantiating the client. Setting `Content-Type` to `null` will omit the header, however the native fetch API
		will likely set it to `text/plain` in this case.

	expect(contentType).toBe("text/plain;charset=UTF-8");
	expect(contentType).not.toBe("application/json");

Do not set content-type on body-less requests #1826

Do not set content-type on body-less requests #1826

Conversation

goce-cz commented Aug 7, 2024

Changes

How to Review

Checklist

changeset-bot bot commented Aug 7, 2024 • edited Loading

🦋 Changeset detected

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerwanp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow commented Aug 12, 2024

goce-cz commented Aug 14, 2024

goce-cz commented Aug 28, 2024

drwpow commented Aug 29, 2024

goce-cz commented Aug 30, 2024

drwpow commented Aug 30, 2024

goce-cz commented Aug 30, 2024

drwpow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkis commented Sep 20, 2024 • edited Loading

changeset-bot bot commented Aug 7, 2024 •

edited

Loading

bkis commented Sep 20, 2024 •

edited

Loading