Repository navigation
Conversation
🦋 Changeset detectedLatest commit: b188b5d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/schema-routes/schema-routes.ts">
<violation number="1" location="src/schema-routes/schema-routes.ts:158">
P3: The optional trailing-colon branch `(?::(?=\/|$))?` — the legacy `:param:` form — is promoted in the changeset as a supported fallback, but the new regression test in tests/spec/issue-1833/basic.test.ts only covers mid-segment colons and segment-start params. No test exercises the legacy form (e.g. `/users/:id:` at route end or `/api/:id:/archive` where the trailing colon is consumed before `/`), so that documented branch can break silently. Add a route with a trailing-colon param to the test and assert the generated path drops the delimiter colon.</violation>
<violation number="2" location="src/schema-routes/schema-routes.ts:158">
P3: The new `(?<=\/)` lookbehind requires a leading `/` before `:param`, so a param that starts the very first segment of the route is no longer recognized when the route string does not start with `/`. Every valid OpenAPI path begins with `/`, but `onPreBuildRoutePath` can return a slash-less rewrite (e.g. `"user/:id"` or `":id"`): old code parsed the leading `:param`, new code leaks it as literal URL text with no path argument generated. Consider matching a segment start too (`(?:^|(?<=\/)):`) and deriving the replacement prefix from the character before the match instead of assuming `/`, or document that hook-rewritten routes must keep the leading slash.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
| // a mid-segment colon (e.g. AIP-136 `/v1/items:search`) is a literal suffix. | ||
| const pathParamMatches = (routeName || "").match( | ||
| /({[\w[\\\]^`][-_.\w]*})|(:[\w[\\\]^`][-_.\w]*:?)/g, | ||
| /({[\w[\\\]^`][-_.\w]*})|((?<=\/):[\w[\\\]^`][-_.\w]*(?::(?=\/|$))?)/g, |
There was a problem hiding this comment.
P3: The optional trailing-colon branch (?::(?=\/|$))? — the legacy :param: form — is promoted in the changeset as a supported fallback, but the new regression test in tests/spec/issue-1833/basic.test.ts only covers mid-segment colons and segment-start params. No test exercises the legacy form (e.g. /users/:id: at route end or /api/:id:/archive where the trailing colon is consumed before /), so that documented branch can break silently. Add a route with a trailing-colon param to the test and assert the generated path drops the delimiter colon.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/schema-routes/schema-routes.ts, line 158:
<comment>The optional trailing-colon branch `(?::(?=\/|$))?` — the legacy `:param:` form — is promoted in the changeset as a supported fallback, but the new regression test in tests/spec/issue-1833/basic.test.ts only covers mid-segment colons and segment-start params. No test exercises the legacy form (e.g. `/users/:id:` at route end or `/api/:id:/archive` where the trailing colon is consumed before `/`), so that documented branch can break silently. Add a route with a trailing-colon param to the test and assert the generated path drops the delimiter colon.</comment>
<file context>
@@ -152,8 +152,10 @@ export class SchemaRoutes {
+ // a mid-segment colon (e.g. AIP-136 `/v1/items:search`) is a literal suffix.
const pathParamMatches = (routeName || "").match(
- /({[\w[\\\]^`][-_.\w]*})|(:[\w[\\\]^`][-_.\w]*:?)/g,
+ /({[\w[\\\]^`][-_.\w]*})|((?<=\/):[\w[\\\]^`][-_.\w]*(?::(?=\/|$))?)/g,
);
</file context>
| // a mid-segment colon (e.g. AIP-136 `/v1/items:search`) is a literal suffix. | ||
| const pathParamMatches = (routeName || "").match( | ||
| /({[\w[\\\]^`][-_.\w]*})|(:[\w[\\\]^`][-_.\w]*:?)/g, | ||
| /({[\w[\\\]^`][-_.\w]*})|((?<=\/):[\w[\\\]^`][-_.\w]*(?::(?=\/|$))?)/g, |
There was a problem hiding this comment.
P3: The new (?<=\/) lookbehind requires a leading / before :param, so a param that starts the very first segment of the route is no longer recognized when the route string does not start with /. Every valid OpenAPI path begins with /, but onPreBuildRoutePath can return a slash-less rewrite (e.g. "user/:id" or ":id"): old code parsed the leading :param, new code leaks it as literal URL text with no path argument generated. Consider matching a segment start too ((?:^|(?<=\/)):) and deriving the replacement prefix from the character before the match instead of assuming /, or document that hook-rewritten routes must keep the leading slash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/schema-routes/schema-routes.ts, line 158:
<comment>The new `(?<=\/)` lookbehind requires a leading `/` before `:param`, so a param that starts the very first segment of the route is no longer recognized when the route string does not start with `/`. Every valid OpenAPI path begins with `/`, but `onPreBuildRoutePath` can return a slash-less rewrite (e.g. `"user/:id"` or `":id"`): old code parsed the leading `:param`, new code leaks it as literal URL text with no path argument generated. Consider matching a segment start too (`(?:^|(?<=\/)):`) and deriving the replacement prefix from the character before the match instead of assuming `/`, or document that hook-rewritten routes must keep the leading slash.</comment>
<file context>
@@ -152,8 +152,10 @@ export class SchemaRoutes {
+ // a mid-segment colon (e.g. AIP-136 `/v1/items:search`) is a literal suffix.
const pathParamMatches = (routeName || "").match(
- /({[\w[\\\]^`][-_.\w]*})|(:[\w[\\\]^`][-_.\w]*:?)/g,
+ /({[\w[\\\]^`][-_.\w]*})|((?<=\/):[\w[\\\]^`][-_.\w]*(?::(?=\/|$))?)/g,
);
</file context>
The parser preserves literal colon suffixes in generated URLs and recognizes colon parameters only at the start of a path segment.
Problem
/v1/items:searchproduced a requiredsearch: stringargument and the URL/v1/items${search}./v1/items/{itemId}:archivehad the same extra-argument and suffix-loss problem.Solution
src/schema-routes/schema-routes.tsuses the same segment boundary for parameter detection and replacement.A literal suffix cannot capture a later parameter's substitution.
The parser consumes a legacy trailing colon only before
/or the route end, so/:itemId:archiveretains its suffix.The parser retains
/users/:userIdand brace parameters for compatibility with existing clients.Full Express route grammar remains outside scope: embedded colon syntax becomes literal path text.
Verification
The inline OpenAPI regression test in
tests/spec/issue-1833/basic.test.tsfails on the baseline and passes with this fix.The package test suite passes.
Closes #1833.
Summary by cubic
Fixes routes with AIP-136 custom methods (like
/v1/items:searchor/v1/items/{itemId}:archive) generating a bogus requiredstringargument and dropping the literal:suffixfrom the request URL.:paramNameis now parsed as an Express-style path parameter only at the start of a path segment, so/users/:userIdkeeps working while mid-segment colons stay part of the literal path. A legacy trailing colon is consumed only before/or the route end, so/:itemId:archiveretains its suffix. Added a regression test intests/spec/issue-1833/basic.test.tsthat fails on the baseline and passes with the fix.Written for commit b188b5d. Summary will update on new commits.