route to the most specific http endpoint (#27567)

the httpRouter currently has somewhat odd behavior, because we allow paths to overlap but only if you define them in a certain order. new behavior: http route paths can always overlap, unless they are exactly the same. when picking one to execute, we choose an exact path if one matches, and otherwise we pick the longest prefix path that matches. added test cases, and also ran similar tests manually GitOrigin-RevId: 372c4bdaf384e91dfa8d4c6cf77e6d75ec1708e2
get-convex · Jul 3, 2024 · 9226c22 · 9226c22
1 parent a92fc71
commit 9226c22
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 37 deletions.
diff --git a/npm-packages/convex/src/server/router.test.ts b/npm-packages/convex/src/server/router.test.ts
@@ -93,29 +93,45 @@ test("HttpRouter pathPrefix", () => {
     http.route({ pathPrefix: "/path1/", method: "GET", handler: action1 });
   }).toThrow();
 
-  // prefix is shadowed by prefix
-  expect(() => {
-    http.route({
-      pathPrefix: "/path1/foo/",
-      method: "GET",
-      handler: action1,
-    });
-  }).toThrow();
+  // more specific pathPrefix
+  http.route({
+    pathPrefix: "/path1/foo/",
+    method: "GET",
+    handler: action1,
+  });
 
-  // path is shadowed by prefix
-  expect(() => {
-    http.route({ path: "/path1/foo", method: "GET", handler: action1 });
-  }).toThrow();
+  // less specific pathPrefix
+  http.route({
+    pathPrefix: "/",
+    method: "GET",
+    handler: action1,
+  });
+
+  // Longest matching prefix is used.
+  expect(http.lookup("/path1/foo/bar", "GET")).toEqual([
+    action1,
+    "GET",
+    "/path1/foo/*",
+  ]);
+  expect(http.lookup("/path1/foo", "GET")).toEqual([
+    action1,
+    "GET",
+    "/path1/*",
+  ]);
+  expect(http.lookup("/path1", "GET")).toEqual([action1, "GET", "/*"]);
+
+  // Exact path is more specific than prefix
+  http.route({ path: "/path1/foo", method: "GET", handler: action1 });
+  expect(http.lookup("/path1/foo", "GET")).toEqual([
+    action1,
+    "GET",
+    "/path1/foo",
+  ]);
+  // Duplicate exact match
+  expect(() =>
+    http.route({ path: "/path1/foo", method: "GET", handler: action1 }),
+  ).toThrow();
 
   // Not shadowed: last path segment is different
   http.route({ pathPrefix: "/path11/", method: "GET", handler: action1 });
-
-  // path is shadowed by prefix
-  expect(() => {
-    http.route({
-      pathPrefix: "/path1/foo/",
-      method: "GET",
-      handler: action1,
-    });
-  }).toThrow();
 });
diff --git a/npm-packages/convex/src/server/router.ts b/npm-packages/convex/src/server/router.ts
@@ -169,18 +169,14 @@ export class HttpRouter {
     }
 
     if ("path" in spec) {
+      if ("pathPrefix" in spec) {
+        throw new Error(
+          `Invalid httpRouter route: cannot contain both 'path' and 'pathPrefix'`,
+        );
+      }
       if (!spec.path.startsWith("/")) {
         throw new Error(`path '${spec.path}' does not start with a /`);
       }
-      const prefixes =
-        this.prefixRoutes.get(method) || new Map<string, PublicHttpAction>();
-      for (const [prefix, _] of prefixes.entries()) {
-        if (spec.path.startsWith(prefix)) {
-          throw new Error(
-            `${spec.method} path ${spec.path} is shadowed by pathPrefix ${prefix}`,
-          );
-        }
-      }
       const methods: Map<RoutableMethod, PublicHttpAction> =
         this.exactRoutes.has(spec.path)
           ? this.exactRoutes.get(spec.path)!
@@ -203,12 +199,10 @@ export class HttpRouter {
       }
       const prefixes =
         this.prefixRoutes.get(method) || new Map<string, PublicHttpAction>();
-      for (const [prefix, _] of prefixes.entries()) {
-        if (spec.pathPrefix.startsWith(prefix)) {
-          throw new Error(
-            `${spec.method} pathPrefix ${spec.pathPrefix} is shadowed by pathPrefix ${prefix}`,
-          );
-        }
+      if (prefixes.has(spec.pathPrefix)) {
+        throw new Error(
+          `${spec.method} pathPrefix ${spec.pathPrefix} is already defined`,
+        );
       }
       prefixes.set(spec.pathPrefix, handler);
       this.prefixRoutes.set(method, prefixes);
@@ -281,7 +275,10 @@ export class HttpRouter {
     if (exactMatch) return [exactMatch, method, path];
 
     const prefixes = this.prefixRoutes.get(method) || new Map();
-    for (const [pathPrefix, endpoint] of prefixes.entries()) {
+    const prefixesSorted = [...prefixes.entries()].sort(
+      ([prefixA, _a], [prefixB, _b]) => prefixB.length - prefixA.length,
+    );
+    for (const [pathPrefix, endpoint] of prefixesSorted) {
       if (path.startsWith(pathPrefix)) {
         return [endpoint, method, `${pathPrefix}*`];
       }