refactor: overhaul body and query matching #1632

mastermatt · 2019-07-18T20:34:36Z

Drop deep-equal and qs dependencies.

Replace qs.parse with querystring.parse from the std lib.
The main difference between the two being that qs "unpacks" nested
objects when the search keys use JSON path notation eg "foo[bar][0]".
This has no affect on URL encoded form bodies during matching because
we manually convert the notation to nested objects for comparison now,
which means users can now provide expectations in either form.
Similarly, when matching search params using a provided object, there
is no net effect. The only breaking change is when a user provides a
callback function to .query().

Replace deep-equal with a custom utility function in common.js.
Doing so not only dropped a dependency, it also achieved a more generic
utility for our use case with less code.

Interceptor matching had some refactoring:

Query matching was moved to its own method in an effort to isolate concerns.
Matching the request method was moved to the top of the match method.
Debugging statements were updated as a baby step towards having more insight into why a match was missed.

Fixes #507
Fixes #1552

BREAKING CHANGE: The only argument passed to the Interceptor.query
callback now receives the output from querystring.parse instead of
qs.parse. This means that instead of nested objects the argument will
be a flat object.

Also don't bother calling `transformRequestBodyFunction` or `matchBody` if there is no request body on the Interceptor.

Drop `deep-equal` and `qs` dependency. Replace `qs.parse` with `querystring.parse` from the std lib. The main difference between the two being that `qs` "unpacks" nested objects when the search keys use JSON path notation eg "foo[bar][0]". This has no affect on URL encoded form bodies during matching because we manually convert the notation to nested objects for comparison now, which means users can now provide expectations in either form. Similarly, when matching search params using a provided object, there is no net affect. The only breaking change is when a user provides a callback function to `.query()`. Replace `deep-equal` with a custom utility function in common.js. Doing so not only dropped a dependency, it also achieved a more generic utility for our usec ase with less code. Interceptor matching had some refactoring: - Query matching was moved to its own method in an effort to isolate concerns. - Matching the request method was moved to the top of the match method. - Debugging statements were updated as a baby step towards having more insight into why a match was missed. Fixes nock#507 Fixes nock#1552 BREAKING CHANGE: The only argument passed to the Interceptor.query callback now receives the output from querystring.parse instead of qs.parse. This means that instead of nested objects the argument will be a flat object.

mastermatt · 2019-07-18T20:35:17Z

tests/test_query.js

@@ -412,3 +412,35 @@ test('query(true) will match when the path has no query', t => {
    t.end()
  })
 })
+


These two tests are the regression tests for #1552

gr2m

Looks great 👍

paulmelnikow · 2019-07-19T16:22:58Z

I'd like to dig into this a little bit, mostly in thinking clearly about exactly what's changed.

I'm fine with the breaking change to the query() callback, though we should add a note to the API documentation (probably documenting both the 10.x and 11.x behaviors).

mastermatt · 2019-07-19T16:46:09Z

@paulmelnikow sounds good. I'm going to hold off on the doc updates until you're ok with the change themselves.

paulmelnikow

👍

Thanks for giving me the time to read this!

I want to add that I'm in two minds about whether we should support this behavior. It is so common in frameworks that it is a kind of de facto standard, however since it's not really a standard, various systems, including backends written in other languages, may implement it subtly differently. In the future I wonder if we could disable the nested interpretation by default, and enable it with an option passed to nock().

Since we currently support it, I'm fine continuing to support it with this new implementation, and appreciate the cleanup. Just something to consider for the future.

nockbot · 2019-07-28T17:19:41Z

🎉 This PR is included in version 11.0.0-beta.30 🎉

The release is available on:

Your semantic-release bot 📦🚀

The `qs` dependency was removed, however, recorder.js retained usage of it. Replacing it with `querystring` from the std lib is sufficient. Introduced: nock#1632 Fixes: nock#1651

The `qs` dependency was removed, however, recorder.js retained usage of it. Replacing it with `querystring` from the std lib is sufficient. Introduced: #1632 Fixes: #1651

nockbot · 2019-08-12T23:55:22Z

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

* refactor(match_body): options obj as arg instead of context Also don't bother calling `transformRequestBodyFunction` or `matchBody` if there is no request body on the Interceptor. * perf: prefer regexp.test over match when applicable * refactor: overhaul body and query matching Drop `deep-equal` and `qs` dependency. Replace `qs.parse` with `querystring.parse` from the std lib. The main difference between the two being that `qs` "unpacks" nested objects when the search keys use JSON path notation eg "foo[bar][0]". This has no affect on URL encoded form bodies during matching because we manually convert the notation to nested objects for comparison now, which means users can now provide expectations in either form. Similarly, when matching search params using a provided object, there is no net affect. The only breaking change is when a user provides a callback function to `.query()`. Replace `deep-equal` with a custom utility function in common.js. Doing so not only dropped a dependency, it also achieved a more generic utility for our usec ase with less code. Interceptor matching had some refactoring: - Query matching was moved to its own method in an effort to isolate concerns. - Matching the request method was moved to the top of the match method. - Debugging statements were updated as a baby step towards having more insight into why a match was missed. Fixes #507 Fixes #1552 BREAKING CHANGE: The only argument passed to the Interceptor.query callback now receives the output from querystring.parse instead of qs.parse. This means that instead of nested objects the argument will be a flat object.

The `qs` dependency was removed, however, recorder.js retained usage of it. Replacing it with `querystring` from the std lib is sufficient. Introduced: #1632 Fixes: #1651

