-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Allow plugins to register handler types #1521
Changes from 6 commits
beb3d50
bb9d082
847e8aa
b08fc92
7e74c11
28f7611
512d74b
6e7e7e6
0a41158
4ffe0e0
3270767
d6200b9
6fb6178
a4602af
9d98069
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,9 @@ exports = module.exports = internals.Route = function (options, server, env) { | |
schemaError = Schema.routeConfig(this.settings); | ||
Utils.assert(!schemaError, 'Invalid route config for', options.path, ':', schemaError); | ||
|
||
schemaError = Schema.routeHandler(this.settings.handler, server.handlerSchema); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Schema validation should move into each handler generation function. This also removes the need to pass the schema when creating custom handlers. Instead, the handler generation method can just do it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would that work? The handler generation method has access to the handler function and the schema, but not what the user has actually specified. For example:
The validation can't actually happen until:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
Utils.assert(!schemaError, 'Invalid route handler for', options.path, ':', schemaError); | ||
|
||
this.server = server; | ||
this.env = env || {}; // Plugin-specific environment | ||
this.method = options.method.toLowerCase(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,7 +128,7 @@ internals.routeOptionsSchema = { | |
method: Joi.alternatives(Joi.string(), Joi.array().includes(Joi.string()).min(1)).required(), | ||
path: Joi.string().required(), | ||
vhost: [Joi.string(), Joi.array()], | ||
handler: Joi.any(), // Validated in route.config | ||
handler: Joi.any(), // Validated in routeHandler() | ||
config: Joi.object().allow(null) | ||
}; | ||
|
||
|
@@ -156,53 +156,7 @@ internals.pre = [ | |
|
||
internals.routeConfigSchema = { | ||
pre: Joi.array().includes(internals.pre.concat(Joi.array().includes(internals.pre).min(1))), | ||
handler: [ | ||
Joi.func(), | ||
Joi.string(), | ||
Joi.object({ | ||
directory: Joi.object({ | ||
path: Joi.alternatives(Joi.string(), Joi.array().includes(Joi.string()), Joi.func()).required(), | ||
index: Joi.boolean(), | ||
listing: Joi.boolean(), | ||
showHidden: Joi.boolean(), | ||
redirectToSlash: Joi.boolean(), | ||
lookupCompressed: Joi.boolean(), | ||
defaultExtension: Joi.string().alphanum() | ||
}), | ||
file: [ | ||
Joi.string(), | ||
Joi.func(), | ||
Joi.object({ | ||
path: Joi.string().required(), | ||
filename: Joi.string().with('mode'), | ||
mode: Joi.string().valid('attachment', 'inline').allow(false), | ||
lookupCompressed: Joi.boolean() | ||
}) | ||
], | ||
proxy: Joi.object({ | ||
host: Joi.string().xor('mapUri', 'uri'), | ||
port: Joi.number().integer().without('mapUri', 'uri'), | ||
protocol: Joi.string().valid('http', 'https', 'http:', 'https:').without('mapUri', 'uri'), | ||
uri: Joi.string().without('host', 'port', 'protocol', 'mapUri'), | ||
passThrough: Joi.boolean(), | ||
rejectUnauthorized: Joi.boolean(), | ||
xforward: Joi.boolean(), | ||
redirects: Joi.number().min(0).integer().allow(false), | ||
timeout: Joi.number().integer(), | ||
mapUri: Joi.func().without('host', 'port', 'protocol', 'uri'), | ||
onResponse: Joi.func(), | ||
postResponse: Joi.func(), // Backward compatibility | ||
ttl: Joi.string().valid('upstream').allow(null) | ||
}), | ||
view: [ | ||
Joi.string(), | ||
Joi.object({ | ||
template: Joi.string(), | ||
context: Joi.object() | ||
}) | ||
] | ||
}).length(1) | ||
], | ||
handler: Joi.any(), // Validated in routeHandler() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not right. It still can be one of the provided types here, including object with one key. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The validation still exists, it's just getting put off until later. I got the idea from https://github.com/spumko/hapi/blob/master/lib/schema.js#L131 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It shouldn't. Validate the basic requirements first, then the handler-specifics later. |
||
bind: Joi.object().allow(null), | ||
payload: Joi.object({ | ||
output: Joi.string().valid('data', 'stream', 'file'), | ||
|
@@ -253,6 +207,72 @@ internals.routeConfigSchema = { | |
}; | ||
|
||
|
||
// Validate route handler | ||
|
||
exports.routeHandler = function (config, handlerSchema) { | ||
|
||
var schema = Utils.clone(internals.routeHandlerSchema); | ||
var error; | ||
|
||
schema.push(Joi.object(handlerSchema).length(1)); | ||
error = Joi.validate(config, schema); | ||
|
||
return (error ? error.annotated() : null); | ||
}; | ||
|
||
|
||
// Not a joi object. Route handler schema is compiled based on registered handler types | ||
exports.routeHandlerObjectSchema = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Break this into a schema per handler type and call those from each handler. |
||
directory: Joi.object({ | ||
path: Joi.alternatives(Joi.string(), Joi.array().includes(Joi.string()), Joi.func()).required(), | ||
index: Joi.boolean(), | ||
listing: Joi.boolean(), | ||
showHidden: Joi.boolean(), | ||
redirectToSlash: Joi.boolean(), | ||
lookupCompressed: Joi.boolean(), | ||
defaultExtension: Joi.string().alphanum() | ||
}), | ||
file: [ | ||
Joi.string(), | ||
Joi.func(), | ||
Joi.object({ | ||
path: Joi.string().required(), | ||
filename: Joi.string().with('mode'), | ||
mode: Joi.string().valid('attachment', 'inline').allow(false), | ||
lookupCompressed: Joi.boolean() | ||
}) | ||
], | ||
proxy: Joi.object({ | ||
host: Joi.string().xor('mapUri', 'uri'), | ||
port: Joi.number().integer().without('mapUri', 'uri'), | ||
protocol: Joi.string().valid('http', 'https', 'http:', 'https:').without('mapUri', 'uri'), | ||
uri: Joi.string().without('host', 'port', 'protocol', 'mapUri'), | ||
passThrough: Joi.boolean(), | ||
rejectUnauthorized: Joi.boolean(), | ||
xforward: Joi.boolean(), | ||
redirects: Joi.number().min(0).integer().allow(false), | ||
timeout: Joi.number().integer(), | ||
mapUri: Joi.func().without('host', 'port', 'protocol', 'uri'), | ||
onResponse: Joi.func(), | ||
postResponse: Joi.func(), // Backward compatibility | ||
ttl: Joi.string().valid('upstream').allow(null) | ||
}), | ||
view: [ | ||
Joi.string(), | ||
Joi.object({ | ||
template: Joi.string(), | ||
context: Joi.object() | ||
}) | ||
] | ||
}; | ||
|
||
|
||
internals.routeHandlerSchema = [ | ||
Joi.func(), | ||
Joi.string() | ||
]; | ||
|
||
|
||
exports.view = function (options) { | ||
|
||
var schema = internals.viewSchema({ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,10 +13,14 @@ var Defaults = require('./defaults'); | |
var Request = require('./request'); | ||
var Router = require('./router'); | ||
var Schema = require('./schema'); | ||
var File = require('./file'); | ||
var Proxy = require('./proxy'); | ||
var Directory = require('./directory'); | ||
var Views = require('./views'); | ||
var Ext = require('./ext'); | ||
var Handler = require('./handler'); | ||
var Utils = require('./utils'); | ||
var Joi = require('joi'); | ||
// Pack delayed required inline | ||
|
||
|
||
|
@@ -218,6 +222,19 @@ exports = module.exports = internals.Server = function (/* host, port, options * | |
this.info.uri = this.info.protocol + '://' + (this._host || Os.hostname() || 'localhost') + ':' + this.info.port; | ||
} | ||
} | ||
|
||
this.handlers = { | ||
proxy: Proxy.handler, | ||
file: File.handler, | ||
directory: Directory.handler, | ||
view: Views.handler | ||
}; | ||
this.handlerSchema = { | ||
proxy: Schema.routeHandlerObjectSchema.proxy, | ||
file: Schema.routeHandlerObjectSchema.file, | ||
directory: Schema.routeHandlerObjectSchema.directory, | ||
view: Schema.routeHandlerObjectSchema.view | ||
}; | ||
}; | ||
|
||
Utils.inherits(internals.Server, Events.EventEmitter); | ||
|
@@ -469,3 +486,13 @@ internals.Server.prototype.method = function () { | |
|
||
return this.pack._method.apply(this.pack, arguments); | ||
}; | ||
|
||
|
||
internals.Server.prototype.handler = function (name, method, schema) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to follow the same pattern as server methods which are actually managed by the pack. This way, when you have multiple servers, they should all share the same handlers. |
||
|
||
Utils.assert(typeof name === 'string', 'Invalid handler name'); | ||
Utils.assert(typeof method === 'function', 'Handler must be a function:', name); | ||
schema = schema || Joi.any(); | ||
this.handlers[name] = method; | ||
this.handlerSchema[name] = schema; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave this here along with the server handler registration. Instead of the server constructor having the logic of adding the built-ins, create a function here and have the server call it. Keeps the inter dependencies cleaner.