Skip to content

Commit

Permalink
refactor: overhaul body and query matching (#1632)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@mastermatt
mastermatt committed Jul 28, 2019
1 parent 88e85ac commit 35221ce
Show file tree
Hide file tree
Showing 8 changed files with 178 additions and 156 deletions.
52 changes: 49 additions & 3 deletions lib/common.js
View file
Expand Up @@ -424,9 +424,7 @@ function matchStringOrRegexp(target, pattern) {
const str =
(!_.isUndefined(target) && target.toString && target.toString()) || ''

return pattern instanceof RegExp
? str.match(pattern)
: str === String(pattern)
return pattern instanceof RegExp ? pattern.test(str) : str === String(pattern)
}

/**
Expand Down Expand Up @@ -546,6 +544,53 @@ function urlToOptions(url) {
return options
}

/**
* Determines if request data matches the expected schema.
*
* Used for comparing decoded search parameters, request body JSON objects,
* and URL decoded request form bodies.
*
* Performs a general recursive strict comparision with two caveats:
* - The expected data can use regexp to compare values
* - JSON path notation and nested objects are considered equal
*/
const dataEqual = (expected, actual) =>
deepEqual(expand(expected), expand(actual))

/**
* Converts flat objects whose keys use JSON path notation to nested objects.
*
* The input object is not mutated.
*
* @example
* { 'foo[bar][0]': 'baz' } -> { foo: { bar: [ 'baz' ] } }
*/
const expand = input =>
Object.entries(input).reduce((acc, [k, v]) => _.set(acc, k, v), {})

/**
* Performs a recursive strict comparison between two values.
*
* Expected values or leaf nodes of expected object values that are RegExp use test() for comparison.
*/
const deepEqual = (expected, actual) => {
debug('deepEqual comparing', typeof expected, expected, typeof actual, actual)
if (expected instanceof RegExp) {
return expected.test(actual)
}

if (Array.isArray(expected) || _.isPlainObject(expected)) {
const expKeys = Object.keys(expected)
if (expKeys.length !== Object.keys(actual).length) {
return false
}

return expKeys.every(key => deepEqual(expected[key], actual[key]))
}

return expected === actual
}

exports.normalizeClientRequestArgs = normalizeClientRequestArgs
exports.normalizeRequestOptions = normalizeRequestOptions
exports.normalizeOrigin = normalizeOrigin
Expand All @@ -567,3 +612,4 @@ exports.percentDecode = percentDecode
exports.matchStringOrRegexp = matchStringOrRegexp
exports.formatQueryValue = formatQueryValue
exports.isStream = isStream
exports.dataEqual = dataEqual
132 changes: 53 additions & 79 deletions lib/interceptor.js
View file
@@ -1,13 +1,14 @@
'use strict'

const matchBody = require('./match_body')
const common = require('./common')
const _ = require('lodash')
const debug = require('debug')('nock.interceptor')
const stringify = require('json-stringify-safe')
const qs = require('qs')
const _ = require('lodash')
const querystring = require('querystring')
const url = require('url')

const common = require('./common')
const matchBody = require('./match_body')

let fs
try {
fs = require('fs')
Expand Down Expand Up @@ -228,11 +229,16 @@ Interceptor.prototype.match = function match(options, body) {
}

const method = (options.method || 'GET').toUpperCase()
let { path } = options
let { path = '' } = options
let matches
let matchKey
const { proto } = options

if (this.method !== method) {
debug(`Method did not match. Request ${method} Interceptor ${this.method}`)
return false
}

if (this.scope.transformPathFunction) {
path = this.scope.transformPathFunction(path)
}
Expand Down Expand Up @@ -277,6 +283,25 @@ Interceptor.prototype.match = function match(options, body) {
return false
}

// Match query strings when using query()
if (this.queries === null) {
debug('query matching skipped')
} else {
// can't rely on pathname or search being in the options, but path has a default
const [pathname, search] = path.split('?')
const matchQueries = this.matchQuery({ search })

debug(matchQueries ? 'query matching succeeded' : 'query matching failed')

if (!matchQueries) {
return false
}

// If the query string was explicitly checked then subsequent checks against
// the path using a callback or regexp only validate the pathname.
path = pathname
}

// If we have a filtered scope then we use it instead reconstructing
// the scope from the request options (proto, host and port) as these
// two won't necessarily match and we have to remove the scope that was
Expand All @@ -287,93 +312,26 @@ Interceptor.prototype.match = function match(options, body) {
matchKey = common.normalizeOrigin(proto, options.host, options.port)
}

// Match query strings when using query()
let matchQueries = true
let queryIndex = -1
let queryString
let queries

if (this.queries) {
queryIndex = path.indexOf('?')
queryString = queryIndex !== -1 ? path.slice(queryIndex + 1) : ''
queries = qs.parse(queryString)

// Only check for query string matches if this.queries is an object
if (_.isObject(this.queries)) {
if (_.isFunction(this.queries)) {
matchQueries = this.queries(queries)
} else {
// Make sure that you have an equal number of keys. We are
// looping through the passed query params and not the expected values
// if the user passes fewer query params than expected but all values
// match this will throw a false positive. Testing that the length of the
// passed query params is equal to the length of expected keys will prevent
// us from doing any value checking BEFORE we know if they have all the proper
// params
debug('this.queries: %j', this.queries)
debug('queries: %j', queries)
if (_.size(this.queries) !== _.size(queries)) {
matchQueries = false
} else {
const self = this
_.forOwn(queries, function matchOneKeyVal(val, key) {
const expVal = self.queries[key]
let isMatch
if (val === undefined || expVal === undefined) {
isMatch = false
} else if (expVal instanceof RegExp) {
isMatch = common.matchStringOrRegexp(val, expVal)
} else if (_.isArray(expVal) || _.isObject(expVal)) {
isMatch = _.isEqual(val, expVal)
} else {
isMatch = common.matchStringOrRegexp(val, expVal)
}
matchQueries = matchQueries && !!isMatch
})
}
debug('matchQueries: %j', matchQueries)
}
}

// Remove the query string from the path
if (queryIndex !== -1) {
path = path.substr(0, queryIndex)
}
}

if (typeof this.uri === 'function') {
matches =
matchQueries &&
method === this.method &&
common.matchStringOrRegexp(matchKey, this.basePath) &&
// This is a false positive, as `uri` is not bound to `this`.
// eslint-disable-next-line no-useless-call
this.uri.call(this, path)
} else {
matches =
method === this.method &&
common.matchStringOrRegexp(matchKey, this.basePath) &&
common.matchStringOrRegexp(path, this.path) &&
matchQueries
common.matchStringOrRegexp(path, this.path)
}

// special logger for query()
if (queryIndex !== -1) {
this.scope.logger(
`matching ${matchKey}${path}?${queryString} to ${
this._key
} with query(${stringify(this.queries)}): ${matches}`
)
} else {
this.scope.logger(`matching ${matchKey}${path} to ${this._key}: ${matches}`)
}
this.scope.logger(`matching ${matchKey}${path} to ${this._key}: ${matches}`)

if (matches) {
if (matches && this._requestBody !== undefined) {
if (this.scope.transformRequestBodyFunction) {
body = this.scope.transformRequestBodyFunction(body, this._requestBody)
}

matches = matchBody.call(options, this._requestBody, body)
matches = matchBody(options, this._requestBody, body)
if (!matches) {
this.scope.logger("bodies don't match: \n", this._requestBody, '\n', body)
}
Expand Down Expand Up @@ -406,11 +364,11 @@ Interceptor.prototype.matchAddress = function matchAddress(options) {
const matchKey = `${method} ${proto}://${options.host}${path}`

if (isRegex && !isRegexBasePath) {
return !!matchKey.match(comparisonKey) && !!path.match(this.path)
return !!matchKey.match(comparisonKey) && this.path.test(path)
}

if (isRegexBasePath) {
return !!matchKey.match(this.scope.basePath) && !!path.match(this.path)
return this.scope.basePath.test(matchKey) && !!path.match(this.path)
}

return comparisonKey === matchKey
Expand All @@ -420,6 +378,22 @@ Interceptor.prototype.matchHostName = function matchHostName(options) {
return options.hostname === this.scope.urlParts.hostname
}

Interceptor.prototype.matchQuery = function matchQuery(options) {
if (this.queries === true) {
return true
}

const reqQueries = querystring.parse(options.search)
debug('Interceptor queries: %j', this.queries)
debug(' Request queries: %j', reqQueries)

if (_.isFunction(this.queries)) {
return this.queries(reqQueries)
}

return common.dataEqual(this.queries, reqQueries)
}

Interceptor.prototype.filteringPath = function filteringPath(...args) {
this.scope.filteringPath(...args)
return this
Expand Down Expand Up @@ -482,7 +456,7 @@ Interceptor.prototype.query = function query(queries) {
if (queries instanceof url.URLSearchParams) {
// Normalize the data into the shape that is matched against.
// Duplicate keys are handled by combining the values into an array.
queries = qs.parse(queries.toString())
queries = querystring.parse(queries.toString())
} else if (!_.isPlainObject(queries)) {
throw Error(`Argument Error: ${queries}`)
}
Expand Down
51 changes: 11 additions & 40 deletions lib/match_body.js
View file
@@ -1,19 +1,13 @@
'use strict'

const deepEqual = require('deep-equal')
const qs = require('qs')
const _ = require('lodash')
const common = require('./common')

module.exports = function matchBody(spec, body) {
if (typeof spec === 'undefined') {
return true
}
const querystring = require('querystring')

const options = this || {}
const common = require('./common')

module.exports = function matchBody(options, spec, body) {
if (spec instanceof RegExp) {
return body.match(spec)
return spec.test(body)
}

if (Buffer.isBuffer(spec)) {
Expand All @@ -30,9 +24,8 @@ module.exports = function matchBody(spec, body) {
''
).toString()

const isMultipart = contentType.indexOf('multipart') >= 0
const isUrlencoded =
contentType.indexOf('application/x-www-form-urlencoded') >= 0
const isMultipart = contentType.includes('multipart')
const isUrlencoded = contentType.includes('application/x-www-form-urlencoded')

// try to transform body to json
let json
Expand All @@ -45,12 +38,12 @@ module.exports = function matchBody(spec, body) {
if (json !== undefined) {
body = json
} else if (isUrlencoded) {
body = qs.parse(body, { allowDots: true })
body = querystring.parse(body)
}
}

if (typeof spec === 'function') {
return spec.call(this, body)
return spec.call(options, body)
}

// strip line endings from both so that we get a match no matter what OS we are running on
Expand All @@ -63,6 +56,8 @@ module.exports = function matchBody(spec, body) {
spec = spec.replace(/\r?\n|\r/g, '')
}

// Because the nature of URL encoding, all the values in the body have been cast to strings.
// dataEqual does strict checking so we we have to cast the non-regexp values in the spec too.
if (isUrlencoded) {
spec = mapValuesDeep(spec, function(val) {
if (_.isRegExp(val)) {
Expand All @@ -72,7 +67,7 @@ module.exports = function matchBody(spec, body) {
})
}

return deepEqualExtended(spec, body)
return common.dataEqual(spec, body)
}

/**
Expand All @@ -92,27 +87,3 @@ function mapValuesDeep(obj, cb) {
}
return cb(obj)
}

function deepEqualExtended(spec, body) {
if (spec && spec.constructor === RegExp) {
return spec.test(body)
}
if (
spec &&
(spec.constructor === Object || spec.constructor === Array) &&
body
) {
const keys = Object.keys(spec)
const bodyKeys = Object.keys(body)
if (keys.length !== bodyKeys.length) {
return false
}
for (let i = 0; i < keys.length; i++) {
if (!deepEqualExtended(spec[keys[i]], body[keys[i]])) {
return false
}
}
return true
}
return deepEqual(spec, body, { strict: true })
}
8 changes: 2 additions & 6 deletions package-lock.json
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 35221ce

Please sign in to comment.