From 25c3ad272e97960336dec630bcbaca6c1767a260 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sun, 16 Aug 2026 19:17:05 -0400 Subject: [PATCH] fix(core): support ; and :length, and scope requiredness to the expression Addresses Copilot's round-2 review on #2035. It reported "no new comments" but carried three *suppressed* ones; all three reproduced against the pinned SDK. 1. The `;` path-parameter operator is absent from the SDK's operator list, so `{;id}` parsed as a variable literally named ";id" and expanded to nothing. Added the operator and its named expansion (`;a=1;b=2`). 2. An RFC 6570 prefix modifier was folded into the variable name: `{id:3}` yielded a variable called "id:3" and expanded to nothing. Varspecs are now parsed properly and the value truncated before encoding. Truncation is by code point, since `String.prototype.slice` counts UTF-16 units and would split an astral character into a lone surrogate. For both of these the wrong URI is the lesser problem: a form has to *name* the variables it asks the user to fill, so the panel was rendering fields labelled `;id` and `id:3` that nobody could use. 3. Requiredness was applied per variable, but it is a property of the *expression*: RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` with only `a` filled expands to `a`'s value -- the SDK does this too. The panel was refusing input the expander would have accepted. `hasRequiredValues` now encodes "any one name in a required group suffices", and such a field reads "Any one of: a, b" rather than falsely claiming each is mandatory. Two structural consequences, each pinned by a test: - Takeover is now per TEMPLATE rather than per expression. Splicing corrected fragments into a template the SDK re-expands would leave its cross-expression `?`-to-`&` rewrite blind to the fragments already resolved. - Expansion is split into a strict variant that throws and a lenient one that returns the raw template. `readResourceFromTemplate` wraps the thrown error with the template name -- three pre-existing integration tests assert that -- while the form must not throw on a server-supplied template, since that would take out the panel on render. The strict variant constructs the SDK template unconditionally, because that construction is what validates syntax: otherwise `x://{;a}{b,c` would take the own-expansion path and its unclosed tail would pass as literal text with nothing objecting. Signed-off-by: cliffhall --- AGENTS.md | 13 +- README.md | 12 +- .../ResourceTemplatePanel.tsx | 24 +- .../web/src/test/core/mcp/uriTemplate.test.ts | 168 ++++++++- clients/web/src/utils/uriTemplate.test.ts | 10 +- clients/web/src/utils/uriTemplate.ts | 25 +- core/mcp/inspectorClient.ts | 26 +- core/mcp/uriTemplate.ts | 330 +++++++++++++----- 8 files changed, 468 insertions(+), 140 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 5005d6d8..527eccbc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -77,9 +77,16 @@ v2/main/ │ │ # shared by the web Resources form and │ │ # readResourceFromTemplate (TUI + CLI), so the │ │ # clients cannot drift on what a template means; -│ │ # delegates to the SDK's UriTemplate but corrects -│ │ # its multi-name `{a,b}` branch, which skips both -│ │ # encoding and the operator prefix — #1919; +│ │ # delegates to the SDK's UriTemplate for what it +│ │ # gets right, and takes over a whole template +│ │ # containing any of the three shapes it does not: +│ │ # `{a,b}` (raw-joined, unencoded, prefix dropped), +│ │ # `{;id}` (operator absent from its list), and +│ │ # `{id:3}` (prefix modifier folded into the name) — +│ │ # the last two would render form fields literally +│ │ # labelled `;id` / `id:3`. Requiredness is per +│ │ # EXPRESSION, not per variable (hasRequiredValues): +│ │ # `{a,b}` with only `a` filled is expandable — #1919; │ │ # modernTaskSchemas.ts: SEP-2663 modern Tasks │ │ # extension wire schemas + normalize/handle helpers, │ │ # used by the raw-wire tasks/* channel — #1631; diff --git a/README.md b/README.md index 0bc30868..f274f411 100644 --- a/README.md +++ b/README.md @@ -249,7 +249,17 @@ Open the Resources tab and pick **events_by_topic**, then enter `foo/bar`. The r > The plain `foobar://events` resource is registered deliberately, not as filler. The SDK's `UriTemplate.match()` compiles `{?topic}` to a **required** `\?topic=([^&]+)`, so a template alone cannot serve the blank read — `match("foobar://events")` returns `null`. A real server exposes the unfiltered collection as its own resource; the showcase does the same so that step actually resolves. -All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` with one correction applied first: the SDK's `expandPart` takes an early `names.length > 1` branch that raw-joins the values, skipping both percent-encoding **and** the operator prefix, so `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` and `x://a{/p,q}` to `x://ax y,z`. Those expressions are expanded by the helper and spliced in as literal text before the SDK sees them; every single-name and query expression still goes through the SDK untouched. +All three clients expand through one shared helper, [`core/mcp/uriTemplate.ts`](./core/mcp/uriTemplate.ts) — the web Resources form directly, the TUI and CLI via `InspectorClient.readResourceFromTemplate` — so they cannot disagree about what a template means. It delegates to the SDK's `UriTemplate` for every expression the SDK handles correctly, and takes over any template containing one of the three shapes it does not (each measured against the pinned SDK, not inferred): + +| Shape | SDK `variableNames` | SDK expansion | Correct | +| --- | --- | --- | --- | +| `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, operator prefix dropped | `foo%2Fbar,q` | +| `{;id}` | `[";id"]` | `""` — the `;` operator is not in its list | `;id=7` | +| `{id:3}` | `["id:3"]` | `""` — the prefix modifier is folded into the name | `abc` | + +The last two matter beyond the URI: a form has to *name* the variables it asks the user to fill, so on the SDK's parse it would render fields literally labelled `;id` and `id:3`. Takeover is per **template**, not per expression, so the cross-expression `?`-to-`&` rewrite always sees every expression that actually emitted. + +One consequence worth knowing when writing a test server: the SDK's **matcher** has the mirrored gaps (`partToRegExp` emits a single capture for `{a,b}` and knows no `;`), so an SDK-backed server cannot round-trip those templates whatever the client sends. Emitting a spec-correct URI is the half the client controls; the unit tests cover those shapes directly rather than through a showcase server. #### Advertised extensions diff --git a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx index eafd16f4..f2ac2b8e 100644 --- a/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx +++ b/clients/web/src/components/groups/ResourceTemplatePanel/ResourceTemplatePanel.tsx @@ -16,6 +16,7 @@ import { AnnotationBadge } from "../../elements/AnnotationBadge/AnnotationBadge" import { CopyButton } from "../../elements/CopyButton/CopyButton"; import { expandUriTemplate, + hasRequiredValues, previewUriTemplate, templateVariables, } from "../../../utils/uriTemplate"; @@ -215,12 +216,12 @@ export function ResourceTemplatePanel({ void runCompletion(varName, value, buildContext(varName)); } - // Only the variables whose absence would change the URI's shape gate the + // Only the expressions whose absence would change the URI's shape gate the // read; an unfilled `{?topic}` is a legitimate request for the unfiltered - // resource, and RFC 6570 drops the whole expression for it. - const canSubmit = declaredVariables.every( - (v) => !v.required || variables[v.name]?.length > 0, - ); + // resource, and RFC 6570 drops the whole expression for it. The rule is + // per-expression rather than per-variable -- `{a,b}` with only `a` filled + // expands to `a`'s value -- so it lives in core beside the expander. + const canSubmit = hasRequiredValues(declaredVariables, variables); function handleSubmit() { onReadResource(expandUriTemplate(uriTemplate, variables)); @@ -239,12 +240,19 @@ export function ResourceTemplatePanel({ {description && {description}} - {declaredVariables.map(({ name: varName, required }) => { + {declaredVariables.map(({ name: varName, required, groupNames }) => { /* v8 ignore next -- `?? ""` fallback unreachable: `variables` is seeded with every declared variable, so the key is always present. */ const fieldValue = variables[varName] ?? ""; // RFC 6570 omits an undefined variable under a query/path-segment - // operator entirely, so those fields are genuinely optional. - const description = required ? undefined : "Optional"; + // operator entirely, so those fields are genuinely optional. In a + // required multi-name expression no single field is mandatory either + // -- any one of them satisfies it -- so say which, rather than + // marking each one required and blocking valid input. + const description = !required + ? "Optional" + : groupNames.length > 1 + ? `Any one of: ${groupNames.join(", ")}` + : undefined; return useAutocomplete ? ( { kind: "expression", source: "{topic}", operator: "", + varspecs: [{ name: "topic" }], names: ["topic"], }, ]); @@ -25,6 +28,7 @@ describe("parseUriTemplate", () => { kind: "expression", source: "{?a,b*}", operator: "?", + varspecs: [{ name: "a" }, { name: "b" }], names: ["a", "b"], }, ]); @@ -44,13 +48,13 @@ describe("parseUriTemplate", () => { describe("templateVariables", () => { it("finds a simple variable and marks it required", () => { expect(templateVariables("foobar://events/{topic}")).toEqual([ - { name: "topic", operator: "", required: true }, + { name: "topic", operator: "", required: true, groupNames: ["topic"] }, ]); }); it("finds a query variable the old `\\{(\\w+)\\}` regex could not see", () => { expect(templateVariables("foobar://events{?topic}")).toEqual([ - { name: "topic", operator: "?", required: false }, + { name: "topic", operator: "?", required: false, groupNames: ["topic"] }, ]); }); @@ -72,7 +76,7 @@ describe("templateVariables", () => { it("deduplicates a repeated name and keeps it required if any use is", () => { expect(templateVariables("x://{?id}/{id}")).toEqual([ - { name: "id", operator: "?", required: true }, + { name: "id", operator: "?", required: true, groupNames: ["id"] }, ]); }); @@ -192,3 +196,161 @@ describe("expandUriTemplate - multi-name expressions (SDK correction)", () => { ); }); }); + +describe("varspec modifiers", () => { + // The pinned SDK folds a `:length` modifier into the variable name -- + // `new UriTemplate("x://a/{id:3}").variableNames` is `["id:3"]`, and it + // expands to "x://a/" -- so a form built on it would render a field the + // user cannot usefully fill. + it("parses a prefix modifier off the variable name", () => { + expect(templateVariables("x://a/{id:3}")).toEqual([ + { name: "id", operator: "", required: true, groupNames: ["id"] }, + ]); + }); + + it("truncates the value to the prefix length before encoding", () => { + expect(expandUriTemplate("x://a/{id:3}", { id: "abcdef" })).toBe( + "x://a/abc", + ); + }); + + it("encodes what survives truncation", () => { + expect(expandUriTemplate("x://a/{id:3}", { id: "a/bcdef" })).toBe( + "x://a/a%2Fb", + ); + }); + + it("truncates by code point, never splitting an astral character", () => { + // "\u{1F600}" is one code point but two UTF-16 units, so a naive + // `slice(0, 1)` would emit a lone surrogate. + expect(expandUriTemplate("x://{v:1}", { v: "\u{1F600}x" })).toBe( + `x://${encodeURIComponent("\u{1F600}")}`, + ); + }); + + it("ignores a malformed modifier rather than inventing a truncation", () => { + expect(templateVariables("x://{id:}")[0].name).toBe("id"); + expect(expandUriTemplate("x://{id:}", { id: "abcdef" })).toBe("x://abcdef"); + }); + + it("strips the explode modifier from the name", () => { + expect(templateVariables("x://{id*}")[0].name).toBe("id"); + }); +}); + +describe("the ; (path-parameter) operator", () => { + // Absent from the SDK's operator list entirely: it parses `{;id}` as a + // variable named ";id" and expands to "". + it("is recognised as an operator, not part of the name", () => { + expect(templateVariables("x://a{;id}")).toEqual([ + { name: "id", operator: ";", required: false, groupNames: ["id"] }, + ]); + }); + + it("expands to a named path parameter", () => { + expect(expandUriTemplate("x://a{;id}", { id: "7" })).toBe("x://a;id=7"); + }); + + it("repeats its separator per pair", () => { + expect(expandUriTemplate("x://a{;a,b}", { a: "1", b: "2" })).toBe( + "x://a;a=1;b=2", + ); + }); + + it("encodes the value", () => { + expect(expandUriTemplate("x://a{;p}", { p: "x/y" })).toBe("x://a;p=x%2Fy"); + }); + + it("omits cleanly when undefined", () => { + expect(expandUriTemplate("x://a{;id}", {})).toBe("x://a"); + }); +}); + +describe("hasRequiredValues", () => { + // A required *expression* is satisfied by any one of its names, because + // RFC 6570 drops the undefined ones -- verified against the SDK: + // `x://{a,b}` with only `a` expands to "x://only-a". + it("accepts a multi-name expression with only one name filled", () => { + const vars = templateVariables("x://{a,b}"); + expect(hasRequiredValues(vars, { a: "only-a", b: "" })).toBe(true); + expect(expandUriTemplate("x://{a,b}", { a: "only-a", b: "" })).toBe( + "x://only-a", + ); + }); + + it("rejects a multi-name expression with nothing filled", () => { + const vars = templateVariables("x://{a,b}"); + expect(hasRequiredValues(vars, { a: "", b: "" })).toBe(false); + }); + + it("still requires a lone required variable", () => { + const vars = templateVariables("file:///users/{userId}/profile"); + expect(hasRequiredValues(vars, { userId: "" })).toBe(false); + expect(hasRequiredValues(vars, { userId: "alice" })).toBe(true); + }); + + it("never blocks on an omittable expression", () => { + const vars = templateVariables("foobar://events{?topic}"); + expect(hasRequiredValues(vars, { topic: "" })).toBe(true); + }); + + it("is satisfied by a template with no variables at all", () => { + expect(hasRequiredValues(templateVariables("file:///static.txt"), {})).toBe( + true, + ); + }); +}); + +describe("cross-expression query joining", () => { + it("rewrites a second ? to & on the own-expansion path too", () => { + // Forced onto the own-expansion path by the `;` expression; the `?`-to-`&` + // rewrite must still apply, exactly as the SDK does it. + expect( + expandUriTemplate("x://a{;k}{?one}{?two}", { + k: "v", + one: "1", + two: "2", + }), + ).toBe("x://a;k=v?one=1&two=2"); + }); + + it("uses ? for the first query expression that actually emits", () => { + expect( + expandUriTemplate("x://a{;k}{?one}{?two}", { k: "v", two: "2" }), + ).toBe("x://a;k=v?two=2"); + }); +}); + +describe("strict vs lenient expansion", () => { + // `readResourceFromTemplate` wraps the thrown error with the template name; + // the web panel instead needs the raw template back, because an invalid + // template comes from the server and throwing would take out the panel. + it.each(["file:///{unclosed", "{a,b,c"])( + "strict throws on the invalid template %s", + (template) => { + expect(() => expandUriTemplateStrict(template, { x: "1" })).toThrow(); + }, + ); + + it.each(["file:///{unclosed", "{a,b,c"])( + "lenient returns %s unchanged", + (template) => { + expect(expandUriTemplate(template, { x: "1" })).toBe(template); + }, + ); + + it("validates syntax even when taking the own-expansion path", () => { + // `{;a}` forces own-expansion, and this module's parser treats the + // unclosed tail as literal text -- so without the unconditional SDK + // construction nothing would reject this. + expect(() => expandUriTemplateStrict("x://{;a}{b,c", { a: "1" })).toThrow(); + }); + + it("agrees with the lenient variant on a valid template", () => { + const template = "foobar://events{?topic}"; + const values = { topic: "foo/bar" }; + expect(expandUriTemplateStrict(template, values)).toBe( + expandUriTemplate(template, values), + ); + }); +}); diff --git a/clients/web/src/utils/uriTemplate.test.ts b/clients/web/src/utils/uriTemplate.test.ts index 7a1ead92..8a50ff7e 100644 --- a/clients/web/src/utils/uriTemplate.test.ts +++ b/clients/web/src/utils/uriTemplate.test.ts @@ -23,12 +23,18 @@ describe("previewUriTemplate", () => { ).toBe("foobar://events/foo%2Fbar"); }); - it("keeps a multi-name expression whole until every name is filled", () => { + it("expands a partially-filled multi-name expression, as submitting would", () => { + // RFC 6570 drops the undefined names rather than the whole expression, so + // showing `{?one,two}` here would promise a URI the submit does not send. expect(previewUriTemplate("x://a{?one,two}", { one: "1" })).toBe( - "x://a{?one,two}", + "x://a?one=1", ); }); + it("keeps a multi-name expression whole while none of its names is filled", () => { + expect(previewUriTemplate("x://a{?one,two}", {})).toBe("x://a{?one,two}"); + }); + it("still rewrites the second ? to & when both query expressions resolve", () => { expect( previewUriTemplate("x://a{?one}{?two}", { one: "1", two: "2" }), diff --git a/clients/web/src/utils/uriTemplate.ts b/clients/web/src/utils/uriTemplate.ts index 4c3f8ea2..0a14a33d 100644 --- a/clients/web/src/utils/uriTemplate.ts +++ b/clients/web/src/utils/uriTemplate.ts @@ -9,20 +9,21 @@ */ import { - applyMultiNameCorrection, definedValues, + expandUriTemplate, parseUriTemplate, } from "@inspector/core/mcp/uriTemplate.js"; -import { UriTemplate } from "@modelcontextprotocol/client"; export { expandUriTemplate, + hasRequiredValues, parseUriTemplate, templateVariables, } from "@inspector/core/mcp/uriTemplate.js"; export type { TemplatePart, TemplateVariable, + VarSpec, } from "@inspector/core/mcp/uriTemplate.js"; /** @@ -41,8 +42,10 @@ const deferredToken = (index: number) => `\u0000${index}\u0000`; * * Unfilled expressions are swapped for an inert token and restored after * expansion -- rather than expanding each filled expression in isolation -- so - * the SDK still sees one whole template and applies its cross-expression rules - * (notably rewriting a second `?` query expression to `&`). + * the expander still sees one whole template and applies its cross-expression + * rules (notably rewriting a second `?` query expression to `&`). Routing the + * rewritten template back through `expandUriTemplate` is what keeps the preview + * honest: it can never promise a URI that submitting would not send. */ export function previewUriTemplate( uriTemplate: string, @@ -55,23 +58,15 @@ export function previewUriTemplate( const rewritten = parts .map((part) => { if (part.kind === "literal") return part.text; - if (part.names.every((name) => defined[name] !== undefined)) { - // Route the kept expression through the same multi-name correction the - // real expansion applies, so the preview can never promise a URI that - // submitting would not actually send. - return applyMultiNameCorrection([part], defined); + if (part.names.some((name) => defined[name] !== undefined)) { + return part.source; } deferred.push(part.source); return deferredToken(deferred.length - 1); }) .join(""); - let expanded: string; - try { - expanded = new UriTemplate(rewritten).expand(defined); - } catch { - return uriTemplate; - } + const expanded = expandUriTemplate(rewritten, defined); // Restored by exact-string replacement rather than by a pattern: a regex // matching the token would have to embed U+0000 literally, which `eslint` diff --git a/core/mcp/inspectorClient.ts b/core/mcp/inspectorClient.ts index 9cdedd1c..2a98dc2f 100644 --- a/core/mcp/inspectorClient.ts +++ b/core/mcp/inspectorClient.ts @@ -190,12 +190,7 @@ import { convertToolParameters, convertPromptArguments, } from "../json/jsonUtils.js"; -import { UriTemplate } from "@modelcontextprotocol/client"; -import { - applyMultiNameCorrection, - definedValues, - parseUriTemplate, -} from "./uriTemplate.js"; +import { expandUriTemplateStrict } from "./uriTemplate.js"; import { InspectorClientEventTarget, type TaskWithOptionalCreatedAt, @@ -4976,20 +4971,15 @@ export class InspectorClient extends InspectorClientEventTarget { const uriTemplateString = uriTemplate; - // Expand the template's uriTemplate using the provided params. Routed - // through the shared helpers in ./uriTemplate.js so this and the web - // Resources form cannot disagree: `definedValues` makes a blank field read - // as *undefined* (the expression drops out) rather than as the empty string - // (a valueless `?topic=`), and `applyMultiNameCorrection` fixes the SDK's - // mis-expansion of `{a,b}`-style expressions before it sees them (#1919). + // Expand through the shared helper in ./uriTemplate.js so this and the web + // Resources form cannot disagree. It drops blank values so an unfilled + // optional field reads as *undefined* (the expression disappears) rather + // than as the empty string (a valueless `?topic=`), and it covers the + // expression shapes the SDK's own expander gets wrong -- `{a,b}`, `{;id}`, + // and the `{id:3}` prefix modifier (#1919). let expandedUri: string; try { - const defined = definedValues(params); - const corrected = applyMultiNameCorrection( - parseUriTemplate(uriTemplateString), - defined, - ); - expandedUri = new UriTemplate(corrected).expand(defined); + expandedUri = expandUriTemplateStrict(uriTemplateString, params); } catch (error) { throw new Error( `Failed to expand URI template "${uriTemplate}": ${error instanceof Error ? error.message : String(error)}`, diff --git a/core/mcp/uriTemplate.ts b/core/mcp/uriTemplate.ts index 0518b608..28afd7e0 100644 --- a/core/mcp/uriTemplate.ts +++ b/core/mcp/uriTemplate.ts @@ -1,18 +1,45 @@ /** * RFC 6570 URI Template parsing and expansion, shared by every client (#1919). * - * Expansion is delegated to the SDK's `UriTemplate` — with one correction - * applied first, documented on {@link expandMultiNameExpression} below. This - * lives in `core/` rather than in a client so the web form, the TUI, and the - * CLI cannot disagree about what a template means; `InspectorClient - * .readResourceFromTemplate` and the web Resources form both expand through - * {@link expandUriTemplate}. + * This lives in `core/` rather than in a client so the web Resources form, the + * TUI, and the CLI cannot disagree about what a template means: the form calls + * {@link templateVariables} / {@link expandUriTemplate} directly, and the TUI + * and CLI reach the same code through `InspectorClient.readResourceFromTemplate`. + * + * ## Why this is not simply `new UriTemplate(t).expand(v)` + * + * Expansion is delegated to the SDK's `UriTemplate` for every expression it + * handles correctly — which is the overwhelmingly common case, and keeping it + * there means we cannot drift from the SDK on the ordinary path. But its parser + * and expander are incomplete in three ways that a *form* makes visible, + * because a form has to name the variables it is asking the user to fill in. + * Each was measured against the pinned SDK, not inferred: + * + * | Shape | SDK `variableNames` | SDK expansion | Correct | + * | ------------ | ------------------- | --------------------------------- | -------------- | + * | `{a,b}` | `["a","b"]` | `foo/bar,q` — unencoded, no prefix| `foo%2Fbar,q` | + * | `{;id}` | `[";id"]` | `""` — operator unknown | `;id=7` | + * | `{id:3}` | `["id:3"]` | `""` — modifier folded into name | `abc` | + * + * For the last two the damage is not just a wrong URI: the form would render + * fields literally labelled `;id` and `id:3`, which the user cannot fill in + * usefully. So this module parses varspecs properly and, **when a template + * contains any expression the SDK gets wrong, expands that whole template + * itself** in {@link expandParts} rather than splicing corrected fragments into + * a template the SDK then re-expands — splicing would leave the SDK's + * cross-expression `?`-to-`&` rewrite unaware of the fragments we resolved. */ import { UriTemplate } from "@modelcontextprotocol/client"; -/** The RFC 6570 operators, in the order the SDK's parser tests for them. */ -const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; +/** + * The RFC 6570 operators. + * + * `;` is here but **not** in the SDK's own list, which is why `{;id}` parses + * there as a variable literally named `;id`. Order matters only in that each is + * a distinct single character; the first match wins. + */ +const OPERATORS = ["+", "#", ".", "/", ";", "?", "&"] as const; /** * Operators whose expansion omits cleanly when the variables in it are @@ -29,7 +56,20 @@ const OPERATORS = ["+", "#", ".", "/", "?", "&"] as const; * `#` is in the omittable set on the same measurement: `x://a{#frag}` with no * `frag` expands to exactly `x://a`. A fragment is optional by construction. */ -const OMITTABLE_OPERATORS = new Set(["#", ".", "/", "?", "&"]); +const OMITTABLE_OPERATORS = new Set(["#", ".", "/", ";", "?", "&"]); + +/** Operators that expand to `name=value` pairs rather than bare values. */ +const NAMED_OPERATORS = new Set([";", "?", "&"]); + +/** A single variable reference inside an expression, e.g. `id` or `id:3`. */ +export interface VarSpec { + name: string; + /** + * The RFC 6570 prefix modifier (`{id:3}`), a maximum length in *characters*. + * Applied to the value before percent-encoding, per §3.2.1. + */ + maxLength?: number; +} interface TemplateLiteral { kind: "literal"; @@ -42,7 +82,9 @@ interface TemplateExpression { source: string; /** The RFC 6570 operator, or `""` for a simple expression. */ operator: string; - /** Variable names in the expression, `*` (explode) and whitespace stripped. */ + /** The variable references, in order. */ + varspecs: VarSpec[]; + /** Bare variable names, `*` and any `:length` modifier stripped. */ names: string[]; } @@ -53,20 +95,50 @@ export interface TemplateVariable { /** The operator of the expression the variable was first seen in. */ operator: string; /** - * True when omitting the variable would change the URI's structure rather - * than shorten it — see {@link OMITTABLE_OPERATORS}. + * True when the expression this variable belongs to cannot be omitted + * without changing the URI's structure — see {@link OMITTABLE_OPERATORS}. + * + * Note this is a property of the *expression*, not of the single variable: + * RFC 6570 drops undefined names from a multi-name expression, so `{a,b}` + * with only `a` filled expands to `a`'s value. Use {@link hasRequiredValues} + * rather than testing every required variable individually, or a form will + * refuse input the expander would have accepted. */ required: boolean; + /** + * Every name in the expression this variable belongs to, itself included. + * A single-name expression yields a one-element array. + */ + groupNames: string[]; +} + +/** Parses one varspec (`id`, `id*`, `id:3`) into a name and optional prefix. */ +function parseVarSpec(raw: string): VarSpec | null { + // The explode modifier is stripped rather than honored: it only changes how + // a list or map value is joined, and every value reaching this module is a + // single string. + const spec = raw.replace("*", "").trim(); + if (spec.length === 0) return null; + + const colon = spec.indexOf(":"); + if (colon === -1) return { name: spec }; + + const name = spec.slice(0, colon); + const length = Number(spec.slice(colon + 1)); + // A malformed modifier (`{id:}`, `{id:abc}`) is not a valid varspec; keep the + // name and ignore the modifier rather than inventing a truncation. + if (name.length === 0) return null; + return Number.isInteger(length) && length > 0 + ? { name, maxLength: length } + : { name }; } /** * Splits a template into literal runs and expressions. * - * Deliberately mirrors the SDK parser's own scanning rules (first matching - * operator character wins, names split on `,`, `*` stripped) so this never - * disagrees with the class that ultimately does the expanding. An unclosed - * `{` yields a trailing literal, which is what makes the callers below degrade - * to "render no inputs, show the template verbatim" rather than throw. + * An unclosed `{` yields a trailing literal, which is what makes the callers + * below degrade to "render no inputs, show the template verbatim" rather than + * throw at the user. */ export function parseUriTemplate(uriTemplate: string): TemplatePart[] { const parts: TemplatePart[] = []; @@ -91,16 +163,17 @@ export function parseUriTemplate(uriTemplate: string): TemplatePart[] { } const body = uriTemplate.slice(i + 1, end); const operator = OPERATORS.find((op) => body.startsWith(op)) ?? ""; - const names = body + const varspecs = body .slice(operator.length) .split(",") - .map((name) => name.replace("*", "").trim()) - .filter((name) => name.length > 0); + .map(parseVarSpec) + .filter((spec): spec is VarSpec => spec !== null); parts.push({ kind: "expression", source: uriTemplate.slice(i, end + 1), operator, - names, + varspecs, + names: varspecs.map((spec) => spec.name), }); i = end + 1; } @@ -125,7 +198,12 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { if (existing) { existing.required = existing.required || required; } else { - byName.set(name, { name, operator: part.operator, required }); + byName.set(name, { + name, + operator: part.operator, + required, + groupNames: part.names, + }); } } } @@ -134,9 +212,28 @@ export function templateVariables(uriTemplate: string): TemplateVariable[] { } /** - * Drops empty entries so an untouched optional field reads as *undefined* to - * the SDK (the expression disappears) rather than as the empty string (which - * would expand to a valueless `?topic=`). + * Whether `values` supplies everything expansion structurally needs. + * + * A required *expression* is satisfied by any one of its names having a value, + * because RFC 6570 drops the undefined ones — `{a,b}` with only `a` filled + * expands to `a`'s value, which the SDK does too. Testing each required + * variable individually would block that valid input. + */ +export function hasRequiredValues( + variables: TemplateVariable[], + values: Record, +): boolean { + return variables.every( + (variable) => + !variable.required || + variable.groupNames.some((name) => (values[name] ?? "").length > 0), + ); +} + +/** + * Drops empty entries so an untouched optional field reads as *undefined* + * (the expression disappears) rather than as the empty string (which would + * expand to a valueless `?topic=`). */ export function definedValues( values: Record, @@ -154,103 +251,156 @@ function encodeValue(value: string, operator: string): string { } /** - * Expands a **multi-name, non-query** expression (`{a,b}`, `{/a,b}`, …). + * Applies a prefix modifier, then encodes. * - * The pinned SDK gets this branch wrong: `UriTemplate.expandPart` takes an - * early `part.names.length > 1` path that raw-joins the values with `,` — - * skipping `encodeValue` *and* the operator prefix entirely. Measured against - * the pinned SDK, `x://{a,b}` with `a = "foo/bar"` expands to `x://foo/bar,q` - * (unencoded, so the slash creates a path segment — the very defect #1919 is - * about), and `x://a{/p,q}` expands to `x://ax y,z` — no leading `/`, spaces - * intact. Only the `?`/`&` operators are handled correctly there, because they - * are dispatched before that branch. - * - * So those expressions are expanded here and spliced into the template as - * *literal* text before the SDK ever sees them. This is deliberately surgical: - * every other shape — the overwhelmingly common single-name expression, and - * every query expression — still goes through the SDK untouched, so the two - * cannot drift on the ordinary path, and if the SDK fixes its branch this - * correction keeps producing the same (correct) answer. - * - * Splicing is safe because both encoders escape `{` and `}` (to `%7B`/`%7D`), - * so an expanded value can never be re-parsed as an expression. - * - * Returns `""` when no name in the expression has a value, matching RFC 6570's - * rule that an expression with only undefined variables expands to nothing. + * Truncation is by *code point* (`Array.from`), not by `slice`: RFC 6570 counts + * the prefix in characters, and `String.prototype.slice` counts UTF-16 code + * units, so it can cut an astral character in half and yield a lone surrogate. */ -function expandMultiNameExpression( +function renderValue(value: string, spec: VarSpec, operator: string): string { + const truncated = + spec.maxLength === undefined + ? value + : Array.from(value).slice(0, spec.maxLength).join(""); + return encodeValue(truncated, operator); +} + +/** + * True for an expression the SDK would get wrong, and which this module must + * therefore expand itself. See the table in the module comment. + */ +function needsOwnExpansion(part: TemplatePart): boolean { + if (part.kind !== "expression") return false; + return ( + // Multi-name, non-query: the SDK raw-joins, skipping encoding and prefix. + (part.varspecs.length > 1 && + part.operator !== "?" && + part.operator !== "&") || + // The SDK has no `;` operator at all. + part.operator === ";" || + // The SDK folds everything after a `:` into the variable name. Keyed on the + // raw source rather than on a parsed `maxLength` so a *malformed* modifier + // (`{id:}`, `{id:abc}`) is caught too: this module drops the modifier and + // looks up `id`, while the SDK would look up `id:` and find nothing. + part.source.includes(":") + ); +} + +/** Expands one expression per RFC 6570. Returns "" if no name has a value. */ +function expandExpression( part: TemplateExpression, values: Record, ): string { - const encoded = part.names - .map((name) => values[name]) - .filter((value) => value !== undefined) - .map((value) => encodeValue(value, part.operator)); + const present = part.varspecs.filter( + (spec) => values[spec.name] !== undefined, + ); + if (present.length === 0) return ""; - if (encoded.length === 0) return ""; + const { operator } = part; - switch (part.operator) { + if (NAMED_OPERATORS.has(operator)) { + const pairs = present.map( + (spec) => + `${spec.name}=${renderValue(values[spec.name], spec, operator)}`, + ); + // `;` repeats its separator per pair; `?`/`&` join with `&`. + return operator === ";" + ? `;${pairs.join(";")}` + : `${operator}${pairs.join("&")}`; + } + + const rendered = present.map((spec) => + renderValue(values[spec.name], spec, operator), + ); + + switch (operator) { case "#": - return `#${encoded.join(",")}`; + return `#${rendered.join(",")}`; case ".": - return `.${encoded.join(".")}`; + return `.${rendered.join(".")}`; case "/": - return `/${encoded.join("/")}`; + return `/${rendered.join("/")}`; // "" and "+" — a bare comma-joined list, no prefix. default: - return encoded.join(","); + return rendered.join(","); } } /** - * True for the expressions {@link expandMultiNameExpression} has to take over: - * more than one name, and not a query operator (which the SDK dispatches before - * its broken branch and therefore handles correctly). + * Expands a whole parsed template, mirroring the SDK's `expand` — including its + * rule that a second query expression switches its leading `?` to `&`. */ -function needsMultiNameCorrection(part: TemplateExpression): boolean { - return ( - part.names.length > 1 && part.operator !== "?" && part.operator !== "&" - ); -} - -/** - * Rebuilds `uriTemplate` with every mis-expanded multi-name expression already - * resolved to literal text, leaving the rest for the SDK. - */ -export function applyMultiNameCorrection( +export function expandParts( parts: TemplatePart[], values: Record, ): string { - return parts - .map((part) => { - if (part.kind === "literal") return part.text; - return needsMultiNameCorrection(part) - ? expandMultiNameExpression(part, values) - : part.source; - }) - .join(""); + let result = ""; + let hasQueryParam = false; + + for (const part of parts) { + if (part.kind === "literal") { + result += part.text; + continue; + } + const expanded = expandExpression(part, values); + if (!expanded) continue; + + const isQuery = part.operator === "?" || part.operator === "&"; + result += isQuery && hasQueryParam ? `&${expanded.slice(1)}` : expanded; + if (isQuery) hasQueryParam = true; + } + + return result; } /** * Expands a template against the entered values per RFC 6570 — percent-encoding * each value according to its operator, and omitting expressions whose - * variables were left blank. + * variables were left blank. **Throws** on a template that is not valid. * - * A template the SDK refuses to parse falls back to the raw template string, - * which is what the user already sees in the preview and what the server will - * reject with a legible error; throwing here would take out the whole panel. + * Templates the SDK handles correctly still go through the SDK, so the two + * cannot drift on the ordinary path; only a template containing at least one + * expression from the table above is expanded here instead. That is a + * whole-template switch rather than a per-expression splice so the `?`-to-`&` + * rewrite always sees every expression that actually produced output. + * + * The SDK template is constructed *before* choosing a path, and unconditionally: + * that construction is what validates the syntax and throws `Unclosed template + * expression`, and callers such as `readResourceFromTemplate` wrap that error. + * Skipping it on the own-expansion path would silently accept a template like + * `{;a}{b,c` — this module's parser treats the unclosed tail as literal text, + * so nothing else would object. + */ +export function expandUriTemplateStrict( + uriTemplate: string, + values: Record, +): string { + const defined = definedValues(values); + const sdkTemplate = new UriTemplate(uriTemplate); + const parts = parseUriTemplate(uriTemplate); + return parts.some(needsOwnExpansion) + ? expandParts(parts, defined) + : sdkTemplate.expand(defined); +} + +/** + * {@link expandUriTemplateStrict}, but falling back to the raw template string + * instead of throwing. + * + * This is the form's variant: the template comes from the connected server, not + * from the user, so an invalid one is not something the user can fix from the + * panel — and what they already see in the URI preview is the raw template. + * Letting it throw would take out the whole panel on render; returning it + * unchanged sends the server a URI it rejects with a legible error instead. + * Call sites that need the failure (`readResourceFromTemplate`, which wraps it + * with the template name) use the strict variant. */ export function expandUriTemplate( uriTemplate: string, values: Record, ): string { - const defined = definedValues(values); try { - const corrected = applyMultiNameCorrection( - parseUriTemplate(uriTemplate), - defined, - ); - return new UriTemplate(corrected).expand(defined); + return expandUriTemplateStrict(uriTemplate, values); } catch { return uriTemplate; }