feat: set undefined on null input #2731

metcoder95 · 2020-12-10T21:56:59Z

Fixes #2618
On empty body.payload, undefined is set instead of null.

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

metcoder95 · 2020-12-10T21:58:39Z

lib/validation.js

+  const isUndefined = request[paramName] === undefined
+  const ret = validatorFunction && validatorFunction(isUndefined ? null : request[paramName])


I'm not 100% sure of this approach. I think it's good as we're explicitly passing null indicating the absence of value instead of undefined which only exists in JavaScript. Please, let me know your thoughts.

What is this solving?

Basically, if we set undefined as the default value for an empty req body when running the schema validation, doesn't matter if the schema allows the body to be nullable, as we're passing undefined the validation will throw right away. Changing from undefined to null allows those schemas which are nullable to validate and pass if the value it's empty (at least the body)

I see. This is validating the value of path parameters. How can a path parameter be null? @Eomm what is your opinion here?

For now yes, I can reduce it to only the body for sure. I'll like to hear your opinion, maybe I'm missing something where there's a better way to handle it

This function is called 4 times with the paramName equals to params, querystring, headers and body
(only in the case paramName===body this check is needed if I have understood the case)
I saw there are some tests that cover this case already

I think this check is faster than a string compare so it LGTM

Eomm

We should add some tests to check:

how to send an empty body response (is reply.send() working?)
send empty in sync and async context
a notice in docs on how to deal with it

metcoder95 · 2020-12-14T11:07:38Z

We should add some tests to check:

how to send an empty body response (is reply.send() working?)

send empty in sync and async context

a notice in docs on how to deal with it

Answering your points 🙂

It does, it's returning an empty string. Actually, I'm also wondering if it's expected when sending an empty body. Should we handle it differently?
I'm not having any issues (at least tests are not failing), but let me add those tests cases explicitly 👍
Sure, just let me know if I'm missing something, in all cases I'll update the docs based on all the comments for this PR

jsumners · 2020-12-14T13:02:09Z

It does, it's returning an empty string. Actually, I'm also wondering if it's expected when sending an empty body. Should we handle it differently?

Is this new behavior? I genuinely cannot recall. I know there are codes like 204 that shouldn't even send an empty string.

Eomm

It does, it's returning an empty string. Actually, I'm also wondering if it's expected when sending an empty body. Should we handle it differently?

I was wrong sorry, I was thinking of another PR 🤦

Sure, just let me know if I'm missing something, in all cases I'll update the docs based on all the comments for this PR

In the hook section there are a couple of hooks that say that the req.body is null (now undefined).

Eomm · 2020-12-14T21:16:03Z

lib/validation.js

+  const isUndefined = request[paramName] === undefined
+  const ret = validatorFunction && validatorFunction(isUndefined ? null : request[paramName])


This function is called 4 times with the paramName equals to params, querystring, headers and body
(only in the case paramName===body this check is needed if I have understood the case)
I saw there are some tests that cover this case already

I think this check is faster than a string compare so it LGTM

metcoder95 · 2020-12-14T21:42:36Z

It does, it's returning an empty string. Actually, I'm also wondering if it's expected when sending an empty body. Should we handle it differently?

Is this new behavior? I genuinely cannot recall. I know there are codes like 204 that shouldn't even send an empty string.

I didn't check the behavior on 204, let me double-check it

I was wrong sorry, I was thinking of another PR 🤦

No issues 😄

In the hook section there are a couple of hooks that say that the req.body is null (now undefined).

Working on it 👍

mcollina

lgtm

metcoder95 · 2020-12-15T15:35:25Z

@jsumners when double-checking the behavior on 204, the response is sending an empty string as a response. Taking a look into other tests, like this one, we're basically just validating that the body it's empty, then I think this is expected. I added a new test just to cover that scenario. Sounds good?

delvedor

LGTM

While this should be a bugfix, I agree that it might cause some disruption, so let's be safe and release it as a major.

* feat: handle empty body.payload as undefined * test: adjust tests to body === undefined * test: add tests for undefined req.body on 204 * docs: update docs for undefined as default

github-actions · 2022-02-04T00:14:20Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

metcoder95 added 2 commits December 10, 2020 22:50

feat: handle empty body.payload as undefined

5aa3ed2

test: adjust tests to body === undefined

fddbc21

metcoder95 commented Dec 10, 2020

View reviewed changes

Eomm added the semver-major Issue or PR that should land as semver major label Dec 11, 2020

Eomm reviewed Dec 11, 2020

View reviewed changes

Eomm reviewed Dec 14, 2020

View reviewed changes

fox1t approved these changes Dec 15, 2020

View reviewed changes

mcollina approved these changes Dec 15, 2020

View reviewed changes

metcoder95 added 2 commits December 15, 2020 16:30

test: add tests for undefined req.body on 204

e080ce4

docs: update docs for undefined as default

91e0be0

metcoder95 requested a review from jsumners December 15, 2020 16:41

metcoder95 mentioned this pull request Dec 15, 2020

feat: Set HEAD routes automatically for each GET route #2700

Merged

4 tasks

Eomm approved these changes Dec 15, 2020

View reviewed changes

delvedor approved these changes Dec 16, 2020

View reviewed changes

delvedor added the v4.x Issue or pr related to Fastify v4 label Dec 16, 2020

mcollina merged commit 67a7a19 into fastify:next Dec 16, 2020

metcoder95 mentioned this pull request Jan 18, 2021

Handle null input #2618

Closed

RafaelGSS mentioned this pull request Sep 7, 2021

Body object in request could be null (Type change) #3299

Closed

4 tasks

github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: set undefined on null input #2731

feat: set undefined on null input #2731

metcoder95 commented Dec 10, 2020 •

edited

metcoder95 Dec 10, 2020

jsumners Dec 11, 2020

metcoder95 Dec 14, 2020

jsumners Dec 14, 2020

metcoder95 Dec 14, 2020

Eomm Dec 14, 2020

Eomm left a comment

metcoder95 commented Dec 14, 2020

jsumners commented Dec 14, 2020

Eomm left a comment

Eomm Dec 14, 2020

metcoder95 commented Dec 14, 2020

mcollina left a comment

metcoder95 commented Dec 15, 2020

delvedor left a comment

github-actions bot commented Feb 4, 2022

		const isUndefined = request[paramName] === undefined
		const ret = validatorFunction && validatorFunction(isUndefined ? null : request[paramName])

feat: set undefined on null input #2731

feat: set undefined on null input #2731

Conversation

metcoder95 commented Dec 10, 2020 • edited

Checklist

metcoder95 Dec 10, 2020

Choose a reason for hiding this comment

jsumners Dec 11, 2020

Choose a reason for hiding this comment

metcoder95 Dec 14, 2020

Choose a reason for hiding this comment

jsumners Dec 14, 2020

Choose a reason for hiding this comment

metcoder95 Dec 14, 2020

Choose a reason for hiding this comment

Eomm Dec 14, 2020

Choose a reason for hiding this comment

Eomm left a comment

Choose a reason for hiding this comment

metcoder95 commented Dec 14, 2020

jsumners commented Dec 14, 2020

Eomm left a comment

Choose a reason for hiding this comment

Eomm Dec 14, 2020

Choose a reason for hiding this comment

metcoder95 commented Dec 14, 2020

mcollina left a comment

Choose a reason for hiding this comment

metcoder95 commented Dec 15, 2020

delvedor left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 4, 2022

metcoder95 commented Dec 10, 2020 •

edited