Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/drift-multi-parameter-path-segments.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@redocly/cli': patch
---

Fixed `drift` and `coverage` failing to match a path template whose segment mixes literal text with parameters, such as `/instances/{worldId}:{instanceId}`.
Only a segment that was entirely one parameter was recognized, so these templates were compiled as literal text and never matched any request.
Affected requests were reported as undocumented by `drift` and left out of the `coverage` figures.
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import { compileOpenApiPath } from '../utils/http.js';

describe('compileOpenApiPath', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for the fix — the matching logic is correct, but the generated regexes need a performance improvement before this can merge.

Each parameter compiles to a lazy ([^/]+?) group, so when a segment holds several parameters, the regex engine tries every possible way to split a non-matching value between them — super-linear backtracking. Measured on this branch, a single exec call:

  • {a}:{b}:{c}.json against a 5 KB non-matching path: ~2 s
  • {a}:{b}:{c}:{d}:{e}.json against the same input: >120 s (had to kill the process)

Since exec is synchronous, this blocks the event loop — drift just hangs, no error, no timeout. Captures routinely contain long junk paths (scanner traffic, malformed URLs), so this is reachable in normal use, and main isn't affected today because it never emits more than one group per segment.

Could you rework the compilation so matching stays linear no matter how many parameters a segment holds? Here's a test that pins the requirement — it currently fails at ~2,000 ms:

it('rejects a long non-matching value without backtracking blow-up', () => {
  const { regex } = compileOpenApiPath('/v1/{a}:{b}:{c}.json');
  const longPath = '/v1/' + 'a:'.repeat(2500) + 'a';

  const startedAt = performance.now();
  expect(regex.exec(longPath)).toBeNull();
  expect(performance.now() - startedAt).toBeLessThan(100);
});

Please also add this test which ensures that we don't introduce solution that may break trailing literal handling:

it('lets a parameter before a trailing literal contain the literal separator char', () => {
  const { regex } = compileOpenApiPath('/files/{name}.json');

  expect(regex.exec('/files/report.v1.json')?.[1]).toBe('report.v1');
});

Matching behavior shouldn't change — e.g. /files/{name}.json must still match /files/report.v1.json with name = report.v1, so simply excluding the separator characters from the parameter's character class isn't enough.

it('compiles a segment that is a single parameter', () => {
const { regex, params } = compileOpenApiPath('/users/{userId}');

expect(params).toEqual(['userId']);
expect(regex.exec('/users/usr_abc')?.[1]).toBe('usr_abc');
});

it('does not let a parameter span a path separator', () => {
const { regex } = compileOpenApiPath('/users/{userId}');

expect(regex.exec('/users/usr_abc/friends')).toBeNull();
});

it('compiles two parameters separated by a literal inside one segment', () => {
const { regex, params } = compileOpenApiPath('/instances/{worldId}:{instanceId}');

expect(params).toEqual(['worldId', 'instanceId']);

const match = regex.exec(
'/instances/wrld_a:85981~group(grp_b)~groupAccessType(public)~region(us)'
);
expect(match?.[1]).toBe('wrld_a');
expect(match?.[2]).toBe('85981~group(grp_b)~groupAccessType(public)~region(us)');
});

it('splits on the first separator when the trailing value holds another', () => {
const { regex } = compileOpenApiPath('/instances/{worldId}:{instanceId}');
const match = regex.exec('/instances/wrld_a:12345~region(us):extra');

expect(match?.[1]).toBe('wrld_a');
expect(match?.[2]).toBe('12345~region(us):extra');
});

it('keeps matching a multi-parameter segment when a suffix segment follows', () => {
const { regex } = compileOpenApiPath('/instances/{worldId}:{instanceId}/shortName');

expect(regex.exec('/instances/wrld_a:123~private(usr_b)/shortName')?.[2]).toBe(
'123~private(usr_b)'
);
});

it('ranks a partially literal segment above a bare parameter', () => {
expect(compileOpenApiPath('/instances/{worldId}:{instanceId}').score).toBeGreaterThan(
compileOpenApiPath('/instances/{instanceId}').score
);
});

it('treats a segment with no parameters as a literal', () => {
const { regex, params } = compileOpenApiPath('/instances/recent');

expect(params).toEqual([]);
expect(regex.exec('/instances/anything')).toBeNull();
expect(regex.exec('/instances/recent')).not.toBeNull();
});
});
32 changes: 27 additions & 5 deletions packages/cli/src/commands/drift/utils/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,14 +167,36 @@ export function compileOpenApiPath(pathTemplate: string): {
return '';
}

const paramMatch = segment.match(/^\{([^}]+)\}$/);
if (paramMatch) {
// A segment may hold several parameters around literal text, as in
// `/instances/{worldId}:{instanceId}`. Matching the whole segment as one
// parameter would miss those, and treating it as a literal never matches.
const paramsBefore = params.length;
let compiled = '';
let literalLength = 0;
let offset = 0;

for (const paramMatch of segment.matchAll(/\{([^}]+)\}/g)) {
const literal = segment.slice(offset, paramMatch.index);

compiled += escapeRegex(literal);
literalLength += literal.length;
params.push(paramMatch[1]);
return '([^/]+)';
compiled += '([^/]+?)';
offset = paramMatch.index + paramMatch[0].length;
}

score += 2;
return escapeRegex(segment);
const trailing = segment.slice(offset);
compiled += escapeRegex(trailing);
literalLength += trailing.length;

if (params.length === paramsBefore) {
score += 2;
} else if (literalLength > 0) {
// More specific than a bare parameter, less so than a whole literal.
score += 1;
}

return compiled;
})
.join('/');

Expand Down