Skip to content
This repository has been archived by the owner on May 5, 2023. It is now read-only.

[ms-rest] Using \W instead of \w for regex match to include some characters #2175

Closed
wants to merge 1 commit into from
Closed

[ms-rest] Using \W instead of \w for regex match to include some characters #2175

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion runtime/ms-rest/lib/webResource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ class WebResource {
}
let baseUrl = options.baseUrl;
let url = baseUrl + (baseUrl.endsWith('/') ? '' : '/') + (options.pathTemplate.startsWith('/') ? options.pathTemplate.slice(1) : options.pathTemplate);
let segments = url.match(/({\w*\s*\w*})/ig);
let segments = url.match(/({\W*\S*\W*})/ig);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the fix. With this change, a text like {foo bar} that used to be a match for the old pattern, will no longer be a match. Is that intentional, why?

In all, this change can be summarized as so :

  1. change looking for any word character to any non word character
  2. change looking for a space to any non-whitespace character

I don't understand this change, feel free to drop by and talk to me 😃

Copy link

Choose a reason for hiding this comment

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

Yeah, not sure I understand it either. Is "-" the only special char we want to allow or do we allow others? if so, should we just have the expression accept "-" between words?
I'm not sure about the original regex either though, do we allow spaces in between words for the property inside {}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@balajikris @veronicagg Yes those are valid questions. I am also not sure why the initial regex was allowing white space / tabs between {}.

Secondly, after looking around I saw that technically swagger path templating & rfc6570 does not say that - or space is not allowed. Moreover, sway and it's regex matcher intentionally decided not to support -.

The question remains for us is, path /storage/{storage-account-name} seems to be valid in keyvault/2016-10-01/swagger/keyvault.json with parameter defined as "name": "storage-account-name". The swagger validation tool does not complain about semantic validation but

  1. From linter perspective, do we have any reason for this to be camelCased? For example similar to Rule M3016
  2. From model validation perspective how to approach this?

Let me know your thoughts!!

if (segments && segments.length) {
if (options.pathParameters === null || options.pathParameters === undefined || typeof options.pathParameters !== 'object') {
throw new Error(`pathTemplate: ${options.pathTemplate} has been provided. Hence, options.pathParameters ` +
Expand Down