Body object in request could be null (Type change)

[ReeceXW]

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[Eomm]

I don't remember if we cover this use case in a not-ts test actually.

Would you mind adding it?

[climba03003]

I think it will be a breaking change for most of the TypeScript users

[ReeceXW]

I think it will be a breaking change for most of the TypeScript users

I don't think this is a bad thing. 2 situations:

1. They already have null checks on the body object, no type issues will arise
2. They have no null checks on the body object, which I would consider a bug

[RafaelGSS]

Just for curiosity, the types accept req.body as undefined, what's the use case to check the specific null condition?

[climba03003]

I think it will be a breaking change for most of the TypeScript users

I don't think this is a bad thing. 2 situations:

1. They already have null checks on the body object, no type issues will arise

2. They have no null checks on the body object, which I would consider a bug

When the users specify a schema, the null check will be on the validation phase.
So, the users do not need to be double check it every time.

Also, the default type is unknown. Most of the time, the users need to cast it or use the RouteGenericInterface for the type guard.

Example,

// it is valid before and we know that body must be object
fastify.post<{ Body: object }>('/', { schema: { body: { type: 'object' } } }, function(request, reply) {
  reply.send(request.body.error)
})

For GET request, it is the only case that need the null for body as we do not validate or parse the payload.

[Eomm]

For GET request, it is the only case that need the null for body as we do not validate or parse the payload.

Here the doubt: is it null or undefined?

null is still a valid json payload

[climba03003]

For GET request, it is the only case that need the null for body as we do not validate or parse the payload.

Here the doubt: is it null or undefined?

null is still a valid json payload

The default value is null and if we do not parse body and modify this value.
It should be null.

fastify/lib/request.js

Line 14 in fee3427

this.body = null

[RafaelGSS]

For GET request, it is the only case that need the null for body as we do not validate or parse the payload.

Here the doubt: is it null or undefined?
null is still a valid json payload

The default value is null and if we do not parse body and modify this value.
It should be null.

fastify/lib/request.js

Line 14 in fee3427

this.body = null

@fastify/core We know why are we using null as default instead of undefined? I do feel that as @Eomm mentioned, null is still a valid payload. In those cases, might be better to use undefined, isn't it?

[jsumners]

#2618

[RafaelGSS]

Thanks a lot, @jsumners! So, since #2731 was landed I'm -1 in this PR, since as @climba03003 mentioned, it may break some typescript users and in fact, in the next major release this behavior will change again.

[L2jLiga]

I'm -1 on this changes, in my code I mostly rely on request/reply schemas which should cover this case until I mark it explicitly as nullable

[mcollina]

lgtm

[zekth]

I'm -1 on this changes, in my code I mostly rely on request/reply schemas which should cover this case until I mark it explicitly as nullable

Could you share a snippet? Isn't this declaration fixing this issue?

export const postWishlist = async (req: FastifyRequest<{ Body: foobar }>, resp: FastifyReply): Promise<void> => {

If so, i think we should enforce people to use this kind of declaration as it's TypeScript compliant. And users should not use something like const bar = req.body.foo as foo.

[L2jLiga]

I'm -1 on this changes, in my code I mostly rely on request/reply schemas which should cover this case until I mark it explicitly as nullable

Could you share a snippet? Isn't this declaration fixing this issue?

export const postWishlist = async (req: FastifyRequest<{ Body: foobar }>, resp: FastifyReply): Promise<void> => {

If so, i think we should enforce people to use this kind of declaration as it's TypeScript compliant. And users should not use something like const bar = req.body.foo as foo.

Sure, here's repository: https://github.com/L2jLiga/fastify-request-body-validation

With changes from this PR typescript emit error:

[zekth]

That's a blocker indeed. It shouldn't break the defined non null types like @L2jLiga shared.

[Eomm]

@ReeceXW do you want to finalize this PR?

Reading the comments above I think we should:

1. target the next branch
2. within the PR feat: set undefined on null input #2731, the body can be:
   • undefined if the payload is missing --> already covered
   • null if the client sends a null payload --> should it be aligned within the JSON schema?

is that correct @fastify/typescript ?

[mcollina]

Closed for no activity, feel free to reopen in case you still want to pursue this change