Unexpected behavior in current deepEqual function

[MichaelBuhler]

Concerning the deepEqual function currently here: https://github.com/nock/nock/blob/main/lib/common.js#L570-L590

Behavior

I have run into an issue with expecting fields to be undefined in request bodies. The following behavior is observed:

// Case 1
deepEqual({ a: 'a', b: undefined }, { a: 'a' })
// false

// Case 2
deepEqual({ a: 'a', b: undefined }, { a: 'a', c: undefined }) // note: b and c are different
// true, but only coincidentally because the objects have the same number of keys

These problems stem from the serialization/deserialization of JSON bodies on requests, since undefined is not serializable in JSON. So the value of expected is not modified by nock, while the value of actual is more truly JSON.parse(JSON.stringify(actual)) since the payload is first serialized by the request lib and then deserialized by nock.

There is a very slight performance gain in the current implementation which assumes that two objects with differing number of keys are not equal. Case 2 (from above) sneakily slips past this check, which gives me the result I want, but for the wrong reason, and is incongruous with Case 1. I also don't believe this strictness is the real spirit of the matching functionality of nock:

As in Case 1, if I expect a body of { a: 'a', b: undefined }, I am asserting that b should be undefined/not present in the request body, so I think that {"a":"a"} should be a valid/matched request body.

Possible solution

Modify the deepEqual function, like so:

--- a/lib/common.js
+++ b/lib/common.js
@@ -579,11 +579,10 @@ function deepEqual(expected, actual) {
     }

     const expKeys = Object.keys(expected)
-    if (expKeys.length !== Object.keys(actual).length) {
-      return false
-    }
+    const actKeys = Object.keys(actual)
+    const allKeys = Array.from(new Set(expKeys.concat(actKeys)).values())

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

   return expected === actual

This passes every existing test case, plus fixes the problems noted above.

How to reproduce the issue

See above.

Does the bug have a test case?

Not yet.

Versions

Software	Version(s)
Nock	13.0.2
Node	14.4.0
TypeScript	3.9.5

[MichaelBuhler]

These are additional behaviors I uncovered but are not fixed by the above possible solution.

deepEqual([ 'a', 'b' ], ['b', 'a'])
// false, but possibly not in the spirit of matching

deepEqual([ 'a', 'b' ], { '0': 'a', '1': 'b' })
// true, but doesn't seem intuitively correct

[mastermatt]

Thanks for this, I agree this is a bug.

The tests cases you provided are great.
I will say, we've had discussions about array order in the past when matching (I'll try to find the threads), and we do consider deepEqual([ 'a', 'b' ], ['b', 'a']) // false to be the correct behavior.

I'll play around with this over the weekend. I think I'm inclined to separate the array and object comparisons into separate flows. If for no other reason than to improve readability around the magic of getting the keys on an array.

[github-actions]

🎉 This issue has been resolved in version 13.0.3 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