Skip to content

Fixed bug in Response validator #40

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixed bug in Response validator #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e407af9
Fixed bug in Response validator
Dec 22, 2021
058d6b1
Fixed another issue caused by the patching of the res.json. This time…
Dec 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions express-joi-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,19 @@ function buildErrorString(err, container) {
return ret
}

function patchResponseForResponseValidation(res, validationFn) {
res._origJson = res.json.bind(res)
res.json = validationFn
}

function restoreResponseAfterResponseValidation(res) {
if (res._origJson) {
res.json = res._origJson
}
}

module.exports.fixupResponse = restoreResponseAfterResponseValidation
Copy link
Owner

@evanshortiss evanshortiss Dec 22, 2021

Choose a reason for hiding this comment

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

I think it'd be best not to expose this. Make the fix-up automatic in all scenarios, including response, if passError is enabled. WDYT?

Copy link
Owner

@evanshortiss evanshortiss Dec 22, 2021

Choose a reason for hiding this comment

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

Ah, I see your last comment. Right, this is a tricky...

Copy link
Author

@ronniel1 ronniel1 Dec 23, 2021

Choose a reason for hiding this comment

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

This is indeed Hacky. Do you have any other suggestion?

Copy link
Owner

@evanshortiss evanshortiss Dec 25, 2021

Choose a reason for hiding this comment

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

I had this idea stewing in my head around providing schemas for certain status code ranges. It's a bit more complex though. Something like:

validation.response({
  // Schemas to run for given status codes
  '2xx': schemaSuccess,
  '4xx': schemaFail
}, {
  // These could be defined globally instead of as part of 
  // the call to response()
  passError: true
  passErrorSchemas: {
    '5xx': errrorHandlerSchema
  }
})

If no schema is provided for 5xx, then perhaps validation is bypassed?

Copy link
Author

@ronniel1 ronniel1 Dec 30, 2021

Choose a reason for hiding this comment

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

This seems like a very good interface.
It seems to me that it will still need the mechanism of overriding the json function on the Response object and require restoring the original json function


module.exports.createValidator = function generateJoiMiddlewareInstance(cfg) {
cfg = cfg || {} // default to an empty config
// We'll return this instance of the middleware
Expand Down Expand Up @@ -97,17 +110,17 @@ module.exports.createValidator = function generateJoiMiddlewareInstance(cfg) {
function response(schema, opts = {}) {
const type = 'response'
return (req, res, next) => {
const resJson = res.json.bind(res)
res.json = validateJson
patchResponseForResponseValidation(res, validateJson)
next()

function validateJson(json) {
const ret = schema.validate(json, opts.joi)
const { error, value } = ret
if (!error) {
// return res.json ret to retain express compatibility
return resJson(value)
return res._origJson(value)
} else if (opts.passError || cfg.passError) {
restoreResponseAfterResponseValidation(res)
ret.type = type
next(ret)
} else {
Expand Down
47 changes: 43 additions & 4 deletions index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,22 @@ const expect = require('chai').expect
describe('express joi', function() {
var schema, mod

function getRequester(middleware) {
const errorHandler = (err, req, res, next) => {
let fixupResponse = require('./express-joi-validation.js').fixupResponse
if (err) {
if (err.error && err.error.isJoi) {
res.status(422).json({
type: err.type,
message: err.error.toString()
})
} else {
fixupResponse(res)
res.status(500).send({ message: err.message })
}
} else next(err)
}

function getRequester(middleware, errMiddlware = undefined) {
const app = require('express')()

// Must apply params middleware inline with the route to match param names
Expand Down Expand Up @@ -94,6 +109,12 @@ describe('express joi', function() {
const { key } = req.params
res.json({ key: +key || 'none' })
})
app.get('/error', middleware, (req, res, next) => {
next(new Error('an error'))
})
if (errMiddlware) {
app.use(errMiddlware)
}

return supertest(app)
}
Expand Down Expand Up @@ -248,13 +269,31 @@ describe('express joi', function() {
.expect(200)
})

it('should pass an error to subsequent handler if it is asked', function() {
it('should pass an error to subsequent handler if it is asked', function(done) {
const middleware = mod.response(schema, {
passError: true
})
return getRequester(middleware)
const res = getRequester(middleware, errorHandler)
.get('/response/one')
.expect(500)
.expect(422)
.end(function(err, res) {
expect(res.body.type).to.contain('response')
expect(res.body.message).to.contain('ValidationError:')
done()
})
})

it('Allow error handler to work if error occured in request handler', function(done) {
const middleware = mod.response(schema, {
passError: true
})
const res = getRequester(middleware, errorHandler)
.get('/error')
.expect(422)
Copy link
Owner

@evanshortiss evanshortiss Dec 22, 2021

Choose a reason for hiding this comment

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

This should be a 500 error, yet the test is passing. That's odd. Checking res.statusCode in the end block instead will do the trick.

I'll need to look into why supertest is not asserting it correctly since it seems to be the same for other tests...

Copy link
Author

@ronniel1 ronniel1 Dec 23, 2021

Choose a reason for hiding this comment

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

The errorHandler sets the status code to 422, so I think this works as expected

.end(function(err, res) {
expect(res.body.message).to.contain('an error')
done()
})
})

it('should return an alternative status for failure', function() {
Expand Down