Skip to content

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

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

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/chilled-forks-rush.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"openapi-fetch": minor
---

Do not set content-type on body-less requests
11 changes: 11 additions & 0 deletions docs/openapi-fetch/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,17 @@ const { data, error } = await client.PUT("/submit", {
});
```

::: tip

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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


:::

## Path serialization

openapi-fetch supports path serialization as [outlined in the 3.1 spec](https://swagger.io/docs/specification/serialization/#path). This happens automatically, based on the specific format in your OpenAPI schema:
Expand Down
28 changes: 15 additions & 13 deletions packages/openapi-fetch/src/index.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
// settings & const
const DEFAULT_HEADERS = {
"Content-Type": "application/json",
};

const PATH_PARAM_RE = /\{[^{}]+\}/g;

/** Add custom parameters to Request object */
Expand Down Expand Up @@ -41,7 +37,6 @@ export default function createClient(clientOptions) {
...baseOptions
} = { ...clientOptions };
baseUrl = removeTrailingSlash(baseUrl);
baseHeaders = mergeHeaders(DEFAULT_HEADERS, baseHeaders);
const middlewares = [];

/**
Expand All @@ -58,6 +53,7 @@ export default function createClient(clientOptions) {
parseAs = "json",
querySerializer: requestQuerySerializer,
bodySerializer = globalBodySerializer ?? defaultBodySerializer,
body,
...init
} = fetchOptions || {};
if (localBaseUrl) {
Expand All @@ -78,19 +74,25 @@ export default function createClient(clientOptions) {
});
}

const serializedBody = body === undefined ? undefined : bodySerializer(body);

const defaultHeaders =
// with no body, we should not to set Content-Type
serializedBody === undefined ||
// if serialized body is FormData; browser will correctly set Content-Type & boundary expression
serializedBody instanceof FormData
? {}
: {
"Content-Type": "application/json",
};

const requestInit = {
redirect: "follow",
...baseOptions,
...init,
headers: mergeHeaders(baseHeaders, headers, params.header),
body: serializedBody,
headers: mergeHeaders(defaultHeaders, baseHeaders, headers, params.header),
};
if (requestInit.body !== undefined) {
requestInit.body = bodySerializer(requestInit.body);
// remove `Content-Type` if serialized body is FormData; browser will correctly set Content-Type & boundary expression
if (requestInit.body instanceof FormData) {
requestInit.headers.delete("Content-Type");
}
}

let id;
let options;
Expand Down
144 changes: 136 additions & 8 deletions packages/openapi-fetch/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@ import createClient, {
type BodySerializer,
type FetchOptions,
type MethodResponse,
type HeadersOptions,
type Middleware,
type MiddlewareCallbackParams,
type QuerySerializerOptions,
type Client,
type PathBasedClient,
createPathBasedClient,
} from "../src/index.js";
import { server, baseUrl, useMockRequestHandler, toAbsoluteURL } from "./fixtures/mock-server.js";
import { baseUrl, server, toAbsoluteURL, useMockRequestHandler } from "./fixtures/mock-server.js";
import type { paths } from "./fixtures/api.js";

beforeAll(() => {
Expand Down Expand Up @@ -819,12 +820,7 @@ describe("client", () => {
await client.GET("/self");

// assert default headers were passed
expect(getRequest().headers).toEqual(
new Headers({
...headers, // assert new header got passed
"Content-Type": "application/json", // probably doesn’t need to get tested, but this was simpler than writing lots of code to ignore these
}),
);
expect(getRequest().headers).toEqual(new Headers(headers));
});

it("can be overridden", async () => {
Expand All @@ -850,7 +846,6 @@ describe("client", () => {
expect(getRequest().headers).toEqual(
new Headers({
"Cache-Control": "no-cache",
"Content-Type": "application/json",
}),
);
});
Expand Down Expand Up @@ -894,6 +889,139 @@ describe("client", () => {
});
});

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

const ALL_METHODS = [...BODY_ACCEPTING_METHODS, ["GET"], ["HEAD"]] as const;

const fireRequestAndGetContentType = async (options: {
defaultHeaders?: HeadersOptions;
method: (typeof ALL_METHODS)[number][number];
fetchOptions: FetchOptions<any>;
}) => {
const client = createClient<any>({ baseUrl, headers: options.defaultHeaders });
const { getRequest } = useMockRequestHandler({
baseUrl,
method: "all",
path: "/blogposts-optional",
status: 200,
});
await client[options.method]("/blogposts-optional", options.fetchOptions as any);

const request = getRequest();
return request.headers.get("content-type");
};

it.each(ALL_METHODS)("no content-type for body-less requests - %s", async (method) => {
const contentType = await fireRequestAndGetContentType({
method,
fetchOptions: {},
});

expect(contentType).toBe(null);
});

it.each(ALL_METHODS)("no content-type for `undefined` body requests - %s", async (method) => {
const contentType = await fireRequestAndGetContentType({
method,
fetchOptions: {
body: undefined,
},
});

expect(contentType).toBe(null);
});

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),
);
Comment on lines +934 to +937
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?


it.each(METHOD_BODY_COMBINATIONS)(
"implicit default content-type for body-full requests - %s, %j",
async (method, body) => {
const contentType = await fireRequestAndGetContentType({
method,
fetchOptions: {
body,
},
});

expect(contentType).toBe("application/json");
},
);

it.each(METHOD_BODY_COMBINATIONS)(
"provided default content-type for body-full requests - %s, %j",
async (method, body) => {
const contentType = await fireRequestAndGetContentType({
defaultHeaders: {
"content-type": "application/my-json",
},
method,
fetchOptions: {
body,
},
});

expect(contentType).toBe("application/my-json");
},
);

it.each(METHOD_BODY_COMBINATIONS)(
"native-fetch default content-type for body-full requests, when default is suppressed - %s, %j",
async (method, body) => {
const contentType = await fireRequestAndGetContentType({
defaultHeaders: {
"content-type": null,
},
method,
fetchOptions: {
body,
},
});
// the fetch implementation won't allow sending a body without content-type,
// and it defaults to `text/plain;charset=UTF-8`, however the actual default value
// is irrelevant and might be flaky across different fetch implementations
// for us, it's important that it's not `application/json`
expect(contentType).not.toBe("application/json");
},
);

it.each(METHOD_BODY_COMBINATIONS)(
"specified content-type for body-full requests - %s, %j",
async (method, body) => {
const contentType = await fireRequestAndGetContentType({
method,
fetchOptions: {
body,
headers: {
"content-type": "application/my-json",
},
},
});

expect(contentType).toBe("application/my-json");
},
);

it.each(METHOD_BODY_COMBINATIONS)(
"specified content-type for body-full requests, even when default is suppressed - %s, %j",
async (method, body) => {
const contentType = await fireRequestAndGetContentType({
method,
fetchOptions: {
body,
headers: {
"content-type": "application/my-json",
},
},
});

expect(contentType).toBe("application/my-json");
},
);
});

describe("fetch", () => {
it("createClient", async () => {
function createCustomFetch(data: any) {
Expand Down