Skip to content

Commit 5d305a6

Browse files
committed
fix: sort programmatic routes by specificity to prevent shadowing
1 parent 86d6cde commit 5d305a6

2 files changed

Lines changed: 60 additions & 0 deletions

File tree

packages/fresh/src/router.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import { sortRoutePaths } from "./fs_routes.ts";
2+
13
export type Method =
24
| "HEAD"
35
| "GET"
@@ -89,6 +91,20 @@ export class UrlPatternRouter<T> implements Router<T> {
8991
};
9092
this.#dynamics.set(pathname, def);
9193
this.#dynamicArr.push(def);
94+
this.#dynamicArr.sort((a, b) => {
95+
const aPath = a.pattern.pathname;
96+
const bPath = b.pattern.pathname;
97+
// Convert URLPattern format (:param, :param*) to
98+
// filesystem route format ([param], [...param]) so that
99+
// sortRoutePaths can rank by specificity.
100+
const aFs = aPath
101+
.replace(/:(\w+)\*/g, "[...$1]")
102+
.replace(/:(\w+)/g, "[$1]");
103+
const bFs = bPath
104+
.replace(/:(\w+)\*/g, "[...$1]")
105+
.replace(/:(\w+)/g, "[$1]");
106+
return sortRoutePaths(aFs, bFs);
107+
});
92108
}
93109

94110
byMethod = def.byMethod;

packages/fresh/src/router_test.ts

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,3 +319,47 @@ Deno.test("UrlPatternRouter - non-standard method on dynamic route", () => {
319319
pattern: "/books/:id",
320320
});
321321
});
322+
323+
Deno.test("UrlPatternRouter - specific route matches before catch-all regardless of registration order", () => {
324+
const router = new UrlPatternRouter<() => string>();
325+
const catchAll = () => "catch-all";
326+
const specific = () => "specific";
327+
328+
// Register catch-all first, then specific route
329+
router.add("GET", "/blog/:rest*", catchAll);
330+
router.add("GET", "/blog/:id", specific);
331+
332+
// Specific route should match /blog/123
333+
const res = router.match("GET", new URL("/blog/123", "http://localhost"));
334+
expect(res.item).toBe(specific);
335+
expect(res.params).toEqual({ id: "123" });
336+
337+
// Catch-all should still match paths the specific route doesn't
338+
const res2 = router.match(
339+
"GET",
340+
new URL("/blog/a/b/c", "http://localhost"),
341+
);
342+
expect(res2.item).toBe(catchAll);
343+
expect(res2.params).toEqual({ rest: "a/b/c" });
344+
});
345+
346+
Deno.test("UrlPatternRouter - multiple dynamic routes sorted by specificity", () => {
347+
const router = new UrlPatternRouter<() => string>();
348+
const catchAll = () => "catch-all";
349+
const byId = () => "by-id";
350+
const byName = () => "by-name";
351+
352+
// Register catch-all first, then specific routes
353+
router.add("GET", "/api/:rest*", catchAll);
354+
router.add("GET", "/api/:name", byName);
355+
router.add("GET", "/api/:id", byId);
356+
357+
// Registration order was: catchAll, byName, byId
358+
// After sorting, more specific routes should match first.
359+
// /api/:id and /api/:name are equally specific (both dynamic),
360+
// so the sort order between them is stable/deterministic.
361+
const res = router.match("GET", new URL("/api/hello", "http://localhost"));
362+
// Should match one of the specific routes, not the catch-all
363+
expect(res.item).not.toBe(catchAll);
364+
expect(res.methodMatch).toBe(true);
365+
});

0 commit comments

Comments
 (0)