Fix handling circular references correctly in objects (closes #8663)

[lucasfcosta]

Summary

This PR fixes the StackOverflow caused by passing objects with circular references to toMatchObject as per described in #8663.

And many thanks to @leoselig for providing an accurate report 💖

Why this problem happens

1. The object matching happens in the toMatchObject method of expect. This method uses equals and passes two custom testers to it: iterableEquality and subsetEquality.
2. The equals method will then use the passed customTesters to check for equality between a and b. If a customTester returns something other than undefined, that result is returned.
3. The subsetEquality method would then use equals for all the properties in the object with itself as one of the customTesters. Due to this, if there were circular references the equals method would call subsetEquality with the very same object repeatedly and the stack would reach its limit.

It's important to highlight that this problem would only occur if the subset were the object which contained circular references since it's the one whose keys we iterate through.

Please notice that even though equals had its own mechanism to avoid infinite recursion on circular references that would never get called because of the customTesters which would cause that function to return early.

Implementation details

To solve this I created a nested function called subsetEqualityWithContext which takes not only a and b but also a WeakMap to keep track of checked objects. I then use subsetEqualityWithContext instead of subsetEquality as one of the customTesters for equals.

This approach works because we add any found objects to seenReferences so that the next time they are seen we can use equals without the customTester which would cause infinite recursion.

A little bit of semantics

The choice to use equals again if the object has already been seen is due to the semantics of circular references when it comes to equality. This is a very important detail. I opted for considering a circular reference as always being equal to another circular reference instead of checking the actual target of the reference.

This implementation choice makes the following objects match successfully even though ref in otherCircularObj does not point to circularObj.

const circularObj = {a: 'hello'};
circularObj.ref = circularObj;

const otherCircularObj = {a: 'hello'};
otherCircularObj.ref = otherCircularObj;

expect(otherCircularObj).toMatchObject(circularObj);

If we simply returned true or false we would be avoiding to check ref and if we used strict equality (===) ref would have to point to the exact same object in order for them to match:

const circularObj = {a: 'hello'};
circularObj.ref = circularObj;

const otherCircularObj = {a: 'hello'};
otherCircularObj.ref = circularObj;

// This would pass if we used strict equality (`===`)
expect(otherCircularObj).toMatchObject(circularObj);

In a testing context, I believe users will want the former behaviour, and therefore I opted for making all circular references semantically equal.

Test plan

I have extensively tested this code both with simple circular references and transitive circular references.

In the tests for toMatchObject, I have checked both if they are matched correctly and also the snapshots of when the matching fails. To make these tests less verbose and repetitive, I also extracted the method responsible for doing the match and the negated match (the one with .not) and reuse them throughout the tests for toMatchObject.

I also added tests for the subsetEquality function in utils to ensure that it's returning the correct boolean results itself and maintain tests granular (covering different layers of toMatchObject).

In these I cover the following cases (separately for simple references and transitive references):

• Circular references in both sides of toMatchObject
• Circular references in different objects which match
• Circular references in different objects which don't match
• Circular references which would be compared against primitives

Circular references in Arrays were already being handled correctly, but I added tests for it just to avoid any possible regressions.

[codecov-io]

Codecov Report

Merging #8687 into master will decrease coverage by 0.33%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #8687      +/-   ##
=========================================
- Coverage   63.83%   63.5%   -0.34%     
=========================================
  Files         274     274              
  Lines       11387   11396       +9     
  Branches     2769    2775       +6     
=========================================
- Hits         7269    7237      -32     
- Misses       3499    3546      +47     
+ Partials      619     613       -6

Impacted Files	Coverage Δ
packages/expect/src/utils.ts	96.79% <100%> (+0.24%)	⬆️
packages/jest-core/src/lib/create_context.ts	0% <0%> (-100%)	⬇️
packages/jest-core/src/runJest.ts	34.66% <0%> (-21.34%)	⬇️
packages/jest-core/src/runGlobalHook.ts	0% <0%> (-17.25%)	⬇️
packages/jest-core/src/TestWatcher.ts	42.85% <0%> (-14.29%)	⬇️
packages/jest-core/src/SearchSource.ts	58.42% <0%> (-5.62%)	⬇️
packages/jest-core/src/watch.ts	73.15% <0%> (-5.38%)	⬇️
packages/jest-core/src/ReporterDispatcher.ts	84.21% <0%> (-5.27%)	⬇️
packages/jest-core/src/TestScheduler.ts	61.83% <0%> (-3.82%)	⬇️
packages/jest-jasmine2/src/reporter.ts	58.13% <0%> (-1.39%)	⬇️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd76829...1f7bf0a. Read the comment docs.

[thymikee]

Nice

[thymikee]

How about moving the initialization of the weak map here?

[lucasfcosta]

Good idea. Just pushed this change.

Btw the Azure builds seem to be very flaky on Windows machines due to their network connection 😢

[pedrottimark]

Lucas thank you for this contribution, and especially such a clear explanation.

[pedrottimark]

@lucasfcosta I have added some line breaks so you can verify if the long-line snapshot is the intended criterion:

exports[
`toMatchObject() circular references simple circular references {pass: false}
expect({"a": "hello", "ref": [Circular]})
.toMatchObject({"a": "world", "ref": [Circular]}) 1`
] =
`"Maximum call stack size exceeded"`;

I think it is from test case: [circularObjA1, circularObjB]

[lucasfcosta]

Excellent catch! I have just fixed that. Please see the Appendix below.

Thank you @pedrottimark ✨

[lucasfcosta]

Appendix

I have just added to this PR a commit which fixes this issue raised by @pedrottimark.

This was a bit more involved than I thought it would be, so there's a long explanation below 😅

Why this happened

This mistake — a very well spotted one — was caused by problems in the function which calculates the object subsets to generate the actual diff when tests fail. This is also why the Stack Overflow did not happen when toMatchObject succeeded. ThetoMatchObject method was working problems but there were problems in the function which generated the diff for when it failed.

When the tests fail, they will use printDiffOrStringify which takes as argument a call to getObjectSubset (the function that was problematic).

This problem was very similar to the previous one and was caused specifically by the getObjectSubset method. This method iterates through all the keys in the subset and calls itself with the content of each. Since there were circular references, this causes an infinite recursion and thus a stack-overflow.

How I solved it

This solution was very similar to the previous one, but there was a major caveat which makes a minor detail in the implementation extremely important.

In getObjectSubset I use a WeakMap which is instantiated the first time we call the function (if we don't pass one) and use it to keep track of all the object objects we have seen and what's the trimmed object which corresponds to them in the new object we are creating. We then iterate over all the properties in object and copy the ones that also exist in the subset to trimmed.

If we happen to go through an object that we have already seen it means it is a circular reference and therefore we must use it's correspondent trimmed object, otherwise the reference will be to the object itself and not a circular reference.

If we used the reference that object has to itself the following would happen:

That nested [[Circular]] would appear because it's a circular reference to the object inside ref, not to the whole object itself.

Keeping track of trimmed as I did and using it fixes this problem.

Tests

I have added tests to cover all the weird cases I could think of in getObjectSubset and updated the necessary snapshots.

---

Other details

• Iterating through object keys instead of subset keys makes the code more readable IMO and also avoids unnecessarily iterating through all the properties in the subset since the broadest result we can have is object anyway.
• Checking properties in this direction (object -> subset) also allows us to simplify the check to see if a property is a circular reference since has will return false for whatever primitives passed to it because WeakMap can only hold objects as keys.

@pedrottimark Thanks for the excellent review and for catching this problem. Without this, all the above wouldn't have been found. Brilliant work.

[lucasfcosta]

Hello everyone,

Last week was very busy, but I just thought I'd ping some of you here so that we could perhaps get this merged. If there's anything I can do please let me know and I'll do my best to answer ASAP.

Thank you very much for the brilliant work 😊

CC @pedrottimark @thymikee @SimenB

[pedrottimark]

Lucas, thank you for hard work, positive interaction, and your patience with a delayed final review.

[lucasfcosta]

Thanks, @pedrottimark, I highly appreciate your work and the whole team's. Thanks for the brilliant and careful review. Looking forward to contributing more often ❤️

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.