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.

Sign up for GitHub

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

Jump to bottom

feat: set undefined on null input #2731

Merged
merged 4 commits into from
Dec 16, 2020
Merged

feat: set undefined on null input #2731

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5aa3ed2
feat: handle empty body.payload as undefined
metcoder95 Dec 10, 2020
fddbc21
test: adjust tests to body === undefined
metcoder95 Dec 10, 2020
e080ce4
test: add tests for undefined req.body on 204
metcoder95 Dec 15, 2020
91e0be0
docs: update docs for undefined as default
metcoder95 Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ function Request (id, params, req, query, log, context) {
this.raw = req
this.query = query
this.log = log
this.body = null
this.body = undefined
}

function getTrustProxyFn (tp) {
Expand Down Expand Up @@ -50,7 +50,7 @@ function buildRegularRequest (R) {
this.raw = req
this.query = query
this.log = log
this.body = null
this.body = undefined
}
_Request.prototype = new R()

Expand Down
3 changes: 2 additions & 1 deletion lib/validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@ function compileSchemasForValidation (context, compile) {
}

function validateParam (validatorFunction, request, paramName) {
const ret = validatorFunction && validatorFunction(request[paramName])
const isUndefined = request[paramName] === undefined
const ret = validatorFunction && validatorFunction(isUndefined ? null : request[paramName])
Comment on lines +70 to +71
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this solving?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (ret === false) return validatorFunction.errors
if (ret && ret.error) return ret.error
if (ret && ret.value) request[paramName] = ret.value
Expand Down
4 changes: 2 additions & 2 deletions test/delete.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ test('shorthand - delete with application/json Content-Type header and without b
t.plan(4)
const fastify = require('..')()
fastify.delete('/', {}, (req, reply) => {
t.equal(req.body, null)
t.equal(req.body, undefined)
reply.send(req.body)
})
fastify.inject({
Expand All @@ -313,6 +313,6 @@ test('shorthand - delete with application/json Content-Type header and without b
}, (err, response) => {
t.error(err)
t.strictEqual(response.statusCode, 200)
t.deepEqual(JSON.parse(response.payload), null)
t.deepEqual(response.payload, '')
})
})
4 changes: 2 additions & 2 deletions test/helper.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ module.exports.payloadMethod = function (method, t, isSetErrorHandler = false) {
}, (err, response, body) => {
t.error(err)
t.strictEqual(response.statusCode, 200)
t.strictEqual(JSON.parse(body.toString()), null)
t.strictEqual(body.toString(), '')
})

// Must use inject to make a request without a Content-Length header
Expand All @@ -199,7 +199,7 @@ module.exports.payloadMethod = function (method, t, isSetErrorHandler = false) {
}, (err, res) => {
t.error(err)
t.strictEqual(res.statusCode, 200)
t.strictEqual(JSON.parse(res.payload), null)
t.strictEqual(res.payload, '')
})
})

Expand Down
4 changes: 2 additions & 2 deletions test/hooks.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2058,7 +2058,7 @@ test('request in onRequest, preParsing, preValidation and onResponse', t => {
const fastify = Fastify()

fastify.addHook('onRequest', function (request, reply, done) {
t.deepEqual(request.body, null)
t.deepEqual(request.body, undefined)
t.deepEqual(request.query, { key: 'value' })
t.deepEqual(request.params, { greeting: 'hello' })
t.deepEqual(request.headers, {
Expand All @@ -2072,7 +2072,7 @@ test('request in onRequest, preParsing, preValidation and onResponse', t => {
})

fastify.addHook('preParsing', function (request, reply, payload, done) {
t.deepEqual(request.body, null)
t.deepEqual(request.body, undefined)
t.deepEqual(request.query, { key: 'value' })
t.deepEqual(request.params, { greeting: 'hello' })
t.deepEqual(request.headers, {
Expand Down
4 changes: 2 additions & 2 deletions test/internals/request.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ test('Regular request', t => {
t.strictEqual(request.ip, 'ip')
t.strictEqual(request.ips, undefined)
t.strictEqual(request.hostname, 'hostname')
t.strictEqual(request.body, null)
t.strictEqual(request.body, undefined)
t.strictEqual(request.method, 'GET')
t.strictEqual(request.url, '/')
t.deepEqual(request.socket, req.socket)
Expand Down Expand Up @@ -94,7 +94,7 @@ test('Request with trust proxy', t => {
t.strictEqual(request.ip, '2.2.2.2')
t.deepEqual(request.ips, ['ip', '1.1.1.1', '2.2.2.2'])
t.strictEqual(request.hostname, 'example.com')
t.strictEqual(request.body, null)
t.strictEqual(request.body, undefined)
t.strictEqual(request.method, 'GET')
t.strictEqual(request.url, '/')
t.deepEqual(request.socket, req.socket)
Expand Down
24 changes: 10 additions & 14 deletions test/nullable-validation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ test('object or null body', t => {
method: 'POST',
url: '/',
handler: (req, reply) => {
t.strictEqual(req.body, null)
reply.code(200).send({ requestBody: req.body })
t.strictEqual(req.body, undefined)
reply.code(200).send({ isUndefinedBody: req.body === undefined })
},
schema: {
body: {
Expand All @@ -79,10 +79,8 @@ test('object or null body', t => {
type: 'object',
nullable: true,
properties: {
requestBody: {
type: 'string',
format: 'email',
nullable: true
isUndefinedBody: {
type: 'boolean'
}
}
}
Expand All @@ -100,7 +98,7 @@ test('object or null body', t => {
}, (err, response, body) => {
t.error(err)
t.strictEqual(response.statusCode, 200)
t.deepEqual(JSON.parse(body), { requestBody: null })
t.deepEqual(JSON.parse(body), { isUndefinedBody: true })
})
})
})
Expand All @@ -114,8 +112,8 @@ test('nullable body', t => {
method: 'POST',
url: '/',
handler: (req, reply) => {
t.strictEqual(req.body, null)
reply.code(200).send({ requestBody: req.body })
t.strictEqual(req.body, undefined)
reply.code(200).send({ isUndefinedBody: req.body === undefined })
},
schema: {
body: {
Expand All @@ -133,10 +131,8 @@ test('nullable body', t => {
type: 'object',
nullable: true,
properties: {
requestBody: {
type: 'string',
format: 'email',
nullable: true
isUndefinedBody: {
type: 'boolean'
}
}
}
Expand All @@ -154,7 +150,7 @@ test('nullable body', t => {
}, (err, response, body) => {
t.error(err)
t.strictEqual(response.statusCode, 200)
t.deepEqual(JSON.parse(body), { requestBody: null })
t.deepEqual(JSON.parse(body), { isUndefinedBody: true })
})
})
})