orval icon indicating copy to clipboard operation
orval copied to clipboard

204 No content generated code throws JSON parsing error

Open zZHorizonZz opened this issue 1 year ago • 1 comments

What are the steps to reproduce this issue?

  1. Create new project with fetch client from openapi scheme which returns 204 (No content).
  2. Try calling the generated method.
  3. You should get error Unexpected end of JSON input error.

What happens?

Basically when there is no response in fetch client response there is no content. Which mean the res.json() will throw error.

Generated code looks like this:

const data = await res.json()

return { status: res.status, data }

What were you expecting to happen?

There shouldn't be a error.

Any logs, error output, etc?

SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at parseJSONFromBytes (node:internal/deps/undici/undici:4306:19)
    at successSteps (node:internal/deps/undici/undici:4288:27)
    at consumeBody (node:internal/deps/undici/undici:4294:9)
    at _Response.json (node:internal/deps/undici/undici:4239:18)

Any other comments?

I think when there is 204 in specs or there is no content in openapi specification the const data should not be generated and data should not be required in generated responses as there are none.

What versions are you using?

  System:
    OS: Linux 6.9 Pop!_OS 22.04 LTS
    CPU: (24) x64 AMD Ryzen 9 7900X 12-Core Processor
    Memory: 9.64 GB / 31.08 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  npmPackages:
    orval: ^7.1.1 => 7.1.1 
    react: rc => 19.0.0-rc-0751fac7-20241002 
    zod: 3.23.8 => 3.23.8 

zZHorizonZz avatar Oct 05 '24 14:10 zZHorizonZz

@zZHorizonZz

There is no solution for this yet, but you can work-around it by using a custom fetch like below:

const getBody = async <T>(c: Response | Request): Promise<T> => {
	const text = await c.text();

	if (!text) {
		return {} as T;
	}

	const contentType = c.headers.get("content-type");

	if (contentType?.includes("application/json")) {
		return JSON.parse(text);
	}

	return text as unknown as T;
};

export const customFetch = async <T>(
	url: string,
	options: RequestInit,
): Promise<T> => {
		const response = await fetch();
		const data = await getBody<T>(response);

		return { status: response.status, data } as T;
};

soartec-lab avatar Oct 13 '24 00:10 soartec-lab

Hello,

I'm facing the same issue. My opennapi.json says 204 (No Content), but orval doesn't take this into account when using fetch (and it incorrectly calls a .json()), the generated code looks like this:

export const getDeleteAllDoneTasksUrl = () => {
  return `/api/tasks/done`
}

export const deleteAllDoneTasks = async ( options?: RequestInit): Promise<deleteAllDoneTasksResponse> => {
  
  const res = await fetch(getDeleteAllDoneTasksUrl(),
  {      
    ...options,
    method: 'DELETE'
  }
  )
  // BUG: Throws an exception if the response contains no content (body):
  const data = waiting for res.json()
  return { status: res.status, data }
}

I tried the fix from https://github.com/orval-labs/orval/issues/1656#issuecomment-2408768709 as it is and registered it in orval.config.js:

..
override: {
        mutator: {
          path: './custom-fetch.ts',
          name: 'customFetch',
        },
..

However, it doesn't work for me as it breaks other APIs (which require a json()-call) - I don't know if this is related to https://github.com/orval-labs/orval/pull/1632.

nimo23 avatar Nov 07 '24 15:11 nimo23

@nimo23

Among the automatically generated custom hooks, custom fetch is called. Are your settings correct?

https://orval.dev/guides/custom-client

soartec-lab avatar Nov 08 '24 00:11 soartec-lab

Among the automatically generated custom hooks, custom fetch is called. Are your settings correct?

Yes, the custom fetch will be called.

nimo23 avatar Nov 08 '24 08:11 nimo23

@nimo23 If configured correctly, it can be customized. Where exactly does the error occur?

soartec-lab avatar Nov 08 '24 14:11 soartec-lab

@soartec-lab thanks, With the help of https://github.com/orval-labs/orval/blob/master/samples/next-app-with-fetch/custom-fetch.ts I get it working.

nimo23 avatar Nov 10 '24 02:11 nimo23

@nimo23 it's good. I'll close this issue and reopen it if you need it again.

soartec-lab avatar Nov 16 '24 02:11 soartec-lab

@soartec-lab Since orval should aim to adhere to the definitions set in openapi.yaml, it should fix this issue automatically (without requiring user intervention). I can imagine that others will also have this problem in the future.

The simple fact is that something like 204 No Content doesn't have a content response per definition, so by default Orval shouldn't try to retrieve a content (via json()). Because this inevitably leads to errors. It would be better to fix this bug. Don't you think so?

nimo23 avatar Nov 16 '24 10:11 nimo23

@nimo23

I agree, I think it's good to support not content. However, there are many things to consider before and after `fetch', and we wanted to make the automatically generated functions as simple as possible. If we can accommodate this while maintaining simplicity, I'm all for it.

soartec-lab avatar Nov 16 '24 13:11 soartec-lab

The main problem is that attempting to call .json() throws an error when there is no content in the response. Instead of throwing an error, I think the generated code should handle this gracefully, when there is no content in the response (whether it's 204 No Content or any other response without a body), the method should return {status: statusCode} without attempting to parse the non-existent body. What do you think @soartec-lab ?

zZHorizonZz avatar Nov 16 '24 15:11 zZHorizonZz

Something like

try{
 const data = await res.json()
}
catch(Error error){
// no content
}

or using something like

// in future: ECMAScript Safe Assignment Operator (?=)
const data ?= await res.json()

nimo23 avatar Nov 18 '24 09:11 nimo23

OK, I'll think about this, so give me some time and this issue be reopened.

soartec-lab avatar Nov 18 '24 13:11 soartec-lab

I have a couple ideas:

1. Check res.body

In this case, you can simply remove the error since it doesn't depend on the status code.

export const listPets = async (
  params?: ListPetsParams,
  options?: RequestInit,
): Promise<listPetsResponse> => {
  const res = await fetch(getListPetsUrl(params), {
    ...options,
    method: 'GET',
  });

-  const data: Pets = await res.json();
+  const data: Pets = res.body ? await res.json() : {};

  return { status: res.status, data, headers: res.headers };
};

2. Check the status code

In this case, you will more strictly follow the REST API specifications. For example, if there is no response even though it is 200, you will still throw an exception.

export const listPets = async (
  params?: ListPetsParams,
  options?: RequestInit,
): Promise<listPetsResponse> => {
  const res = await fetch(getListPetsUrl(params), {
    ...options,
    method: 'GET',
  });

-  const data: Pets = await res.json();
+  const data: Pets = [204, 304].includes(res.status) ? await res.json() : {};

  return { status: res.status, data, headers: res.headers };
};

We will be looking into this further, but do you have any opinions so far?

soartec-lab avatar Dec 27 '24 03:12 soartec-lab

To me personally, it makes sense to only use option with the No Content (204, 304) response when there's actually, well, no content. But APIs aren't always perfect, and I've seen some that return a 200 status with no content. So, I think the first option would be better because it would cover those weird APIs too.

zZHorizonZz avatar Dec 27 '24 09:12 zZHorizonZz

Thank you for your reply. I agree with your opinion. I will wait for more options to gather.

soartec-lab avatar Dec 28 '24 02:12 soartec-lab

I think the 5 possible states should be handled like this:

  1. Present unrequired body (ignore)
  2. Missing required body (accept anyway)
  3. Present but invalid JSON body (error)
  4. Present optional/required body (expected)
  5. Missing optional/unrequired body (expected)

The following status codes should not have bodies according to the spec: 1xx, 204, 205, 304. When these codes are used, the body should always be ignored, even if present (state 1).

As @zZHorizonZz noted, some APIs omit bodies even when they're technically required. This should be accepted (state 2).

@nimo23 mentions in https://github.com/orval-labs/orval/pull/1778#discussion_r1899133450 that with the current suggestion, a present but empty body will fail. I think that is correct behavior, since it is invalid JSON (state 3).

States 4 and 5 represent the expected behavior. I therefore propose handling them explicitly, while treating the others as mentioned:

export const listPets = async (
  params?: ListPetsParams,
  options?: RequestInit,
): Promise<listPetsResponse> => {
  const res = await fetch(getListPetsUrl(params), {
    ...options,
    method: 'GET',
  });
  
- const data: Pets = await res.json();
+ const data: Pets = (res.body && ![204, 205, 304].includes(res.status))
+   ? await res.json()
+   : {};

  return { status: res.status, data, headers: res.headers };
};

clemeth avatar Dec 31 '24 13:12 clemeth

Excellent summary @clemeth

melloware avatar Dec 31 '24 13:12 melloware

Thank you for putting this together. I understand that you have merged both of my ideas. I completely agree with this, so I would like to implement this.

soartec-lab avatar Jan 04 '25 01:01 soartec-lab