Skip to content

[core-http] Fix utils isValidUuid() regression#7387

Merged
jeremymeng merged 4 commits intoAzure:masterfrom
jeremymeng:fix-isvalidduuid
Feb 14, 2020
Merged

[core-http] Fix utils isValidUuid() regression#7387
jeremymeng merged 4 commits intoAzure:masterfrom
jeremymeng:fix-isvalidduuid

Conversation

@jeremymeng
Copy link
Copy Markdown
Member

The regression was introduced in #4916, when the local variable validUuidRegex was moved out of the function into a const. Regex test() with global flag set will change the state of the regex (advancing the lastIndex) and cause next test() to fail in our case. We don't need the global flag here for validating a uuid string.

This change removes the global flag from validUuidRegex constant.

I also remove usage of regex global flags in serializer.ts since they are all used to match single lines.

It caused the next isValidUuid() call to return false.  According to MDN:

> When a regex has the global flag set, test() will advance the lastIndex of the regex. (RegExp.prototype.exec() also advances the lastIndex property.)

Further calls to test(str) will resume searching str starting from lastIndex. The lastIndex property will continue to increase each time test() returns true.

Note: As long as test() returns true, lastIndex will not reset—even when testing a different string!

When test() returns false, the calling regex's lastIndex property will reset to 0.
@jeremymeng
Copy link
Copy Markdown
Member Author

Or we could use match() or search() which shouldn't have this issue.

Copy link
Copy Markdown
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Great find!!

Do you think this is something we can/should make a lint rule about? If so, can you log an issue for that.

Copy link
Copy Markdown
Contributor

@chradek chradek left a comment

Choose a reason for hiding this comment

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

Good catch! And I learned something new: I didn't realize using a regex with global set with test() used lastIndex to keep track of state across invocations:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/lastIndex#Description

@jeremymeng
Copy link
Copy Markdown
Member Author

Do you think this is something we can/should make a lint rule about? If so, can you log an issue for that.

yes it would be useful to have a rule for warning against test() and global flag. As for global flag in general it depends as it might be intended.

@jeremymeng
Copy link
Copy Markdown
Member Author

Logged #7399 for potential lint rule

@jeremymeng jeremymeng changed the title [core-http] Fix utils isValiddUuid() regression [core-http] Fix utils isValidUuid() regression Feb 14, 2020
@jeremymeng jeremymeng merged commit 722a1b8 into Azure:master Feb 14, 2020
@jeremymeng jeremymeng deleted the fix-isvalidduuid branch February 14, 2020 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants