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

204 No content generated code throws JSON parsing error #1656

Closed
zZHorizonZz opened this issue Oct 5, 2024 · 18 comments · Fixed by #1778
Closed

204 No content generated code throws JSON parsing error #1656

zZHorizonZz opened this issue Oct 5, 2024 · 18 comments · Fixed by #1778
Assignees
Labels
bug Something isn't working fetch Fetch client related issue
Milestone

Comments

@zZHorizonZz
Copy link

zZHorizonZz commented Oct 5, 2024

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 
@melloware melloware added the bug Something isn't working label Oct 5, 2024
@soartec-lab soartec-lab added the fetch Fetch client related issue label Oct 13, 2024
@soartec-lab
Copy link
Member

@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;
};

@nimo23
Copy link

nimo23 commented Nov 7, 2024

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 #1656 (comment) 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 #1632.

@soartec-lab
Copy link
Member

@nimo23

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

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

@nimo23
Copy link

nimo23 commented Nov 8, 2024

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

Yes, the custom fetch will be called.

@soartec-lab
Copy link
Member

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

@nimo23
Copy link

nimo23 commented Nov 10, 2024

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

@soartec-lab
Copy link
Member

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

@nimo23
Copy link

nimo23 commented Nov 16, 2024

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

@soartec-lab
Copy link
Member

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

@zZHorizonZz
Copy link
Author

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 ?

@nimo23
Copy link

nimo23 commented Nov 18, 2024

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()

@soartec-lab
Copy link
Member

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

@soartec-lab soartec-lab reopened this Nov 18, 2024
kirillsalikhov added a commit to kirillsalikhov/tree-replication that referenced this issue Nov 18, 2024
when there's no content Orval with fetch client triggers Json parse error
orval-labs/orval#1656
@soartec-lab
Copy link
Member

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?

@zZHorizonZz
Copy link
Author

zZHorizonZz commented Dec 27, 2024

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.

@soartec-lab
Copy link
Member

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

@clemeth
Copy link
Contributor

clemeth commented Dec 31, 2024

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 #1778 (comment) 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 };
};

@melloware
Copy link
Collaborator

Excellent summary @clemeth

@melloware melloware added this to the 7.3.1 milestone Dec 31, 2024
@soartec-lab
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fetch Fetch client related issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants