honojs / hono

Web framework built on Web Standards
https://hono.dev
MIT License
19.98k stars 570 forks source link

Parsing a null body throws exception instead of returning null #2983

Closed JoaquimLey closed 4 months ago

JoaquimLey commented 4 months ago

What is the feature you are proposing?

Current behaviour

When receiving a request with an optional body in its payload, I was hoping that c.req.json() would more gracefully handle the case where there's nothing to parse (body is null).

It currently throws an exception.

Runtime:
bun

Versions:
"hono": "^4.4.5",
"@hono/zod-validator": "^0.2.2",

Expected / Desired behaviour

Return null instead of throwing the exception.


Wanted to avoid catch an exception when parsing the body as this make the router's code a little bit more convoluted than I was hoping for.

On the other hand it makes it explicit that we need to handle the exception on the "consumer" side, which one could argue it has benefits.

Steps to reproduce:

index.ts

app.post("/no-body-example", async (c) => {
  try {
    const body = await c.req.json();
    console.log("Body: ", body);
    return c.json({ message: "OK" }, 201);
  } catch (e) {
    console.error("Error on no body", e);
    return c.json({ message: "Error on request" }, 500);
  }
});

And test case:

it("It should return 201 even when request has no body", async () => {
  const res = await app.request(
    "/no-body-example",
    {
      method: "POST",
      headers: { "Content-Type": "application/json" },
      body: null // or we can simply not specify it or 'undefined'
    },
    {}
  );

  expect(res.status).toBe(201);
});

Result

Error on register  68 |           body = JSON.stringify(body);
69 |         }
70 |         return new Response(body)[key]();
71 |       });
72 |     }
73 |     return bodyCache[key] = raw[key]();
                                     ^
SyntaxError: Unexpected end of JSON input

(...)

188 |         headers: { "Content-Type": "application/json" },
189 |       },
190 |       {}
191 |     );
192 | 
193 |     expect(res.status).toBe(201);
                             ^
error: expect(received).toBe(expected)

While I understand that obfuscating what's happening might not be optimal for some use cases I'm wondering if it makes sense to make this body parsing safe to consumers and instead have a more explicit parsing method that keeps this behaviour.

Relevant part of the framework's request.ts: https://github.com/honojs/hono/blob/main/src/request.ts#L208-L227

  private cachedBody = (key: keyof Body) => {
    const { bodyCache, raw } = this
    const cachedBody = bodyCache[key]

    if (cachedBody) {
      return cachedBody
    }

    const anyCachedKey = Object.keys(bodyCache)[0]
    if (anyCachedKey) {
      return (bodyCache[anyCachedKey as keyof Body] as Promise<BodyInit>).then((body) => {
        if (anyCachedKey === 'json') {
          body = JSON.stringify(body)
        }
        return new Response(body)[key]()
      })
    }

    return (bodyCache[key] = raw[key]())
  }

Happy to give it a go at a PR if this is an acceptable change and/or happy to further discuss this.

JoaquimLey commented 4 months ago

The test case would be something along these lines

https://github.com/honojs/hono/blob/main/src/request.test.ts#L189-L205

  test('req.json()', async () => {
    const req = new HonoRequest(
      new Request('http://localhost', {
        method: 'POST',
        body: '{"foo":"bar"}',
      })
    )
    expect(await req.json()).toEqual(json) // This check is repeated: submitted PR #2984 removing these
    expect(await req.json()).toEqual(json)
    expect(await req.text()).toEqual(text)
    expect(await req.arrayBuffer()).toEqual(buffer)
    expect(await req.blob()).toEqual(
      new Blob([text], {
        type: 'text/plain;charset=utf-8',
      })
    )
  })

  test('req.json() with null body', async () => {
    const req = new HonoRequest(
      new Request('http://localhost', {
        method: 'POST',
      })
    )
    expect(await req.json()).toEqual(null)
  })

Not necessarily expecting this to be the change we introduce to the test suite but, just to help further illustrate the use case.

ref: https://github.com/honojs/hono/pull/2984

yusukebe commented 4 months ago

Hi @JoaquimLey

Regarding parsing JSON body in a request object, we can try to use a standard Request object.

The following code will throw the error:

const req = new Request('http://localhost/', {
  method: 'POST',
  headers: { 'Content-Type': 'application/json' },
  body: null,
})

await req.json()

Bun:

21 |   method: 'POST',
22 |   headers: { 'Content-Type': 'application/json' },
23 |   body: null,
24 | })
25 |
26 | await req.json()
               ^
SyntaxError: Unexpected end of JSON input
      at /Users/yusuke/work/honojs/hono/sandbox/src/2983.mts:26:11

Bun v1.1.17 (macOS arm64)

Node.js:

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 _Request.json (node:internal/deps/undici/undici:4239:18)
    at file:///Users/yusuke/work/honojs/hono/sandbox/src/2983.mts:1:245
    at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:475:24)
    at async asyncRunEntryPointWithESMLoader (node:internal/modules/run_main:109:5)

Node.js v22.2.0

Handling it in the handler also throws the error. The code to reproduce it is here:

const app = new Hono()

app.post('/', async (c) => {
  const data = await c.req.raw.json()
  return c.json(data)
})

await app.request('/', {
  method: 'POST',
  headers: { 'Content-Type': 'application/json' },
  body: null,
})

So, throwing the error may be correctly expected, though we may have to change the error message. I think we have to follow the standard API's behavior for request.json(). What do you think of it?

JoaquimLey commented 4 months ago

I agree we should keep the default behaviour, the issue I had was my client-side handling is already preventing that so I thought this was something specific from hono and not the standard library. I'm going to close this issue and thank you for your feedback.