Implement a faster REST router

[gunjpan]

Faster REST router implementation - iteration 1 Includes:

• Handler method registration and invocation for simple REST paths i.e. /api/[model] & /api/[model]/[method]

Excludes:

• Handler method registration and invocation for REST paths with :parameters e.x. /api/[model]/:id
• Handlers with more than three arguments for example, handlerFn(err, req, res, next){..}

Connect to https://github.com/strongloop-internal/scrum-loopback/issues/730

[bajtos]

@gunjpan please review my comments in our earlier conversations (e.g. in the Spike/POC pull request) and add all unit-tests that I mentioned there. It's ok to add them as "it.skip" initially, I just want to make sure we don't forget about any of them.

[bajtos]

@gunjpan on more task from our earlier conversations: please add the test suite that will run the same code (tests) against both express.Router and RestRouter. The sooner you do that, the sooner we can start adding tests for edge cases like https://github.com/strongloop/strong-remoting/pull/282#discussion_r51957509.

[richardpringle]

I want to propose a different name for Trie and trie.js. Maybe something a little more descriptive like RouteTrie, thoughts?

When I see Trie I immediately assume that the class is simply the trie data structure and is not necessarily related to routing.

[bajtos]

Maybe something a little more descriptive like RouteTrie, thoughts?

It may be better to treat the fact that we use Trie as an implementation detail and don't leak it in the name at all. Then we can call the class RouteTable or RouterLookup or something along those lines. Thoughts?

[richardpringle]

@bajtos, is there any reason not to be transparent? Someone from the community might see it and have a better idea...

I'm not picky about that detail though, I like RouteTable as well.

[bajtos]

cc @raymondfeng PTAL too

[bajtos]

is there any reason not to be transparent? Someone from the community might see it and have a better idea...

So what will happen if we decide to change the implementation later in the future and use a different data structure? Will we have a RouteTrie that's implemented using red-black tree (for example)? Will we rework all code that is using (calling) this class, even though there was no API change beyond the name?

That's why I am proposing to use a name that describes how the class is intended to be used (to look up routes), not how it is implemented (via Trie now). The former should stay the same even in case of changes in the future, the latter will not stay the same.

[richardpringle]

@bajtos, this is still very much a work in progress and there are still a few changes that need to be made that you had originally recommended for @gunjpan, but I would like you to please review before I continue to work on this.

I would also like your opinion on any more test cases that should be added.

[richardpringle]

@gunjpan @davidcheung, could you also please take a look.

[bajtos]

this is still very much a work in progress and there are still a few changes that need to be made that you had originally recommended for @gunjpan, but I would like you to please review before I continue to work on this.

I am not sure what exactly to look out for during review. I did a quick check to see if there is anything obvious and left few comments above.

I would also like your opinion on any more test cases that should be added.

For me, it's most important to have an integration test suite to verify that the new fast router behaves exactly same as the default express router.

I think the best approach is to create a parameterized suite that will be run against the new fastRouter and against the Express router. For example, something along these lines:

describe('fast router', function() {
  testRouter(FastRouter);
});

describe('express Router', function() {
  testRouter(express.Router);
});

function testRouter(Router) {
  var router, request;
  before(setupHttpServer);
  after(stopHttpServer);

  // individual tests calling the provided `router` under the hood
  it('...', function(done) {
    router.get('/', function(req, res, next) {
      // ...
    });
    request.get('/')
      .expect(..., done);
  });

  function setupHttpServer(done) {
    router = new Router();
    // create http server handling requests via router
    // create supertest instance invoking this server
  }
}

As for test cases, we should test all HTTP verbs, then different URL paths (/, path with one segment, path with multiple segments, path with a trailing /, check how double/triple slash is handled - at the start //api, in the middle /api//cars, and at the end /api/cars//). Check how all is handled - what is the priority compared to specific verbs like get/post. Look at express source code dealing with path parameters and add test cases to verify we correctly handle edge cases.

[bajtos]

We should also verify that when you register a handler for a given verb+path, other endpoints will not trigger the handler.

Verify the priority of paths with path parameters vs. paths with static names, think about GET /api/cars/:id vs. GET /api/cars/findOne: /api/cars/1 should be routed to findById(), /api/cars/findOne should be routed to findOne().

[richardpringle]

@bajtos

I am not sure what exactly to look out for

I wanted you to look out for the things that you found. It's better to catch those mistakes early instead of having to do a big re-factor at the end. It's a much faster process to review as I move forward instead of one big repeating review/re-factor cycle at the end.

[richardpringle]

Also, I keep getting the same failure for multiple tests on Node 0.10 and 0.12

     TypeError: undefined is not a function
      at Trie.find (lib/trie.js:9:2363)
      at Function.RestRouter.handle (lib/rest-router.js:9:1870)
      at router (lib/rest-router.js:9:688)
      at corsHandler (lib/rest-adapter.js:9:13002)
      at lib/rest-adapter.js:9:11460
      at test/phase-handlers.test.js:20:30
      at HTTPParser.parserOnIncoming [as onIncoming] (_http_server.js:492:12)
      at HTTPParser.parserOnHeadersComplete (_http_common.js:111:23)
      at Socket.socketOnData (_http_server.js:343:22)
      at readableAddChunk (_stream_readable.js:163:16)
      at Socket.Readable.push (_stream_readable.js:126:10)
      at TCP.onread (net.js:540:20)

[bajtos]

Few more things to test: values of path parameters should be url-decoded, i.e. /api/cars/a+b should produce value 'a b', similarly for % based sequences.

[bajtos]

Also, I keep getting the same failure for multiple tests on Node 0.10 and 0.12

That's weird, because lib/trie.js:9 is this now:

Trie.prototype.add = function(route, handler) {

@richardpringle can you reproduce the problem locally on your machine too? (I personally use and recommend nvm for quick & easy switching between node versions.)

[bajtos]

@gunjpan I am re-assigning this back to you. Please ping me when there patch is ready for another round of review.

[gunjpan]

@bajtos : Thank you. I may need to spend some time to see what richard did, rebase and see which way to go. Will update you when done. Thanks.

[richardpringle]

@gunjpan, let me know if you need any help!

[bajtos]

I am closing this patch for now, we can re-open it when the time to work on this feature comes again.