Skip to content

Commit d313e63

Browse files
2chanhaeng2chanhaeng
committed
Apply review feedback from PR #758
Address review comments from automated reviewers: - Harden isPath against malformed templates so it returns false instead of throwing, restoring its JSDoc contract. - Relax the Path type from `{/${string}}/${string}` to `{/${string}}${string}` so isPath is consistent with the router's handling of bare path-expansion templates like `{/var}`. - Run path validation before mutating the router in setActorDispatcher, so a failed registration does not leave a stale `actor` route that confuses subsequent attempts. - Reuse validateSingleIdentifierVariablePath across inbox, outbox, following, followers, liked, featured, featuredTags, and inbox listener registrations instead of the loose Router.variables check, so explode (`*`) and prefix (`:N`) modifiers cannot smuggle through `{identifier}` paths at runtime. - Mark @fedify/uri-template as side-effect free for tree-shaking. - Add a regression test for the unnamed consumeUnnamed minLength pruning bug. The matcher fix is intentionally left for a separate commit so the test stays red until then. #758 Assisted-by: Claude Code:claude-opus-4-7[1m]
1 parent 955ec94 commit d313e63

6 files changed

Lines changed: 73 additions & 86 deletions

File tree

‎packages/fedify/src/federation/builder.ts‎

Lines changed: 37 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -256,16 +256,11 @@ export class FederationBuilderImpl<TContextData>
256256
if (this.router.has("actor")) {
257257
throw new RouterError("Actor dispatcher already set.");
258258
}
259-
const variables = Router.variables(path as Path);
259+
validateSingleIdentifierVariablePath(
260+
path,
261+
"Path for actor dispatcher must have one variable: {identifier}",
262+
);
260263
this.router.add(path as Path, "actor");
261-
if (
262-
variables.size !== 1 ||
263-
!variables.has("identifier")
264-
) {
265-
throw new RouterError(
266-
"Path for actor dispatcher must have one variable: {identifier}",
267-
);
268-
}
269264
const callbacks: ActorCallbacks<TContextData> = {
270265
dispatcher: async (context, identifier) => {
271266
const actor = await this._getTracer().startActiveSpan(
@@ -719,15 +714,10 @@ export class FederationBuilderImpl<TContextData>
719714
);
720715
}
721716
} else {
722-
const variables = Router.variables(path as Path);
723-
if (
724-
variables.size !== 1 ||
725-
!variables.has("identifier")
726-
) {
727-
throw new RouterError(
728-
"Path for inbox dispatcher must have one variable: {identifier}",
729-
);
730-
}
717+
validateSingleIdentifierVariablePath(
718+
path,
719+
"Path for inbox dispatcher must have one variable: {identifier}",
720+
);
731721
this.router.add(path as Path, "inbox");
732722
this.inboxPath = path;
733723
}
@@ -913,16 +903,11 @@ export class FederationBuilderImpl<TContextData>
913903
if (this.router.has("following")) {
914904
throw new RouterError("Following collection dispatcher already set.");
915905
}
916-
const variables = Router.variables(path as Path);
917-
if (
918-
variables.size !== 1 ||
919-
!variables.has("identifier")
920-
) {
921-
throw new RouterError(
922-
"Path for following collection dispatcher must have one variable: " +
923-
"{identifier}",
924-
);
925-
}
906+
validateSingleIdentifierVariablePath(
907+
path,
908+
"Path for following collection dispatcher must have one variable: " +
909+
"{identifier}",
910+
);
926911
this.router.add(path as Path, "following");
927912
const callbacks: CollectionCallbacks<
928913
Actor | URL,
@@ -980,16 +965,11 @@ export class FederationBuilderImpl<TContextData>
980965
if (this.router.has("followers")) {
981966
throw new RouterError("Followers collection dispatcher already set.");
982967
}
983-
const variables = Router.variables(path as Path);
984-
if (
985-
variables.size !== 1 ||
986-
!variables.has("identifier")
987-
) {
988-
throw new RouterError(
989-
"Path for followers collection dispatcher must have one variable: " +
990-
"{identifier}",
991-
);
992-
}
968+
validateSingleIdentifierVariablePath(
969+
path,
970+
"Path for followers collection dispatcher must have one variable: " +
971+
"{identifier}",
972+
);
993973
this.router.add(path as Path, "followers");
994974
const callbacks: CollectionCallbacks<
995975
Recipient,
@@ -1043,16 +1023,11 @@ export class FederationBuilderImpl<TContextData>
10431023
if (this.router.has("liked")) {
10441024
throw new RouterError("Liked collection dispatcher already set.");
10451025
}
1046-
const variables = Router.variables(path as Path);
1047-
if (
1048-
variables.size !== 1 ||
1049-
!variables.has("identifier")
1050-
) {
1051-
throw new RouterError(
1052-
"Path for liked collection dispatcher must have one variable: " +
1053-
"{identifier}",
1054-
);
1055-
}
1026+
validateSingleIdentifierVariablePath(
1027+
path,
1028+
"Path for liked collection dispatcher must have one variable: " +
1029+
"{identifier}",
1030+
);
10561031
this.router.add(path as Path, "liked");
10571032
const callbacks: CollectionCallbacks<
10581033
Like,
@@ -1114,16 +1089,11 @@ export class FederationBuilderImpl<TContextData>
11141089
if (this.router.has("featured")) {
11151090
throw new RouterError("Featured collection dispatcher already set.");
11161091
}
1117-
const variables = Router.variables(path as Path);
1118-
if (
1119-
variables.size !== 1 ||
1120-
!variables.has("identifier")
1121-
) {
1122-
throw new RouterError(
1123-
"Path for featured collection dispatcher must have one variable: " +
1124-
"{identifier}",
1125-
);
1126-
}
1092+
validateSingleIdentifierVariablePath(
1093+
path,
1094+
"Path for featured collection dispatcher must have one variable: " +
1095+
"{identifier}",
1096+
);
11271097
this.router.add(path as Path, "featured");
11281098
const callbacks: CollectionCallbacks<
11291099
Object,
@@ -1185,16 +1155,11 @@ export class FederationBuilderImpl<TContextData>
11851155
if (this.router.has("featuredTags")) {
11861156
throw new RouterError("Featured tags collection dispatcher already set.");
11871157
}
1188-
const variables = Router.variables(path as Path);
1189-
if (
1190-
variables.size !== 1 ||
1191-
!variables.has("identifier")
1192-
) {
1193-
throw new RouterError(
1194-
"Path for featured tags collection dispatcher must have one " +
1195-
"variable: {identifier}",
1196-
);
1197-
}
1158+
validateSingleIdentifierVariablePath(
1159+
path,
1160+
"Path for featured tags collection dispatcher must have one " +
1161+
"variable: {identifier}",
1162+
);
11981163
this.router.add(path as Path, "featuredTags");
11991164
const callbacks: CollectionCallbacks<
12001165
Hashtag,
@@ -1254,15 +1219,10 @@ export class FederationBuilderImpl<TContextData>
12541219
);
12551220
}
12561221
} else {
1257-
const variables = Router.variables(inboxPath as Path);
1258-
if (
1259-
variables.size !== 1 ||
1260-
!variables.has("identifier")
1261-
) {
1262-
throw new RouterError(
1263-
"Path for inbox must have one variable: {identifier}",
1264-
);
1265-
}
1222+
validateSingleIdentifierVariablePath(
1223+
inboxPath,
1224+
"Path for inbox must have one variable: {identifier}",
1225+
);
12661226
this.router.add(inboxPath as Path, "inbox");
12671227
this.inboxPath = inboxPath;
12681228
}

