refactor tunnel middleware into two phases #1044

Merged
merged 10 commits into from Mar 26, 2013

Projects

None yet

2 participants

@ekashida
Contributor

By splitting the tunnel logic into a detection phase and a handling phase,
applications are free to override the handling logic.

In order to maintain backwards-compatibility, instead of refactoring the
existing tunnel middleware, a tunnel-demux middleware to detect the type of
tunnel request, and tunnel-(rpc|specs|type) middleware to handle each type of
request have been added. Mojito itself will now use these new middleware in
place of the single tunnel middleware, which will remain for those applications
that are specifying it in their configuration.

@ekashida
Contributor

Still working on this (tests, docs, end-to-end testing, etc). Would like some feedback for what I have so far!

@caridy caridy and 1 other commented on an outdated diff Mar 21, 2013
lib/app/middleware/mojito-handler-tunnel-specs.js
+
+function sendError(res, msg, code) {
+ sendData(res, {error: msg}, (code || 500));
+}
+
+
+/**
+ * Exports a middleware factory that can handle spec tunnel requests.
+ *
+ * @param {Object} config The configuration.
+ * @return {Function} The handler.
+ */
+module.exports = function (config) {
+ return function (req, res, next) {
+ var specsReq = req._tunnel && req._tunnel.specsReq,
+ instance = {},
@caridy
caridy Mar 21, 2013 Collaborator

do not create objects if they are not going to be used. move this below the if (!specsReq), otherwise you will be creating an empty object per req even when it is not a tunnel request, in which case this will have to be collected by GC.

@ekashida
ekashida Mar 22, 2013 Contributor

agreed

@caridy caridy and 1 other commented on an outdated diff Mar 21, 2013
lib/app/middleware/mojito-handler-tunnel-specs.js
+ * @return {Function} The handler.
+ */
+module.exports = function (config) {
+ return function (req, res, next) {
+ var specsReq = req._tunnel && req._tunnel.specsReq,
+ instance = {},
+ type,
+ name;
+
+ if (!specsReq) {
+ return next();
+ }
+
+ type = specsReq.type;
+ name = specsReq.name;
+ name = name && name.split('.').slice(0, -1).join('.');
@caridy
caridy Mar 21, 2013 Collaborator

isn't easier to use require('path.extname(), then slicing based on that? this line seems complex.

@ekashida
ekashida Mar 22, 2013 Contributor

I don't think it was simplified much, but the comment definitely helps :)

// Remove the extension name.
ext  = libpath.extname(name);                                                  
name = name && ext && name.slice(0, -ext.length);
@ekashida
ekashida Mar 22, 2013 Contributor

I got rid of this logic completely by passing in (from demux) just the basename without the extension:

// Get the basename without the .json extension.                               
name = libpath.basename(path, '.json');
@caridy caridy and 1 other commented on an outdated diff Mar 22, 2013
lib/app/middleware/mojito-handler-tunnel-demux.js
+
+ RPC tunnel:
+ /tunnel (or it could just have the tunnel header)
+
+ Type tunnel:
+ /static/{type}/definition.json
+ /{tunnelPrefix}/{type}/definition.json // custom prefix
+ /tunnel/static/{type}/definition.json // according to a UT
+
+ Spec tunnel:
+ /static/{type}/specs/default.json
+ /{staticPrefix}/{type}/specs/default.json // custom prefix
+ /tunnel/static/{type}/specs/default.json // according to a UT
+ **/
+
+ url = req.url.split('?')[0];
@caridy
caridy Mar 22, 2013 Collaborator

hmm, I guess we should use require('url').parse() to really understand that url.

@ekashida
ekashida Mar 22, 2013 Contributor

agreed

@caridy caridy and 1 other commented on an outdated diff Mar 22, 2013
lib/app/middleware/mojito-handler-tunnel-specs.js
+ type,
+ name;
+
+ if (!specsReq) {
+ return next();
+ }
+
+ type = specsReq.type;
+ name = specsReq.name;
+ name = name && name.split('.').slice(0, -1).join('.');
+
+ if (!type || !name) {
+ return sendError(res, 'Not found: ' + req.url, 500);
+ }
+
+ instance.base = type;
@caridy
caridy Mar 22, 2013 Collaborator

here is where you use instance for the first time, here is where you should create the object with base on it.

@ekashida
ekashida Mar 22, 2013 Contributor

agreed

@caridy caridy and 1 other commented on an outdated diff Mar 22, 2013
lib/app/middleware/mojito-handler-tunnel-specs.js
+ return function (req, res, next) {
+ var specsReq = req._tunnel && req._tunnel.specsReq,
+ instance = {},
+ type,
+ name;
+
+ if (!specsReq) {
+ return next();
+ }
+
+ type = specsReq.type;
+ name = specsReq.name;
+ name = name && name.split('.').slice(0, -1).join('.');
+
+ if (!type || !name) {
+ return sendError(res, 'Not found: ' + req.url, 500);
@caridy
caridy Mar 22, 2013 Collaborator

If it is not found, it should be 404, if it is an invalid url, then 500 is just fine.

@ekashida
ekashida Mar 22, 2013 Contributor

Damn you copy/paste!

@caridy caridy commented on an outdated diff Mar 22, 2013
lib/mojito.js
@@ -90,7 +90,10 @@ MojitoServer.MOJITO_MIDDLEWARE = [
'mojito-parser-body',
'mojito-parser-cookies',
'mojito-contextualizer',
- 'mojito-handler-tunnel',
@caridy
caridy Mar 22, 2013 Collaborator

this will be problematic for BC. Can we preserve the tunnel as a sugar for all the new middleware? middlewares within a middleware is a common practice, and once we refactor mojito middleware infrastructure, in which case we know we will break BC, we can remove the sugar.

@caridy caridy commented on an outdated diff Mar 22, 2013
lib/app/middleware/mojito-handler-tunnel-type.js
+
+function sendError(res, msg, code) {
+ sendData(res, {error: msg}, (code || 500));
+}
+
+
+/**
+ * Exports a middleware factory that can handle type tunnel requests.
+ *
+ * @param {Object} config The configuration.
+ * @return {Function} The handler.
+ */
+module.exports = function (config) {
+ return function (req, res, next) {
+ var typeReq = req._tunnel && req._tunnel.typeReq,
+ instance = {};
@caridy
caridy Mar 22, 2013 Collaborator

same as above.

@caridy caridy commented on an outdated diff Mar 22, 2013
lib/app/middleware/mojito-handler-tunnel-type.js
+ * Exports a middleware factory that can handle type tunnel requests.
+ *
+ * @param {Object} config The configuration.
+ * @return {Function} The handler.
+ */
+module.exports = function (config) {
+ return function (req, res, next) {
+ var typeReq = req._tunnel && req._tunnel.typeReq,
+ instance = {};
+
+ if (!typeReq) {
+ return next();
+ }
+
+ if (!typeReq.type) {
+ return sendError(res, 'Not found: ' + req.url, 500);
@caridy
caridy Mar 22, 2013 Collaborator

same as above

@caridy caridy commented on an outdated diff Mar 22, 2013
lib/app/middleware/mojito-handler-tunnel-rpc.js
+ sendData(res, {error: msg}, (code || 500));
+}
+
+
+/**
+ * Exports a middleware factory that can handle RPC tunnel requests.
+ *
+ * @param {Object} config The configuration.
+ * @return {Function} The handler.
+ */
+module.exports = function (config) {
+ return function (req, res, next) {
+ var command = req.body,
+ instance = command.instance;
+
+ command.context = command.context || {};
@caridy
caridy Mar 22, 2013 Collaborator

move logic after the if

@caridy
Collaborator
caridy commented Mar 22, 2013

done with the first pass @ekashida

@ekashida ekashida added a commit to ekashida/mojito that referenced this pull request Mar 22, 2013
@ekashida ekashida implement feedback from pull #1044
- delay object creation for performance reasons
- use core library methods for parsing url/path
- fix http response code (500 => 404)
- eliminate `that = this` usage by closing over prefix variables directly
5539787
@ekashida ekashida added a commit to ekashida/mojito that referenced this pull request Mar 22, 2013
@ekashida ekashida implement feedback from pull #1044
- delay object creation for performance reasons
- use core library methods for parsing url/path
- fix http response code (500 => 404)
- eliminate `that = this` usage by closing over prefix variables directly
3b24f15
@ekashida ekashida added a commit to ekashida/mojito that referenced this pull request Mar 22, 2013
@ekashida ekashida aggregate new middleware pieces via composition
As suggested in pull #1044.

We definitely need to refactor all the middleware and clean things up.
07d0415
@ekashida
Contributor

@caridy I think I've addressed all your feedback, although I'm still getting those weird unit test errors. I'll keep looking at them...

@caridy caridy commented on an outdated diff Mar 25, 2013
lib/app/middleware/mojito-handler-tunnel-rpc.js
+
+ command = req.body;
+ command.context = command.context || {};
+
+ // When switching from the client context to the server context, we
+ // have to override the runtime.
+ command.context.runtime = 'server';
+
+ // All we need to do is expand the instance given within the RPC call
+ // and attach it within a "tunnelCommand", which will be handled by
+ // Mojito instead of looking up a route for it.
+ config.store.expandInstance(
+ command.instance,
+ command.context,
+ function (err, instance) {
+ // Replace with the expanded instance.
@caridy
caridy Mar 25, 2013 Collaborator

If err is detected, we should call next(err) at least.

@caridy caridy commented on the diff Mar 25, 2013
lib/app/middleware/mojito-handler-tunnel-parser.js
+ path = path.replace(staticPrefix, '')
+ .replace(tunnelPrefix, '');
+
+ if (path) {
+ // Get the basename without the .json extension.
+ name = libpath.basename(path, '.json');
+
+ // Time to get dirty.
+ parts = path.split('/');
+
+ // Get the mojit type.
+ type = parts[1];
+
+ // "Spec" tunnel request
+ if (parts[parts.length - 2] === 'specs') {
+ req._tunnel.specsReq = {
@caridy
caridy Mar 25, 2013 Collaborator

this is a bold assumption, what is req._tunnel is not set? Why is this middleware coupled with others?

@caridy caridy commented on an outdated diff Mar 25, 2013
lib/app/middleware/mojito-handler-tunnel-specs.js
+module.exports = function (config) {
+ return function (req, res, next) {
+ var specsReq = req._tunnel && req._tunnel.specsReq,
+ instance,
+ type,
+ name;
+
+ if (!specsReq) {
+ return next();
+ }
+
+ type = specsReq.type;
+ name = specsReq.name;
+
+ if (!type || !name) {
+ return req._tunnel.sendError(res, 'Not found: ' + req.url, 404);
@caridy
caridy Mar 25, 2013 Collaborator

same as above, bold assumption. I think we should call next(new Error()) instead.

@caridy caridy commented on an outdated diff Mar 25, 2013
lib/app/middleware/mojito-handler-tunnel-type.js
+
+ if (!typeReq.type) {
+ return req._tunnel.sendError(res, 'Not found: ' + req.url, 404);
+ }
+
+ instance = {
+ type: typeReq.type
+ };
+
+ config.store.expandInstanceForEnv(
+ 'client',
+ instance,
+ req.context,
+ function (err, data) {
+ if (err) {
+ return req._tunnel.sendError(
@caridy
caridy Mar 25, 2013 Collaborator

same as above.

@caridy caridy commented on an outdated diff Mar 25, 2013
lib/app/middleware/mojito-handler-tunnel-type.js
+ * Exports a middleware factory that can handle type tunnel requests.
+ *
+ * @param {Object} config The configuration.
+ * @return {Function} The handler.
+ */
+module.exports = function (config) {
+ return function (req, res, next) {
+ var typeReq = req._tunnel && req._tunnel.typeReq,
+ instance;
+
+ if (!typeReq) {
+ return next();
+ }
+
+ if (!typeReq.type) {
+ return req._tunnel.sendError(res, 'Not found: ' + req.url, 404);
@caridy
caridy Mar 25, 2013 Collaborator

same as above.

@caridy caridy commented on an outdated diff Mar 25, 2013
lib/app/middleware/mojito-handler-tunnel.js
+module.exports = function (config) {
+ var parser = require('./mojito-handler-tunnel-parser')(config),
+ rpc = require('./mojito-handler-tunnel-rpc')(config),
+ specs = require('./mojito-handler-tunnel-specs')(config),
+ type = require('./mojito-handler-tunnel-type')(config);
+
+ return function (req, res, next) {
+ var middleware = [
+ parser,
+ rpc,
+ specs,
+ type
+ ];
+
+ // Helper methods.
+ req._tunnel = {
@caridy
caridy Mar 25, 2013 Collaborator

If this is not used in this middleware, it should not be defined here.

ekashida added some commits Mar 21, 2013
@ekashida ekashida refactor tunnel middleware into two phases
By splitting the tunnel logic into a detection phase and a handling phase,
applications are free to override the handling logic.

In order to maintain backwards-compatibility, instead of refactoring the
existing tunnel middleware, a tunnel-demux middleware to detect the type of
tunnel request, and tunnel-(rpc|specs|type) middleware to handle each type of
request have been added. Mojito itself will now use these new middleware in
place of the single tunnel middleware, which will remain for those applications
that are specifying it in their configuration.
9ed57eb
@ekashida ekashida implement feedback from pull #1044
- delay object creation for performance reasons
- use core library methods for parsing url/path
- fix http response code (500 => 404)
- eliminate `that = this` usage by closing over prefix variables directly
14dc572
@ekashida ekashida clean up the logic around path inspection
- Replaced the brittle `trimSlash` stuff with `require('path').normalize`.
9112208
@ekashida ekashida replace tunnel middleware with refactored version
- Remove trailing slashes from tunnel/static prefixes.
- Add flag and flag check to determine when to stop processing the request.
31295f4
@ekashida ekashida fix bug where configured prefixes can be undefined ea75f3a
@ekashida ekashida fix test mock and remove unused variables 68ac17c
@ekashida ekashida execute tunnel middleware as an async queue 8de521c
@ekashida ekashida streamline middleware error handling
Uses the convention where a middleware with an arity of 4 is assumed to be the
error handler.
d4d3c6d
@ekashida ekashida initial unit test commit for refactored tunnel middleware ff14787
@ekashida ekashida simplify response logic with express response sugar
With unit tests!
77f731e
@ekashida ekashida merged commit 1bc4d3e into yahoo:develop Mar 26, 2013

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment