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鈥檒l 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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@raymondfeng
Copy link
Member

commented Apr 11, 2019

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 machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@raymondfeng raymondfeng requested a review from bajtos as a code owner Apr 11, 2019

@raymondfeng raymondfeng changed the title Relax template variable names to allow `-` feat(rest): Relax template variable names to allow `-` Apr 11, 2019

@dhmlau

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

@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?

feat(rest): allow `-` to be used for path template variable names
Some specs use `{account-id}` as part of the path.

@raymondfeng raymondfeng force-pushed the relax-template-varname branch from 7fc2800 to 379a4a9 Apr 11, 2019

@raymondfeng raymondfeng changed the title feat(rest): Relax template variable names to allow `-` feat(rest): allow `-` to be used for path template variable names Apr 11, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

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?

It was a mistake from branching. Fixed.

@hacksparrow hacksparrow self-requested a review Apr 11, 2019

@hacksparrow
Copy link
Member

left a comment

LGTM

@jannyHou
Copy link
Contributor

left a comment

馃憤

@bajtos

bajtos approved these changes Apr 11, 2019

Copy link
Member

left a comment

LGTM.

@bajtos
Copy link
Member

left a comment

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}');

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 11, 2019

Member

Is this a valid OpenAPI path? Will our two router implementations correctly handle such path?

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Apr 11, 2019

Author Member

Good catch. It turns out that - is not allowed by path-to-regexp as it does not match \w. I'll investigate more.

@b-admike
Copy link
Member

left a comment

Same question as @bajtos, but LGTM otherwise.

@bajtos bajtos added bug REST labels Apr 11, 2019

@dhmlau dhmlau referenced this pull request Apr 11, 2019

Closed

Monthly Milestone - April 2019 馃尶馃惏鈽旓笍 #2669

20 of 46 tasks complete
@raymondfeng

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Close this PR with an alternate fix - b6e02fc.

@raymondfeng raymondfeng deleted the relax-template-varname branch Apr 11, 2019

@dhmlau dhmlau added this to the April 2019 milestone milestone Apr 15, 2019

@bajtos

This comment has been minimized.

Copy link
Member

commented Apr 16, 2019

Close this PR with an alternate fix - b6e02fc.

@raymondfeng How did this commit get into master? GitHub is not showing any linked pull request to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.