‎packages/uri-template/package.json‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,5 +64,6 @@
6464
"tsdown": "catalog:",
6565
"typescript": "catalog:"
6666
},
67-
"dependencies": {}
67+
"dependencies": {},
68+
"sideEffects": false
6869
}

‎packages/uri-template/src/template/template.test.ts‎

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { test } from "@fedify/fixture";
22
import { deepEqual, equal } from "node:assert";
3-
import { throws } from "node:assert/strict";
3+
import { ok, throws } from "node:assert/strict";
44
import {
55
createFixedTemplateMatchTest,
66
createFixedTemplateTest,
@@ -112,3 +112,24 @@ test("parses reusable template instances", () => {
112112
},
113113
]);
114114
});
115+
116+
// Regression for the `consumeUnnamed` minLength bug: when an unnamed expression
117+
// has more separated parts than variables, the matcher must let the *current*
118+
// variable absorb fewer parts than the naive `parts - remainingVars` formula
119+
// allows. With `{x:5,y}` against `abc,def,ghi` the only round-trippable
120+
// binding has x consume one part (so prefix:5 truncation does not corrupt the
121+
// joined string); under the buggy minLength formula the matcher only reaches
122+
// the fallback `x undefined, y absorbs everything` decomposition, leaving
123+
// `m.x` undefined.
124+
test("Template#match — unnamed minLength must allow current var to consume one part", () => {
125+
const template = new Template("{x:5,y}");
126+
const m = template.match("abc,def,ghi");
127+
128+
ok(m != null, "matcher returned null for a round-trippable URI");
129+
equal(template.expand(m), "abc,def,ghi");
130+
equal(
131+
m.x,
132+
"abc",
133+
"matcher should reach the binding with x consuming one part",
134+
);
135+
});

‎packages/uri-template/src/tests/template.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,7 @@ export function createMatchBench(
347347
};
348348
};
349349
}
350+
350351
const mod = (i: number, j: number) => Math.floor(i / j);
351352

352353
export function createMatchBenchTestCases(): readonly string[] {

‎packages/uri-template/src/types.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import type { Operator } from "./const.ts";
44
/**
55
* Path-shaped URI Template accepted by the router.
66
*/
7-
export type Path = `/${string}` | `{/${string}}/${string}`;
7+
export type Path = `/${string}` | `{/${string}}${string}`;
88

99
/**
1010
* Primitive value accepted by {@link Template.expand}.

‎packages/uri-template/src/utils.ts‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -20,13 +20,17 @@ export const isLiteral = <T extends { kind: string }>(
2020
* return `false`.
2121
*/
2222
export function isPath(path: string): path is Path {
23-
const template = new Template(path);
23+
try {
24+
const template = new Template(path);
2425

25-
const [first] = template.tokens;
26-
if (first == null) return false;
27-
if (isLiteral(first)) return first.text.startsWith("/");
28-
if (first.operator === "/") return true;
29-
return false;
26+
const [first] = template.tokens;
27+
if (first == null) return false;
28+
if (isLiteral(first)) return first.text.startsWith("/");
29+
if (first.operator === "/") return true;
30+
return false;
31+
} catch {
32+
return false;
33+
}
3034
}
3135

3236
export function assertPath(path: string): asserts path is Path {

0 commit comments

Comments
 (0)