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

RegExp with global flag does not match the request path #2134

Closed
nivsherf opened this issue Jan 15, 2021 · 2 comments · Fixed by #2135, github/rally#242 or dudleycarr/nsqjs#323
Closed

RegExp with global flag does not match the request path #2134

nivsherf opened this issue Jan 15, 2021 · 2 comments · Fixed by #2135, github/rally#242 or dudleycarr/nsqjs#323

Comments

@nivsherf
Copy link

nivsherf commented Jan 15, 2021

What is the expected behavior?
When using a RegExp in the path, I expect nock to match regardless of if the RegExp has the global flag set.

const scope = nock('https://api.github.com')
    .get(/repos.*/g).reply(200, { foo: 'bar' })
const response = await request.get('https://api.github.com/repos/atom/atom/license');
console.log(scope.isDone()); // I expect this to equal true

What is the actual behavior?
A global RegExp does not match even when it should.

// without global flag - /repos.*/ - matches
let scope = nock('https://api.github.com')
    .get(/repos.*/).reply(200, { foo: 'bar' })
const response = await request.get('https://api.github.com/repos/atom/atom/license');
console.log(scope.isDone()); // true

// with global flag - /repos.*/g - doesn't match
scope = nock('https://api.github.com')
    .get(/repos.*/g).reply(200, { foo: 'bar' })
const response = await request.get('https://api.github.com/repos/atom/atom/license');
console.log(scope.isDone()); // false

Possible solution
Since I can see no reason why would someone need global matching here (though it was actually used in a codebase I'm working on, and broke after upgrading nock), and this is broken for quite some time (git bisect shows that the offending commit is c1f7082 from July 19), I suggest throwing an explicit error whenever someone tries to pass a RegExp with the global flag.
If you think that this is a reasonable solution, I will submit a PR.

How to reproduce the issue
Here is the script I used with git bisect

Does the bug have a test case?

Versions

Software Version(s)
Nock v13.0.5
Node v14.15.3
TypeScript
@mastermatt
Copy link
Member

Interesting find. Thanks @nivsherf.

The issue with global flags in this case is that they maintain state. MDN docs for RegExp.prototype.test()

As with exec() (or in combination with it), test() called multiple times on the same global regular expression instance will advance past the previous match.

Which leads to this fun bit of JS quirkiness:

const reg = /repos.*/g;
const str = '/repos/atom/atom/license';
console.log(reg.test(str), reg.test(str), reg.test(str), reg.test(str));
> true false true false

While it isn't sensical for the regexp instance to have a global flag in this use case, I do consider it a bug that Nock isn't consistent with its behavior. Throwing an error would be an option, but so would setting the lastIndex to zero before calling test. The latter would align with legacy behavior and seems like the better user experience to me. I'm curious if anyone would prefer the error.

I have a patch written already, PR inbound...

@mastermatt mastermatt added the bug label Jan 19, 2021
mastermatt added a commit to mastermatt/nock that referenced this issue Jan 19, 2021
When matching via a regexp instance, consistently test the entire target
string.
Sets the `lastIndex` to zero for cases where the `global` flag is set
and the
regexp instance has been tested previously.

fixes: nock#2134
mastermatt added a commit to mastermatt/nock that referenced this issue Jan 19, 2021
When matching via a regexp instance, consistently test the entire target
string.
Sets the `lastIndex` to zero for cases where the `global` flag is set
and the
regexp instance has been tested previously.

fixes: nock#2134
mastermatt added a commit that referenced this issue Jan 20, 2021
When matching via a regexp instance, consistently test the entire target string.
Sets the `lastIndex` to zero for cases where the `global` flag is set and the
regexp instance has been tested previously.

fixes: #2134
@github-actions
Copy link

🎉 This issue has been resolved in version 13.0.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment