Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(o2k) update path param regex to match 1 segment only #3298

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Apr 16, 2021

existing regex \S+ is equivalent to [^\s]+ by adding the / character as not allowed, we limit the regex to match only 1 path segment

This path: /tracks/{hello}/world/{there}
Would create: /tracks/(?<hello>\S+)/world/(?<there>\S+)$

When published, this path matches /tracks/xxx/world/yyy as it should
With captures:
hello: xxx
there: yyy

This path also matches /tracks/xxx/zzz/world/yyy/andthensome but it should not
With captures:
hello: xxx/zzz
there: yyy/andthensome

The last one should have matched only a single path segment, but actually matches multiple.

@Tieske
Copy link
Member Author

Tieske commented Apr 16, 2021

the CI failure doesn't provide a meaningfull hint (at least to me) as to what failed... any help appreciated

@develohpanda develohpanda added the PA-openapi-2-kong Package: OpenAPI 2 Kong label Apr 19, 2021
Copy link
Contributor

@develohpanda develohpanda left a comment

Choose a reason for hiding this comment

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

LGTM after the test fixes

image

Comment on lines 190 to 192
expect(pathVariablesToRegex('/foo/{bar}/{baz}')).toBe(
'/foo/(?<bar>[^\\/\\r\\n\\t\\f\\v ]+)/(?<baz>[^\\/\\r\\n\\t\\f\\v ]+)$',
);
Copy link
Contributor

Choose a reason for hiding this comment

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

The CI failure is to do with this test. The expected result is incorrect? I'm looking at the updated implementation of pathVariablesToRegex() below and it matches what the received result is, not the expected result.

image

Since \s is equivalent to \r\n\t\f\v , just need to update the expected result.

@@ -59,7 +59,7 @@ describe('services', () => {
name: 'My_API-birds_id-get',
strip_path: false,
methods: ['GET'],
paths: ['/birds/(?<id>\\S+)$'],
paths: ['/birds/(?<id>[^\\/\\r\\n\\t\\f\\v ]+)$'],
Copy link
Contributor

Choose a reason for hiding this comment

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

CI also fails with this test for the same reason as my previous comment

image

@@ -68,7 +68,7 @@ export function generateSlug(str: string, options: SlugifyOptions = {}): string
const pathVariableSearchValue = /{([^}]+)}(?!:\/\/)/g;

export function pathVariablesToRegex(p: string): string {
const result = p.replace(pathVariableSearchValue, '(?<$1>\\S+)');
const result = p.replace(pathVariableSearchValue, '(?<$1>[^\\/\\s]+)');
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be worth adding a comment here to explain this regex, something like

Suggested change
const result = p.replace(pathVariableSearchValue, '(?<$1>[^\\/\\s]+)');
// match any non-whitespace character, excluding /
const result = p.replace(pathVariableSearchValue, '(?<$1>[^\\/\\s]+)');

existing regex '\S+' is equivalent to '[^\s]+' by adding the
'/' character as not allowed, we limit the regex to match only 1 path
segment

This path: "/tracks/{hello}/world/{there}”
Would create: “/tracks/(?<hello>\S+)/world/(?<there>\S+)$”

This path works as expected:
/tracks/xxx/world/yyy
With captures:
  hello: xxx
  there: yyy

This path also works, unexpected:
/tracks/xxx/zzz/world/yyy/andthensome
With captures:
  hello: xxx/zzz
  there: yyy/andthensome

The last one should have matched only a single path segment, but
actually matches multiple.
@develohpanda develohpanda merged commit 40deacb into develop Apr 19, 2021
@develohpanda develohpanda deleted the fix/path-param-regex branch April 19, 2021 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PA-openapi-2-kong Package: OpenAPI 2 Kong
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants