-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat(rest): allow -
to be used for path template variable names
#2724
Conversation
-
-
@raymondfeng, I think some changes in this PR overlaps your other PR #2722. Are we going to abandon #2722, or they are meant to be separate changes? |
Some specs use `{account-id}` as part of the path.
7fc2800
to
379a4a9
Compare
-
-
to be used for path template variable names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add end-to-end tests for both /{foo-bar}
and especially for /{foo}-{bar}
to verify that we can route such paths correctly.
|
||
it('allows /{foo}-{bar}', () => { | ||
const path = validateApiPath('/{foo}-{bar}'); | ||
expect(path).to.eql('/{foo}-{bar}'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a valid OpenAPI path? Will our two router implementations correctly handle such path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. It turns out that -
is not allowed by path-to-regexp
as it does not match \w
. I'll investigate more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question as @bajtos, but LGTM otherwise.
Close this PR with an alternate fix - b6e02fc. |
@raymondfeng How did this commit get into |
An OpenAPI spec for banking uses
{account-id}
as path variable names. Before this change,@loopback/rest
reports it as invalid parameter name.Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