* refactor(match_body): options obj as arg instead of context Also don't bother calling `transformRequestBodyFunction` or `matchBody` if there is no request body on the Interceptor. * perf: prefer regexp.test over match when applicable * refactor: overhaul body and query matching Drop `deep-equal` and `qs` dependency. Replace `qs.parse` with `querystring.parse` from the std lib. The main difference between the two being that `qs` "unpacks" nested objects when the search keys use JSON path notation eg "foo[bar][0]". This has no affect on URL encoded form bodies during matching because we manually convert the notation to nested objects for comparison now, which means users can now provide expectations in either form. Similarly, when matching search params using a provided object, there is no net affect. The only breaking change is when a user provides a callback function to `.query()`. Replace `deep-equal` with a custom utility function in common.js. Doing so not only dropped a dependency, it also achieved a more generic utility for our usec ase with less code. Interceptor matching had some refactoring: - Query matching was moved to its own method in an effort to isolate concerns. - Matching the request method was moved to the top of the match method. - Debugging statements were updated as a baby step towards having more insight into why a match was missed. Fixes #507 Fixes #1552 BREAKING CHANGE: The only argument passed to the Interceptor.query callback now receives the output from querystring.parse instead of qs.parse. This means that instead of nested objects the argument will be a flat object.

The `qs` dependency was removed, however, recorder.js retained usage of it. Replacing it with `querystring` from the std lib is sufficient. Introduced: #1632 Fixes: #1651

* refactor(match_body): options obj as arg instead of context Also don't bother calling `transformRequestBodyFunction` or `matchBody` if there is no request body on the Interceptor. * perf: prefer regexp.test over match when applicable * refactor: overhaul body and query matching Drop `deep-equal` and `qs` dependency. Replace `qs.parse` with `querystring.parse` from the std lib. The main difference between the two being that `qs` "unpacks" nested objects when the search keys use JSON path notation eg "foo[bar][0]". This has no affect on URL encoded form bodies during matching because we manually convert the notation to nested objects for comparison now, which means users can now provide expectations in either form. Similarly, when matching search params using a provided object, there is no net affect. The only breaking change is when a user provides a callback function to `.query()`. Replace `deep-equal` with a custom utility function in common.js. Doing so not only dropped a dependency, it also achieved a more generic utility for our usec ase with less code. Interceptor matching had some refactoring: - Query matching was moved to its own method in an effort to isolate concerns. - Matching the request method was moved to the top of the match method. - Debugging statements were updated as a baby step towards having more insight into why a match was missed. Fixes #507 Fixes #1552 BREAKING CHANGE: The only argument passed to the Interceptor.query callback now receives the output from querystring.parse instead of qs.parse. This means that instead of nested objects the argument will be a flat object.

The `qs` dependency was removed, however, recorder.js retained usage of it. Replacing it with `querystring` from the std lib is sufficient. Introduced: #1632 Fixes: #1651

mastermatt added 3 commits July 18, 2019 14:30

refactor(match_body): options obj as arg instead of context

33f0860

Also don't bother calling `transformRequestBodyFunction` or `matchBody` if there is no request body on the Interceptor.

perf: prefer regexp.test over match when applicable

5783244

mastermatt added the refactor label Jul 18, 2019

mastermatt requested review from gr2m and paulmelnikow July 18, 2019 20:34

mastermatt commented Jul 18, 2019

View reviewed changes

gr2m approved these changes Jul 18, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jul 21, 2019

Weekly Digest (14 July, 2019 - 21 July, 2019) #1638

Closed

mastermatt mentioned this pull request Jul 23, 2019

refactor: default options on recorder, remove dead code #1641

Merged

paulmelnikow approved these changes Jul 27, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Jul 28, 2019

Weekly Digest (21 July, 2019 - 28 July, 2019) #1648

Closed

mastermatt merged commit 35221ce into nock:beta Jul 28, 2019

mastermatt deleted the refactor-body-query-matching branch July 28, 2019 17:14

nockbot added the released label Jul 28, 2019

This was referenced Aug 1, 2019

11.0.0-beta.30 imports qs but does not require it #1651

Closed

In 11.0.0-beta.30, when no body is provided .filteringRequestBody() no longer has any effect #1652

Closed

mastermatt mentioned this pull request Aug 1, 2019

bug(recorder): replace qs lib with native querystring #1653

Merged

weekly-digest bot mentioned this pull request Aug 4, 2019

Weekly Digest (28 July, 2019 - 4 August, 2019) #1658

Closed

weekly-digest bot mentioned this pull request Aug 11, 2019

Weekly Digest (4 August, 2019 - 11 August, 2019) #1665

Closed

nockbot mentioned this pull request Sep 4, 2019

The automated release is failing 🚨 #1704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: overhaul body and query matching #1632

refactor: overhaul body and query matching #1632

mastermatt commented Jul 18, 2019 •

edited

mastermatt Jul 18, 2019

gr2m left a comment

paulmelnikow commented Jul 19, 2019

mastermatt commented Jul 19, 2019

paulmelnikow left a comment

nockbot commented Jul 28, 2019

nockbot commented Aug 12, 2019

refactor: overhaul body and query matching #1632

refactor: overhaul body and query matching #1632

Conversation

mastermatt commented Jul 18, 2019 • edited

mastermatt Jul 18, 2019

Choose a reason for hiding this comment

gr2m left a comment

Choose a reason for hiding this comment

paulmelnikow commented Jul 19, 2019

mastermatt commented Jul 19, 2019

paulmelnikow left a comment

Choose a reason for hiding this comment

nockbot commented Jul 28, 2019

nockbot commented Aug 12, 2019

mastermatt commented Jul 18, 2019 •

edited