feat(rest): Implement trie-based routing and tidy up path validation #1765

raymondfeng · 2018-09-26T21:12:55Z

The PR introduces the following improvements:

validate and normalize paths for openapi
add benchmark for routing
make the router pluggable
simplify app routing to plain functions
optimize simple route mapping and use regexp as default
add rest.router binding to allow customization
add docs for routing requests

Optimize routes without variables to use a direct lookup

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

virkt25

LGTM at a high level -- I do have some questions to better understand this PR. See comments below.

virkt25 · 2018-09-27T14:32:20Z

packages/rest/src/router/routing-table.ts

@@ -127,6 +128,8 @@ export class RoutingTable {
      );
    }
    this._routes.push(route);
+    // Sort routes
+    this._routes.sort(compareRoute);


What benefit do we get by sorting the routes?

Think about this example:

/** * A simple controller to bounce back http requests */ export class PingController { constructor(@inject(RestBindings.Http.REQUEST) private req: Request) {} @get('/ping/:me') pingMe(@param.path.string('me') me: string): object { return { greeting: `Hello from ${me}`, }; } // Map to `GET /ping` @get('/ping') ping(): object { return { greeting: 'Hello from LoopBack', date: new Date(), url: this.req.url, headers: Object.assign({}, this.req.headers), }; } @get('/ping/xyz') pingXyz(): object { return { greeting: 'Hello from XYZ', }; } }

I would expect GET /ping/xyz is routed to pingXyz() instead of pingMe(). Without this PR, it matches GET /ping/:me.

I'll add an integration test.

virkt25 · 2018-09-27T14:33:22Z

packages/openapi-v3/test/unit/to-openapi-path.unit.ts

+    expect(path).to.eql('/{foo}');
+  });
+
+  it('allows /:foo/bar', () => {


We're going to start supporting express style of parameters? If so, we should document it.

We didn't do any check before this PR. I think it will be good to allow express style too.

We didn't do any check before this PR. I think it will be good to allow express style too.

I would rather not introduce multiple parameter styles, I am concerned about the confusion it can create and the extra work it will add for any consumer of routes.

virkt25 · 2018-09-27T14:35:25Z

packages/openapi-v3/src/controller-spec.ts

@@ -80,7 +81,7 @@ function resolveControllerSpec(constructor: Function): ControllerSpec {

    const endpoint = endpoints[op];
    const verb = endpoint.verb!;
-    const path = endpoint.path!;
+    const path = toOpenApiPath(endpoint.path!);


Hmm ... why is path not a required property? (If it was we wouldn't need the !.

If path is missing, it's default to /'.

virkt25

LGTM. I'd like to see one more review on this before it's merged.
cc: @b-admike @jannyHou

b-admike

Nice work; I've left some comments, but LGTM overall.

b-admike · 2018-09-27T20:58:31Z

packages/rest/src/router/routing-table.ts

+export function compareRoute(
+  route1: Pick<RouteEntry, 'verb' | 'path'>,
+  route2: Pick<RouteEntry, 'verb' | 'path'>,
+) {


Can we have the return type here as number?

b-admike · 2018-09-27T20:59:17Z

packages/rest/src/router/routing-table.ts

+  route2: Pick<RouteEntry, 'verb' | 'path'>,
+) {
+  // First check verb
+  const verb1 = HTTP_VERBS[route1.verb.toLowerCase()] || 3;


For clarification, why do we set the default verb to patch here?

b-admike · 2018-09-27T21:02:50Z

packages/rest/src/router/routing-table.ts

+  }
+
+  // Now check token by token
+  for (let i = 0; i < tokensForPath1.length; i++) {


Do we have test coverage for this code path?

bajtos

I am not very happy about this change at high level. For medium to long term, we should abandon Express router and use a more performant alternative (probably based on Trie). This was planned all the way back in LoopBack pre-3.0 days, and we have a loopback-next issue keeping track of this work too - see #98.

I would prefer to switch to the new router first and only then start adding new features like express-styled parameters, to ensure we don't accidentally add an Express-specific feature that is difficult to support with the new router.

Having said that, the example where /ping/{me} takes precedence over /ping/xyz is clearly a bug (a violation of OpenAPI Spec) that should be fixed.

bajtos · 2018-10-01T13:36:12Z

packages/openapi-v3/test/unit/to-openapi-path.unit.ts

+    expect(path).to.eql('/{foo}');
+  });
+
+  it('allows /:foo/bar', () => {


We didn't do any check before this PR. I think it will be good to allow express style too.

I would rather not introduce multiple parameter styles, I am concerned about the confusion it can create and the extra work it will add for any consumer of routes.

raymondfeng · 2018-10-01T14:21:45Z

I can update the PR to report errors if Express styles are used for the GA. Here is what led me to create this PR:

I was playing with the Ping controller. As somebody familiar with Express, I used :id style. But our runtime does not complain and it causes problems in the generated OpenAPI spec.
I also found out that /ping/xyz was not routing the correct method if we have /ping/{me} and /ping/xyz.

raymondfeng · 2018-10-01T16:49:16Z

@bajtos I reverted the sorting based approach and implemented trie-based routing based on wayfarer. PTAL.

bajtos

Nice!

bajtos · 2018-10-01T17:27:19Z

packages/openapi-v3/src/controller-spec.ts

+  let tokens = pathToRegExp.parse(path);
+  if (tokens.some(t => typeof t === 'object')) {
+    throw new Error(
+      `Invalid path template: '${path}'. Please use {param} instead of ':param'`,


bajtos · 2018-10-01T17:29:02Z

packages/openapi-v3/src/controller-spec.ts

+ */
+export function toOpenApiPath(path: string = '/') {
+  validatePath(path);
+  return path.replace(/\:([A-Za-z0-9_]+)/g, '{$1}');


Is this replace call still needed, considering that validatePath rejects values containing express-style parameters? Maybe I am missing something?

packages/openapi-v3/test/unit/to-openapi-path.unit.ts

bajtos · 2018-10-01T17:32:50Z

packages/rest/src/router/routing-table.ts

+    // Add the route to the trie
+    const path = route.path.startsWith('/') ? route.path : `/${route.path}`;
+    const verb = route.verb.toLowerCase() || 'get';
+    this._router.on(`/${verb}${path}`, () => {});


Can we store route on the function passed to this._router.on to avoid costly lookup (iteration through this._routes later on? E.g.

// store this._router.on(`/${verb}${path}`, Object.assign(() => {}, {route})); // pick up - I hope that's the Wayfarer API const found = this._router.match(path); const route = found.route;

packages/rest/src/router/routing-table.ts

bajtos · 2018-10-01T17:34:25Z

packages/rest/test/acceptance/routing/routing.acceptance.ts

+        .expect(200, 'hello john');
+      await whenIMakeRequestTo(app)
+        .get('/greet/world')
+        .expect(200, 'hello WORLD');


I like this test much more than the previous version 👍

bajtos · 2018-10-01T17:35:11Z

Could you also update the benchmark to show the new performance with Wayfarer under the hood? See https://github.com/strongloop/loopback-next/tree/master/benchmark

b-admike

This looks awesome 👍

raymondfeng · 2018-10-01T18:12:12Z

@b-admike @bajtos I pushed more changes to use _router to store routes per @bajtos suggestion. PATL.

bajtos

I have few more comments I'd like to see addressed.

I don't want to block the progress though, so feel free to address them in a follow-up pull request if you prefer.

bajtos · 2018-10-02T10:48:38Z

packages/rest/src/router/routing-table.ts

-    debug(' -> found with params: %j', pathParams);
-
-    return createResolvedRoute(this, pathParams);
+    this.path = path.replace(/{([A-Za-z0-9_]*)}/g, ':$1');


This RegExp will convert /{} to /: - is that intentional? Could you please add a test for this case? I think we should preserve /{} as /{} and use + instead of * in the regular expression.

I see you are allowing only [A-Za-z0-9_] characters. Is this based on character list allowed by OpenAPI spec? Please add some tests to show what happens when a different character is used in the path param name, e.g. /{foo*} or /{jméno} ("jméno" is "name" in Czech)

bajtos · 2018-10-02T10:49:15Z

packages/rest/test/acceptance/routing/routing.acceptance.ts

+        }
+
+        greetWorld() {
+          return `hello WORLD`;


Let's use HELLO WORLD to make the distinction from greet more obvious. You can also use regular apostrophes instead of back-ticks.

bajtos · 2018-10-02T10:52:19Z

packages/rest/test/acceptance/routing/routing.acceptance.ts

+        }
+      }
+
+      const spec = anOpenApiSpec()


I think this test setup can be greatly simplified by using app.route API, see https://loopback.io/doc/en/lb4/Routes.html#using-partial-openapi-spec-fragments

app.route(new Route( 'get', '/greet/{name}', anOperationSpec().withParameter({name: 'name', in: 'path', type: 'string'}), (name: string) => `hello ${name}`, )); app.route(new Route( 'get', '/greet/world', anOperationSpec(), () => 'HELLO WORLD', ));

In case .route() is not exposed on RestApplication class, you can call app.restServer.route() instead (or add the missing RestApplication.prototype.route() shortcut).

bajtos · 2018-10-02T10:54:41Z

packages/openapi-v3/src/controller-spec.ts

+ * Validate and normalize the path to OpenAPI path template. No parameter
+ * modifier, custom pattern, or unnamed parameter is allowed.
+ *
+ * @param path Express style path (https://github.com/pillarjs/path-to-regexp)


I find this confusing - the method is used in a way that OpenAPI-styled path is passed in the input.

I think we should remove this method completely and modify resolveControllerSpec implementation to call validatePath instead. Thoughts?

On the second thought, I think we should call validatePath right at the time when a new route is created, regardless of whether the route is Controller-based or handler-based (see app.route API).

Could you please add some tests to verify that invalid paths are correctly rejected for all possible sources of path strings? I am thinking about the following sources: @get decorator on a controller method, app.route/restServer.route, paths provided in the OpenAPI spec document passed to app.api().

raymondfeng · 2018-10-03T15:11:00Z

I added optimization for those routes that don't have path variables. The RoutingTable by default uses RegExpRouter now.

b-admike · 2018-10-04T04:45:25Z

@raymondfeng I looked at your recent changes at a high level and they look great 👍. I'll look into them in detail tomorrow.

bajtos

I am not a fan of having multiple routers when it's not clear how should LB4 users decided which router to use. Personally, I prefer the framework to make to he hard decisions and let users to focus on delivering business value.

I am also concerned about adding ~1.2k lines of new code that we will have to maintain and fix bug inside.

Having said that, if others thinks this is a good idea, then I am not going to block progress.

At minimum, please add documentation explaining the users how to configure the app to use a different router, explain the trade-offs between our two provided routers so that users can better decide which one to use.

Nice to have: document how to implement a custom router. Most importantly, we need a clear documentation on the assumption made by RestServer about the behavior of a custom router. A test suite that people implementing a custom router can execute to validate their implementation would be ideal.

Extensibility comes with costs if we want to make it user friendly.

bajtos · 2018-10-04T07:21:07Z

packages/rest/src/rest.server.ts

-    this._httpHandler = new HttpHandler(this);
+    const router = this.getSync(RestBindings.ROUTER, {optional: true});
+    let routingTable: RoutingTable;
+    if (router) {


Can we move this if to RoutingTable constructor so that RestServer can be simplified as follows:

const routingTable = new RoutingTable(router);

bajtos · 2018-10-04T07:21:50Z

packages/rest/src/router/index.ts

+// License text available at https://opensource.org/licenses/MIT
+
+export {
+  RouteEntry,


Considering how many items we are exporting, I think it may be best to simply export everything.

export * from `./routing-table';

Thoughts?

bajtos · 2018-10-04T07:24:19Z

packages/rest/src/router/routing-table.ts

-  private readonly _routes: RouteEntry[] = [];
+  private readonly _router: RestRouter = new RegExpRouter();
+
+  constructor(router?: RestRouter) {


How about:

constructor(private readonly router: RestRouter = new RegExpRouter()) { // ... }

raymondfeng · 2018-10-04T17:09:10Z

@bajtos Addressed your coding comments and added docs. PTAL.

I would like to have it landed with our GA as the mapping of routes without variables are much faster now.

b-admike · 2018-10-05T05:00:01Z

docs/site/Routing-requests.md

+
+```ts
+import {RestBindings} from '@loopback/rest';
+app.bind(RestBindings.ROUTER).toClass(TrieRouter);


nitpick: Which file would this be in and where can users import TrieRouter from?

Good catch.

b-admike · 2018-10-05T05:02:58Z

packages/rest/src/rest.server.ts

+    /**
+     * Check if there is custom router in the context
+     */
+    const router = this.getSync(RestBindings.ROUTER, {optional: true});


Do we need a try/catch block for the getSync call here?

No, as it uses an optional flag.

b-admike · 2018-10-05T05:05:43Z

packages/rest/test/acceptance/routing/routing.acceptance.ts

+      const server = await app.getServer(RestServer);
+      const handler = await server.get(RestBindings.HANDLER);
+      // tslint:disable-next-line:no-any
+      expect((handler as any)._routes._router).to.be.instanceof(TrieRouter);


nitpick: why do we need to cast handler as any here? can we do const handler = await server.get<HttpHandler>(RestBindings.HANDLER); above?

It's a hacky way to test non-public properties.

@raymondfeng Please add a comment to the code to preserve this explanation for future readers.

bajtos

Reviewed the code in details, PTAL at my comments below.

Please update README in benchmark folder to show the new performance statistics. It would be great to have a similar README for your newly added benchmark, one that shows "baseline" data of the current implementation.

I am not very happy about having to review such a large change in the short time we have :(

One of my major concerns is that we don't have a Router test suite that we could run on both RegExpRouter and TrieRouter to verify that all routers are correctly handling all edge cases we have identified so far. Having two sets of tests (one for each router) is not manageable.

I would prefer very much to leave TrieRouter out of this pull request, probably with BaseRouter too. Adding an extension point (a custom router) can be easily done post-4.0 GA as it's a semver-minor change. Changing the API contract of an ill-designed extension point is much more difficult as it's a breaking change.

I am not convinced the current design of the extension point is good enough to serve us for the next 6+ months:

See my comments about BaseRouter design
There is no easy way how to test all aspects of a new Router to ensure it's fully compliant with our expectations
It's not clear to me if (and how) can users customize the router at Server vs. Application level.

Let's give us more time to find a good design for Router extensibility and implement a robust solution that's easy to use both for users and Router writers.

bajtos · 2018-10-05T08:01:49Z

packages/rest/package.json

@@ -44,7 +44,8 @@
    "path-to-regexp": "^2.2.0",
    "qs": "^6.5.2",
    "strong-error-handler": "^3.2.0",
-    "validator": "^10.4.0"
+    "validator": "^10.4.0",
+    "wayfarer": "^6.6.4"


AFAICT, wayfarer is not used by our code, can you remove this dependency please?

bajtos · 2018-10-05T08:02:47Z

packages/rest/src/rest.application.ts

+    path: string,
+    spec: OperationObject,
+    handler: Function,
+  ): Binding;


Please use this new API to simplify Routing doc page - see https://loopback.io/doc/en/lb4/Routes.html#using-partial-openapi-spec-fragments

Please use this new API to simplify Routing doc page - see https://loopback.io/doc/en/lb4/Routes.html#using-partial-openapi-spec-fragments

This hasn't been addressed AFAICT, please take a look ☝️

bajtos · 2018-10-05T08:06:51Z

packages/rest/src/rest.server.ts

+    const router = this.getSync(RestBindings.ROUTER, {optional: true});
+    const routingTable = new RoutingTable(router);
+
+    this._httpHandler = new HttpHandler(this, routingTable);


I am wondering if it's a good idea for HttpHandler to accept RoutingTable instance. Have you considered an alternative design where HttpHandler accepts a router instance only?

const router = this.getSync(RestBindings.ROUTER, {optional: true}); this._httpHandler = new HttpHandler(this, router);

IMO, the RoutingTable class is an implementation detail of HttpHandler that's not open for extensibility to consumers.

Please note that using the constructor the pass down a version of RestRouter is still internal implementation details, which we can change later. The public contract is to bind a custom RestRouter to the given key.

bajtos · 2018-10-05T08:08:05Z

packages/rest/src/router/openapi-path.ts

+ *
+ * @param path Path template with `{var}`
+ */
+export function toOpenApiPath(path: string = '/') {


I still don't understand the purpose of this toOpenApiPath function - what are the benefits of calling toOpenApiPath as compared to calling validatePath directly?

bajtos · 2018-10-05T08:08:41Z

packages/rest/src/router/openapi-path.ts

+ * Validate the path to be compatible with OpenAPI path template. No parameter
+ * modifier, custom pattern, or unnamed parameter is allowed.
+ */
+export function validatePath(path: string = '/') {


Perhaps validateApiPath would be a better name, to make it clear that we are validating REST API/URL paths and not filesystem paths?

bajtos · 2018-10-05T08:24:36Z

packages/rest/src/router/trie-router.ts

+    debug('Route matched: %j', found);
+
+    if (found) {
+      route = found.node.value!;


Can you fix the type definition of trie.match to communicate the fact that value is always set when a route was found?

For example:

export interface ResolvedNode<T> { node: Node<T> & {value: T}; // the value is always set // tslint:disable-next-line:no-any params?: {[name: string]: any}; }

bajtos · 2018-10-05T08:25:46Z

packages/rest/src/router/trie.ts

+    const keys = path.split('/').filter(Boolean);
+    const params = {};
+    const resolved = search(keys, 0, params, this.root);
+    if (resolved && resolved.node.value != null) return resolved;


if (resolved && this.isNodeWithValue(resolved.node)) return resolved;

bajtos · 2018-10-05T08:28:29Z

packages/rest/src/router/trie.ts

+      let i = 0;
+      for (const n of child.names) {
+        const val = match[++i];
+        resolved.params![n] = decodeURIComponent(val);


Can we avoid relying on ! parameter?

E.g.

// you should probably define a new type for param map (don't we already have one?) const params: {[name: string]: any} = {}; let i = 0; for (const n of child.names) { const val = match[++i]; params[n] = decodeURIComponent(val); } return {params, node: child};

Can we avoid relying on ! operator please?

PTAL ☝️

bajtos · 2018-10-05T08:32:43Z

packages/rest/test/unit/router/routing-table.regexp.unit.ts

+    class TestController {}
+
+    const table = givenRoutingTable();
+    table.registerController(spec, TestController);


Have you considered using route-base approach to avoid the complexity of dealing with controllers?

bajtos · 2018-10-05T08:33:08Z

packages/rest/test/unit/router/routing-table.regexp.unit.ts

+    expect(route).to.have.property('pathParams');
+    expect(route.describe()).to.equal('TestController.greet');
+  });
+


Please add tests to verify how templated paths like /hello/{name} are handled.

raymondfeng · 2018-10-05T18:10:17Z

@bajtos I cleaned up the code based on your feedback.

The final benchmark now shows trie based router performs much better than regexp. I must have messed up some of the testing before.

name                 duration    count    found   missed
TrieRouter          0,1453883       10        8        2
RegExpRouter        0,1220030       10        8        2

name                 duration    count    found   missed
TrieRouter          0,2109957       40       35        5
RegExpRouter        0,8762936       40       35        5

name                 duration    count    found   missed
TrieRouter          0,4895252      160      140       20
RegExpRouter       0,52156699      160      140       20

name                 duration    count    found   missed
TrieRouter         0,16065852      640      560       80
RegExpRouter      0,304921026      640      560       80

name                 duration    count    found   missed
TrieRouter         0,60165877     2560     2240      320
RegExpRouter      4,592089555     2560     2240      320

I now make TrieRouter the default and have test/benchmark coverage for both TrieRouter and RegExpRouter.

bajtos

The patch looks reasonable now. Not all of my comments have been addressed and I found few more places where to improve the implementation (see below), but I don't see any major problems now.

FWIW, LGTM.

bajtos · 2018-10-08T12:41:08Z

packages/rest/src/rest.application.ts

+    path: string,
+    spec: OperationObject,
+    handler: Function,
+  ): Binding;


Please use this new API to simplify Routing doc page - see https://loopback.io/doc/en/lb4/Routes.html#using-partial-openapi-spec-fragments

This hasn't been addressed AFAICT, please take a look ☝️

bajtos · 2018-10-08T12:43:02Z

packages/rest/src/router/regexp-router.ts

+ */
+interface RegExpRouteEntry extends RouteEntry {
+  regexp?: RegExp;
+  keys?: pathToRegExp.Key[];


How can we have RegExpRouteEntry instance with undefined regexp and keys? AFAICT, these two properties are always set by our code.

bajtos · 2018-10-08T12:45:05Z

packages/rest/src/router/regexp-router.ts

+    this.routes.push(entry);
+    const path = toExpressPath(route.path);
+    entry.keys = [];
+    entry.regexp = pathToRegExp(path, entry.keys, {strict: false, end: true});


If you marked keys and regexp as optiona to support the code in addRouteWithPathVars, then I think the following implementation may be better:

const path = toExpressPath(route.path); const keys = []; const regexp = pathToRegExp(path, keys, {strict: false, end: true}); const entry: RegExpRouteEntry = Object.assign(route, {keys, regexp});

bajtos · 2018-10-08T12:46:08Z

packages/rest/src/router/regexp-router.ts

+    pathMatch: RegExpExecArray,
+  ): PathParameterValues {
+    const pathParams: PathParameterValues = {};
+    for (const ix in route.keys!) {


route.keys is always set, we should not need to use ! here.

bajtos · 2018-10-08T12:48:53Z

packages/rest/src/router/regexp-router.ts

+        continue;
+      }
+
+      const match = r.regexp!.exec(request.path);


r.regexp is always set, we should not need to use ! here.

bajtos · 2018-10-08T12:48:59Z

packages/rest/src/router/regexp-router.ts

+    this._sort();
+    for (const r of this.routes) {
+      debug('trying endpoint %s', inspect(r, {depth: 5}));
+      if (r.verb !== request.method!.toLowerCase()) {


Are you sure we need ! after request.method? BaseRouter#getKeyForRequest does not use it and calls request.method.toLowerCase().

bajtos · 2018-10-08T12:51:42Z

packages/rest/src/router/trie.ts

+      let i = 0;
+      for (const n of child.names) {
+        const val = match[++i];
+        resolved.params![n] = decodeURIComponent(val);


Can we avoid relying on ! operator please?

PTAL ☝️

- validate and normalize paths for openapi - add benchmark for routing - make the router pluggable - simplify app routing to plain functions - optimize simple route mapping and use regexp as default - add `rest.router` binding to allow customization - add docs for routing requests

raymondfeng · 2018-10-08T16:23:41Z

@bajtos I addressed your additional comments.

raymondfeng requested review from bajtos and jannyHou as code owners September 26, 2018 21:12

raymondfeng force-pushed the validate-openapi-path branch from c70333d to e6d6765 Compare September 27, 2018 00:44

virkt25 reviewed Sep 27, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch 2 times, most recently from 0833be5 to b595511 Compare September 27, 2018 18:25

virkt25 approved these changes Sep 27, 2018

View reviewed changes

b-admike reviewed Sep 27, 2018

View reviewed changes

b-admike approved these changes Sep 27, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch from b595511 to 267017d Compare September 28, 2018 19:28

bajtos reviewed Oct 1, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch from 267017d to 8983d99 Compare October 1, 2018 16:48

raymondfeng changed the title ~~fix(openapi-v3): validate and normalize paths for openapi~~ feat(openapi-v3): Implement trie-based routing and tidy up path validation Oct 1, 2018

raymondfeng changed the title ~~feat(openapi-v3): Implement trie-based routing and tidy up path validation~~ feat(rest): Implement trie-based routing and tidy up path validation Oct 1, 2018

bajtos reviewed Oct 1, 2018

View reviewed changes

b-admike approved these changes Oct 1, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch 2 times, most recently from 9338db3 to 1181189 Compare October 1, 2018 18:11

raymondfeng force-pushed the validate-openapi-path branch from 1181189 to bc883f4 Compare October 1, 2018 18:53

bajtos reviewed Oct 2, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch 5 times, most recently from b35a395 to 056062a Compare October 3, 2018 04:57

raymondfeng force-pushed the validate-openapi-path branch 3 times, most recently from e2c61d9 to 285e128 Compare October 3, 2018 23:26

bajtos reviewed Oct 4, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch 2 times, most recently from fe823e7 to c40c548 Compare October 4, 2018 14:38

b-admike reviewed Oct 5, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch from 312cb32 to 13a5adb Compare October 5, 2018 05:30

bajtos suggested changes Oct 5, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch 2 times, most recently from 32c7e4b to e05b421 Compare October 5, 2018 18:06

raymondfeng force-pushed the validate-openapi-path branch 3 times, most recently from b3215d3 to 8eecce6 Compare October 8, 2018 03:50

bajtos approved these changes Oct 8, 2018

View reviewed changes

raymondfeng force-pushed the validate-openapi-path branch from 28d87fe to 30dcd0b Compare October 8, 2018 16:05

chore: rename unit test file

409685c

raymondfeng force-pushed the validate-openapi-path branch 2 times, most recently from 6d60f14 to 7b5430e Compare October 8, 2018 16:13

raymondfeng force-pushed the validate-openapi-path branch from 7b5430e to e628cd3 Compare October 8, 2018 16:17

raymondfeng merged commit a682ce2 into master Oct 8, 2018

raymondfeng deleted the validate-openapi-path branch October 8, 2018 17:55

raymondfeng mentioned this pull request Oct 24, 2018

How to use regular expressions in route path and how to make parameters optional #1866

Closed

bajtos mentioned this pull request Jul 9, 2019

Router: switch to Trie-based implementation #98

Closed

feat(rest): Implement trie-based routing and tidy up path validation #1765

feat(rest): Implement trie-based routing and tidy up path validation #1765

Conversation

raymondfeng commented Sep 26, 2018 • edited

Checklist

virkt25 left a 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

virkt25 left a comment

Choose a reason for hiding this comment

b-admike left a 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Oct 1, 2018

raymondfeng commented Oct 1, 2018

bajtos left a 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 Oct 1, 2018

b-admike left a comment

Choose a reason for hiding this comment

raymondfeng commented Oct 1, 2018

bajtos left a 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

raymondfeng commented Oct 3, 2018

b-admike commented Oct 4, 2018

bajtos left a 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

raymondfeng commented Oct 4, 2018

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

raymondfeng Oct 5, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a 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

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

raymondfeng commented Oct 5, 2018

bajtos left a 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

raymondfeng commented Sep 26, 2018 •

edited

raymondfeng Oct 5, 2018 •

edited