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.

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

Implement a faster REST router #282

Closed
wants to merge 13 commits into from
17 changes: 15 additions & 2 deletions lib/rest-adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ var async = require('async');
var HttpInvocation = require('./http-invocation');
var ContextBase = require('./context-base');
var HttpContext = require('./http-context');
var RestRouter = require('./rest-router');

var json = bodyParser.json;
var urlencoded = bodyParser.urlencoded;
Expand Down Expand Up @@ -263,6 +264,10 @@ RestAdapter.prototype.createHandler = function() {

var handleUnknownPaths = this._shouldHandleUnknownPaths();

// Mount rest-router
var fastRouter = new RestRouter();
root.use(fastRouter);

classes.forEach(function(restClass) {
var router = express.Router();
var className = restClass.sharedClass.name;
Expand All @@ -277,6 +282,8 @@ RestAdapter.prototype.createHandler = function() {
var sharedMethod = restMethod.sharedMethod;
debug(' method %s', sharedMethod.stringName);
restMethod.routes.forEach(function(route) {
// add fullPath for Trie insertion
route.fullPath = joinPaths(restClass.getPath(), route.path);
methods.push({ route: route, method: sharedMethod });
});
});
Expand All @@ -285,7 +292,7 @@ RestAdapter.prototype.createHandler = function() {
methods.sort(sortRoutes);

methods.forEach(function(m) {
adapter._registerMethodRouteHandlers(router, m.method, m.route);
adapter._registerMethodRouteHandlers(router, m.method, m.route, fastRouter);
});

if (handleUnknownPaths) {
Expand Down Expand Up @@ -414,7 +421,8 @@ RestAdapter.errorHandler = function(options) {

RestAdapter.prototype._registerMethodRouteHandlers = function(router,
sharedMethod,
route) {
route,
fastRouter) {
var handler = sharedMethod.isStatic ?
this._createStaticMethodHandler(sharedMethod) :
this._createPrototypeMethodHandler(sharedMethod);
Expand All @@ -425,6 +433,11 @@ RestAdapter.prototype._registerMethodRouteHandlers = function(router,
// Express 4.x only supports delete
verb = 'delete';
}

// if (fastRouter.canRegister(route, handler)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's reasonable to expect that fastRouter will support all different route flavours supported by express (e.g. regular expressions). That's why it's important to call canRegister (or perhaps canHandle is a better name?) here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you look at the code for canRegister? It only registers with express if the handler function has more than 3 arguments or if there is a ':' in the path (in other words, any route with a parameter).

So I just want to be clear and say that you are not suggesting to use the function that exists but instead to make sure that the router can handle the route, otherwise pass it to the express router.

Note: this should be done with an if else statement instead of just an if.

Copy link
Member

Choose a reason for hiding this comment

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

So I just want to be clear and say that you are not suggesting to use the function that exists but instead to make sure that the router can handle the route, otherwise pass it to the express router.

Correct.

Note: this should be done with an if else statement instead of just an if.

👍

handler.methodName = sharedMethod.name;
fastRouter[verb](route, handler);
// }
router[verb](route.path, handler);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This if block (L417-421) is causing trouble :( All tests with npm test passed on local, CI fails

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving L422 in else block of L417 causes failures on local. We need to define a clear strategy to port custom router.

Copy link
Member

Choose a reason for hiding this comment

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

I would expect that L422 is not invoked when the method was registered with the new fast router. I'll check.

Copy link
Member

Choose a reason for hiding this comment

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

Here is a clue - I added some logging to RestRouter.

$ mocha -g "strong-remoting-rest call of constructor method should allow arguments in the query"

  strong-remoting-rest
    call of constructor method
REGISTER all /testclass
HANDLE get /testclass/
      1) should allow arguments in the query

Please add debug() logs to both registerPathAndHandlers and handle so that users can easily get logs like that without any code changes. Take a look at how express.Router logs debug messages and add similar logs to your new router.

Copy link
Member

Choose a reason for hiding this comment

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

So the thing is, the handler is registered for all verbs on /testclass. The request for get /testclass does not recognise this handler.

Copy link
Member

Choose a reason for hiding this comment

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

I could not find this comment on GH, so I am responding here.

the built-in router places all verb handler on the top of the handler stack, and keeps a flag to check if the path has an all handler, if yes, it > invokes the all handler first and runs other registered handlers> In our case, trie has a placeholder for all handler, and we can check, but we are only inovking single handler.
That makes perfect sense to me, now we just need to improve error logging so that the user understands what's happening under the hood.
 

  1. If there is an "all" handler already registered for a path, and users is attempting to add another handler, then we should do the same thing we do now when the second handler is registered for the same "path+verb"
     
  2. When registering an "all" handler, check if there are any existing verb-specific handlers for the same path. If there are, then report a warning for each of them, since they won't be invoked.
     
    BTW are you sure the "all" verb handler is on the top? I would expect it's at the bottom, i.e. more specific handlers (GET, POST) are run before generic ones (ALL).
    I'm thinking of changing it in a way that:> in all: handler(req,res,next) , next points to the second handler in the stack for the path and so on.. I will commit this approach in the next commit.> Or, should you have any better idea for this scenario -- after looking at recent commit -- please advise. Thank you :)
    AFAIK, all strong-remoting handler functions either send back the response (and never call next), or fail with an error and call next(err). In both cases the flow control skips all subsequent handler functions.
     
    This is why we keep only the first handler for each path+verb (instead of an array), and that's why I believe we don't need to chain handlers for "all" verb either. Am I missing something?
     
    Miroslav

Copy link
Contributor Author

Choose a reason for hiding this comment

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

BTW are you sure the "all" verb handler is on the top? I would expect it's at the bottom, i.e. more specific handlers (GET, POST) are run before generic ones (ALL).

Yes, I'm pretty sure about it from express API docs:
http://expressjs.com/en/4x/api.html#router.all

and it's also the matter of registering the router.all() first to execute it first in middleware chain.

Copy link
Member

Choose a reason for hiding this comment

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

As I understand docs, all is treated the same way as other, regular verbs. If you put your route handler as the first one, it will work as you describe, but if you put it as the last one, it will work as I described it. So the task for you is to find how strong-remoting is dealing with this. IIRC there is a custom sort of routes, does it take verbs into account too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos : strong-remoting sorts routes in the following custom way: It first sorts by verb and then sorts path-part by part.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. And because "all" starts with the first letter in the alphabet, it will end up being registered as the first one.
 
To be honest, this looks like a bug to me, but I am not sure if it's a good idea to fix it now, when we are possibly introducing other breaking changes caused by the new router implementation.
 
Could you please leave a comment in the code, explaining why we are treating "all" verb handlers as the first ones and open a GH issue to track the task of changing this to the more sensible approach where "all" becomes a fall-back only? The code comment should link to this new issue too.
 
Thanks,
Miroslav

};

Expand Down
59 changes: 59 additions & 0 deletions lib/rest-router.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@

var url = require('url');
var http = require('http');
var Trie = require('./trie');

var HTTP_METHODS = require('methods');

var RestRouter = module.exports = function(options) {
var opts = options || {};
var router = function(req, res, next) {
router.handle(req, res, next);
};

router.__proto__ = RestRouter;
router._trie = new Trie();
router.options = opts;

return router;
};

// express style API for registering route handlers: ex. route[method](path, handler)
// add verb:handler for path to trie DS
HTTP_METHODS.concat('all').forEach(function(method) {
RestRouter[method] = function(route, handler) {
route.fullPath = normalizePath(route.fullPath, this.opts);
var trie = this._trie;
trie = trie.add(route, handler);
return this;
};
});

// handle request, match path and if found, invoke handler method
RestRouter.handle = function(req, res, next) {
var trie = this._trie;
var verb = req.method.toLowerCase();
var path = normalizePath(req.url, this.options);
var methods = trie.find(path);

if (methods && verb in methods) {
methods[verb](req, res, next);
} else {
next();
}
};

// For now: exclude paths with parameters like :id
Copy link
Contributor

Choose a reason for hiding this comment

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

^ remove this

// and handler functions with err obj i.e. (err, req, res, next)
Copy link
Member

Choose a reason for hiding this comment

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

Is this comment still relevant?

Copy link
Contributor

@richardpringle richardpringle May 26, 2016

Choose a reason for hiding this comment

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

I believe so

Copy link
Member

Choose a reason for hiding this comment

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

You commented-out if (router.canRegister)` , therefore no paths/handlers are excluded now AFAICT. I find this comment confusing, because it describes situation before you made your changes and seems to be in contradiction with the current version of the code.


// RestRouter.canRegister = function(route, handler) {
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned earlier, we should still preserve this method.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to re-write this method.

Copy link
Member

Choose a reason for hiding this comment

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

+1

// return !(route.path.includes(':') && handler.length < 4);
// };

function normalizePath(path, options) {
path = url.parse(path).pathname;
if (!options || !(options.caseSensitive)) {
path = path.toLowerCase();
}
return path;
}
70 changes: 70 additions & 0 deletions lib/trie.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@

module.exports = Trie;

function Trie() {
this.methods = {};
}

// insert new nodes to Trie
Trie.prototype.add = function(route, handler) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd personally use verb + fullPath arguments instead of route, e.g.

Trie.prototype.add = function(verb, fullPath, handler) {
  // ...
};

That way the Trie is not coupled with strong-remoting's routes and also it's more obvious what data is needed for add.

var node = this;
var parts = getPathParts(route.fullPath);
var segment;

if (!parts.length) {
addVerbHandler(node, route, handler);
return this;
}
for (var i = 0; i < parts.length; i++) {
segment = parts[i];
if (!(segment in node)) {
node[segment] = new Trie();
node.param = (segment[0] === ':');
}
if (i + 1 === parts.length) {
addVerbHandler(node[segment], route, handler);
}
node = node[segment];
}

return this;
};

// Look for matching path and return methods for a match
Trie.prototype.find = function(path) {
Copy link
Member

Choose a reason for hiding this comment

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

Since find is accepting both path+verb, it's better to modify find to accept both path+verb too, for the sake of consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos: In our implementation, find is only accepting the path as it's being called by RestRouter.handle(). find returns a methods object which has all {verb:handlers} registered for that path.
I'm using RestRouter.handle() to do the matching work for verb. This way, we can keep our matching criteria for verb+path (such as, we may need to handle multiple handler for same verb+path) separated from trie implementation.

For the sake of consistency, I've changed the trie.add(path, verb, handler) to trie.add(route, handler)
and have kept trie.find(path) as it is.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC I was commenting earlier that I would like this to become Trie.prototype.find = function(path, verb).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bajtos : yes, and I commented on an outdated difference here.

var node = this;
var parts = getPathParts(path);
var segment;

for (var i = 0; i < parts.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

btw this can be simplified to for (var i in parts)

segment = parts[i];
if (!(segment in node) && !node.param) return false;
if (node.param) {
segment = Object.keys(node).find(function(key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

array.prototype.find is not an ES5 function

if (key[0] === ':') return key;
});
return node[segment].methods;
}
if (i + 1 === parts.length) return node[segment].methods;
node = node[segment];
}
};

function getPathParts(path) {
return path.trim().split('/').filter(Boolean);
}

function addVerbHandler(node, route, handler) {
node.methods = node.methods || {};
var verb = route.verb;
if (node.methods[verb]) {
console.warn(
'WARN: A handler was already registered for %s /api%s : Ignoring the new handler %s',
route.verb.toUpperCase(),
route.fullPath || '/[unknown]',
Copy link
Member

Choose a reason for hiding this comment

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

I'd personally use [unknown path] instead of /[unknown] - I think the former makes it more clear what is unknown.

handler.methodName || '<unknown>'
Copy link
Member

Choose a reason for hiding this comment

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

Storing metadata on the handler is not great, can you pass it in addVerbHandler arguments instead?

Copy link
Member

Choose a reason for hiding this comment

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

I'd use <anonymous> as the default method name, that's what V8 uses.

> (function() { throw new Error(); })()
Error
    at repl:1:22
    at repl:1:37
    at REPLServer.self.eval (repl.js:110:21)
    at Interface.<anonymous> (repl.js:239:12)
    at Interface.emit (events.js:95:17)
    at Interface._onLine (readline.js:203:10)
    at Interface._line (readline.js:532:8)
    at Interface._ttyWrite (readline.js:761:14)
    at ReadStream.onkeypress (readline.js:100:10)
    at ReadStream.emit (events.js:98:17)

);
return;
}
node.methods[verb] = handler;
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
"jayson": "^1.2.0",
"js2xmlparser": "^0.1.9",
"loopback-phase": "^1.3.0",
"methods": "^1.1.2",
"mux-demux": "^3.7.9",
"qs": "^2.4.2",
"request": "^2.55.0",
Expand Down
93 changes: 93 additions & 0 deletions test/rest-router.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
var assert = require('assert');
var expect = require('chai').expect;
var RestRouter = require('../lib/rest-router.js');
var Trie = require('../lib/trie.js');
var express = require('express');
var supertest = require('supertest');
var http = require('http');

describe('RestRouter', function() {
// Test init
it('initializes with Trie datastructure', function(done) {
var router = new RestRouter();
expect(router).to.have.property('_trie');
expect(router._trie).to.be.instanceOf(Trie);
done();
});

// test handle
it('handles a request', function(done) {
var app = express();
var router = new RestRouter();
var route = { fullPath: '/example', verb: 'get' };
var resBody = 'Hello Wold!';
// this doesn't make sense to me that I have to call router.get(...)
// as well as have route.verb = get
// it doesn't work without it
router.get(route, function(req, res) {
res.end(resBody);
});
app.use(router);
console.log(router._trie);
supertest(app)
.get(route.fullPath)
.expect(200)
.expect(resBody)
.end(function(err, res) {
if (err) return done(err);
done();
});
});

// test handle param
it('handles a request with a parameter', function(done) {
var app = express();
var router = new RestRouter();
var route = { fullPath: '/example/:id', verb: 'get' };
var id = '1';
// this doesn't make sense to me that I have to call router.get(...)
// as well as have route.verb = get
// it doesn't work without 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 should be fixed.

router.get(route, function(req, res, next) {
console.log('hit');
res.end(id);
Copy link
Member

Choose a reason for hiding this comment

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

This test always passes, because you always return id from the variable. You need to return req.params.id instead.

Copy link
Member

Choose a reason for hiding this comment

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

In TDD, it's crucial to start with a failing unit test, to see the test fail and fail for the right (expected) reason. Without that, it's easy to write a test that always passed (like this one) or a test that fails for completely different reasons because it's based on wrong assumptions about how the code under test works.

Last but not least, seeing the test fail is the opportunity to verify that the failure message makes it clear what went wrong, what was expected and actual result.

Copy link
Contributor

@richardpringle richardpringle May 26, 2016

Choose a reason for hiding this comment

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

I understand how TDD works. The test was originally failing because GET /example/1 was not a registered route, /:id was not being replaced by the parameter, the route originally expected the string ':id' instead of '1'. I was not testing to see if req.params.id was being populated properly.

I do however agree that it makes sense to test this here as well.

});
app.use(router);
console.log(router._trie);
supertest(app)
Copy link
Member

Choose a reason for hiding this comment

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

Please simplify the test setup to avoid repetition of the boiler plate setting up an app with a fast router.

IIRC, each call of supertest(app) creates a new http server and binds to a new port, which slows down tests especially on Windows. A better solution is to create a single http server and share it by all tests, see

var server; // eslint-disable-line one-var
function setupRemoteServer(done) {
var app = express();
app.use(function(req, res, next) {
// create the handler for each request
ctx.remoteObjects.handler('rest').apply(ctx.remoteObjects, arguments);
});
server = app.listen(0, '127.0.0.1', function() {
ctx.request = supertest('http://127.0.0.1:' + this.address().port);
done();
});
server.on('error', done);
}
function stopRemoteServer() {
server.close();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. I never planned on leaving the repeated code in each test. I wanted to make sure I was doing this right before I removed it.

.get(route.fullPath.replace(':id', id))
.expect(200)
.expect(id)
.end(function(err, res) {
console.log(res.body);
if (err) return done(err);
done();
});
});

// test handle param with method after
// it('handles a request for a instance method', function() {
// var app = express();
// var router = new RestRouter();
// var route = { fullPath: '/example/:id/properties', verb: 'get' };
// var id = '1';
// // this doesn't make sense to me that I have to call router.get(...)
// // as well as have route.verb = get
// // it doesn't work without it
// router.get(route, function(req, res, next) {
// // List the properties of a mock object
// res.end(req.fullPath);
// });
// app.use(router);
// console.log(router._trie);
// supertest(app)
// .get(route.fullPath.replace(':id', id))
// .expect(200)
// .expect(id)
// .end(function(err, res) {
// console.log(res.body);
// if (err) return done(err);
// done();
// });
// });
});
76 changes: 76 additions & 0 deletions test/trie.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@

var assert = require('assert');
var expect = require('chai').expect;
var Trie = require('../lib/trie');

describe('Trie', function() {
var trie, route;

describe('trie.add()', function() {
beforeEach(function() {
trie = new Trie();
route = { fullPath: '', verb: '' };
});

it('has empty root node with only methods key', function() {
expect(trie).to.have.keys('methods');
});

it('registers hanlder for path: / to root node', function() {
trie.add(setRoute(route, '/', 'get'), 'rootHanlder()');
expect(trie.methods).to.have.keys('get');//{ 'get': 'rootHandler()' });
});

it('create a node for a path with single part, no verb-handler', function() {
trie.add(setRoute(route, '/Planets'));
expect(trie).to.have.property('Planets');
expect(trie.Planets).to.have.property('methods');
});

it('registers handler for path: /Planets', function() {
trie.add(setRoute(route, '/Planets', 'get'), 'handler()');
expect(trie).to.have.deep.property('Planets.methods.get', 'handler()');
});

it('registers handler for path: /Planets/count', function() {
trie.add(setRoute(route, '/Planets/count', 'get'), 'count()');
expect(trie).to.have.deep.property('Planets.count.methods.get', 'count()');
});

it('registers different HTTP verb handlers for the same path ', function() {
trie.add(setRoute(route, '/Planets', 'get'), 'getHandler()');
expect(trie).to.have.deep.property('Planets.methods.get', 'getHandler()');
trie.add(setRoute(route, '/Planets', 'post'), 'postHandler()');
expect(trie).to.have.deep.property('Planets.methods.post', 'postHandler()');
trie.add(setRoute(route, '/Planets', 'delete'), 'deleteHandler()');
expect(trie).to.have.deep.property('Planets.methods.delete', 'deleteHandler()');
});

it('While registering more than one handler for one verb+path,' +
' preserves the handler which was registered first', function() {
trie.add(setRoute(route, '/Planets', 'get'), 'getHandler()');
expect(trie).to.have.deep.property('Planets.methods.get', 'getHandler()');
trie.add(setRoute(route, '/Planets', 'get'), 'anotherGetHandler()');

expect(trie).to.have.deep.property('Planets.methods.get')
.to.not.equal('anotherGetHandler()');
});
});

describe('trie.find()', function() {
it('should return handler for matching path', function() {
expect(trie).to.have.property('Planets');
trie.add(setRoute(route, '/Planets/count', 'get'), 'getCount()');
trie.add(setRoute(route, '/Planets/count', 'post'), 'postCount()');
var handlers = trie.find('/Planets/count');
expect(handlers.get).to.be.equal('getCount()');
expect(handlers.post).to.be.equal('postCount()');
});
});
});

function setRoute(route, fullPath, verb) {
Copy link
Member

Choose a reason for hiding this comment

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

I find this a bit weird. Why can't you simply use a factory function?

function aRoute(verb, fullPath) {
  return {
    verb: verb,
    fullPath: fullPath
  };
});

Copy link
Contributor

@richardpringle richardpringle Jun 7, 2016

Choose a reason for hiding this comment

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

The above is irrelevant if you pass in verb,path, andhandler` to necessary functions as suggested in this comment.

route.fullPath = fullPath,
route.verb = verb;
return route;
}