Implement a faster REST router #282

gunjpan · 2016-02-04T07:11:57Z

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

gunjpan · 2016-02-04T07:25:34Z

lib/rest-adapter.js

+    handler.methodName = sharedMethod.name;
+    handler.path = route.fullPath;
+    customRouter.registerPathAndHandlers({ verb: verb, fullPath: route.fullPath }, handler);
+  }
  router[verb](route.path, handler);


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

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

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

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.

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

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.

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"

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

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.

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?

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

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

bajtos · 2016-02-04T11:00:31Z

@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 · 2016-02-05T11:20:19Z

@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 #282 (comment).

bajtos · 2016-02-08T12:24:38Z

lib/rest-adapter.js

+
+  if (fastRouter.canRegister(route, handler)) {
+    handler.methodName = sharedMethod.name;
+    fastRouter[verb](route, handler);


I think adding extra properties on the handler method makes it more difficult to understand what's going on, because the other parts of the code are depending on the fact that fn.methodName is defined, which is not obvious.

That's why I proposed an API that is an extension of the original Express API:

// express: router[verb](route, handler) // proposed extension: router[verb](route, handler, options)

OTOH, this is a minor detail. Please considered my comment, and if you still feel that handler.methodName is better, then let's do it your way.

If you decide to keep the name on the handler function, then please choose a more descriptive property name. The problem with methodName is that it's not clear whether it refers to an HTTP method (like GET) or a shared/remote method (like Product.create).

@bajtos : I will set it to sharedMethod.stringName thus it would be clear if it's a prototype method or a static method, and as you recommended, will use displayName as:
handler.displayName = sharedMethod.stringName
Is that fine?

richardpringle · 2016-05-25T19:41:06Z

@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 · 2016-05-25T19:42:12Z

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

bajtos · 2016-05-26T08:43:43Z

lib/rest-adapter.js

@@ -425,6 +433,11 @@ RestAdapter.prototype._registerMethodRouteHandlers = function(router,
    // Express 4.x only supports delete
    verb = 'delete';
  }
+
+  // if (fastRouter.canRegister(route, handler)) {


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.

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.

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.

👍

bajtos · 2016-05-26T09:16:08Z

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 · 2016-05-26T09:18:36Z

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 · 2016-05-26T13:38:15Z

lib/rest-router.js

+  }
+};
+
+// For now: exclude paths with parameters like :id


^ remove this

richardpringle · 2016-05-26T13:51:29Z

@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 · 2016-05-26T13:59:13Z

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 · 2016-05-26T14:07:24Z

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 · 2016-05-26T14:14:10Z

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.)

richardpringle · 2016-05-26T14:19:34Z

lib/trie.js

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


array.prototype.find is not an ES5 function

bajtos · 2016-12-01T15:46:03Z

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

gunjpan · 2016-12-01T16:14:40Z

@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 · 2016-12-07T14:02:50Z

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

bajtos · 2017-01-19T14:40:25Z

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

gunjpan added the #review label Feb 4, 2016

gunjpan assigned bajtos Feb 4, 2016

gunjpan mentioned this pull request Feb 4, 2016

Implement vertical slice: Custom router #277

Closed

gunjpan reviewed Feb 4, 2016
View reviewed changes

bajtos assigned gunjpan and unassigned bajtos Feb 4, 2016

gunjpan changed the title ~~Implement rest router from POC.~~ Implement a faster REST router Feb 4, 2016

bajtos reviewed Feb 8, 2016
View reviewed changes

richardpringle assigned bajtos and unassigned gunjpan May 25, 2016

bajtos reviewed May 26, 2016
View reviewed changes

richardpringle reviewed May 26, 2016
View reviewed changes

lib/rest-router.js

}

};

// For now: exclude paths with parameters like :id

Copy link

Contributor

richardpringle May 26, 2016

Choose a reason for hiding this comment

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

^ remove this

richardpringle reviewed May 26, 2016
View reviewed changes

Richard Pringle added 3 commits May 26, 2016 11:00

Replace find() with filter() for ES5 support.

eff2b94

Add parameter to request object.

94166a0

Touch up console warn message.

c79b9e6

bajtos assigned gunjpan and unassigned bajtos Dec 1, 2016

cgole removed the #review label Jan 7, 2017

bajtos closed this Jan 19, 2017

bajtos mentioned this pull request Dec 14, 2017

[OpenAPI] Switch to OpenAPI v3 for spec and routing loopbackio/loopback-next#753

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement a faster REST router #282

Implement a faster REST router #282

gunjpan commented Feb 4, 2016

gunjpan Feb 4, 2016

gunjpan Feb 4, 2016

bajtos Feb 4, 2016

bajtos Feb 4, 2016

bajtos Feb 4, 2016

bajtos Feb 8, 2016

gunjpan Feb 8, 2016

bajtos Feb 8, 2016

gunjpan Feb 9, 2016

bajtos Feb 9, 2016

bajtos commented Feb 4, 2016

bajtos commented Feb 5, 2016

bajtos Feb 8, 2016

bajtos Feb 8, 2016

gunjpan Feb 8, 2016

richardpringle commented May 25, 2016

richardpringle commented May 25, 2016

bajtos May 26, 2016

richardpringle May 26, 2016

bajtos May 26, 2016

bajtos commented May 26, 2016

bajtos commented May 26, 2016

richardpringle May 26, 2016

richardpringle commented May 26, 2016

richardpringle commented May 26, 2016

bajtos commented May 26, 2016

bajtos commented May 26, 2016

richardpringle May 26, 2016

bajtos commented Dec 1, 2016

gunjpan commented Dec 1, 2016

richardpringle commented Dec 7, 2016

bajtos commented Jan 19, 2017

Implement a faster REST router #282

Implement a faster REST router #282

Conversation

gunjpan commented Feb 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 4, 2016

bajtos commented Feb 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardpringle commented May 25, 2016

richardpringle commented May 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented May 26, 2016

bajtos commented May 26, 2016

Choose a reason for hiding this comment

richardpringle commented May 26, 2016

richardpringle commented May 26, 2016

bajtos commented May 26, 2016

bajtos commented May 26, 2016

Choose a reason for hiding this comment

bajtos commented Dec 1, 2016

gunjpan commented Dec 1, 2016

richardpringle commented Dec 7, 2016

bajtos commented Jan 19, 2017