Fix(filter): Filter objects with circular references

[brettshollenberger]

Overview of the issue - When filtering confronts a circular reference, it causes an infinite recursion & subsequent stack overflow.

Motivation for or Use Case - As modeling libraries evolve for Angular (I've spent quite a bit of time on one myself https://github.com/FacultyCreative/ngActiveResource), circular references on related objects are likely to become more frequent. For instance, you have a post that has many comments:

  post.comments[0] = {body: 'great comment', post: post}

Currently, the search method calls itself on each attribute on each object in the collection being filtered. Since post references this particular comment, and the comment references back, the search method will never end.

Angular Version(s) - 1.2.13 (current) and before

Browsers and Operating System - This is a problem for all browsers

Reproduce the error - Test included, plus see this Fiddle: http://jsfiddle.net/S2UY6/9/

Related issues - I do not see the same issue reported, though users seem to have particular difficulty with circular references in the DI system and angular.extend.

Suggest a Fix - Fix included

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#6319)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[caitp]

This test is probably not right, better to just use not.toThrow(), to avoid issues with different VM implementations.

It should also verify that the output looks as expected

[caitp]

So, I'm all for avoiding descending into circular references, but it seems that this change will only avoid throwing or infinite looping when the circular reference is exactly one level deeper than the original object, which is a fairly narrow selection of circular references.

This could be avoided by pushing to a stack, and testing if a property references any of the values within the stack.

Also, this check should probably only be performed when the value is not a primitive or null.

[caitp]

This will skip other properties in the object

[brettshollenberger]

Good points. I'll go at this again :)

[brettshollenberger]

Thanks, @caitp! I think I covered the right bases now; I'm only performing the circular ref check on things capable of having properties (objects, arrays, functions), and keeping these in the filterFilter function scope so they exist during the multiple search calls, and can be garbage collected after the filter finishes :)

[caitp]

So we can simplify this block a bit:

for (var objKey in obj) {
  if (objKey.charAt(0) !== '$') {
    var value = obj[objKey];

    // Will avoid adding `null` to the collection (typeof null === "object")
    if (isObject(value) || isFunction(value)) {
      // If it's a circular reference, just proceed to the next property
      if (evaluatedObjects.indexOf(value) >= 0) continue;
      evaluatedObjects.push(value);
    }
    // No need to duplicate this call in two branches
    if (search(value, text)) {
      return true;
    }
  }
}

[caitp]

So I was talking about this with Igor, and a concern he's having is that because indexOf is O(N), this could majorly impede performance when people use large collections. With Angular 2.0, we can get this down to O(1) using WeakMap, but we're going to have to wait to check this in until we start the 1.3 branch, so that we can easily revert it if it causes too many problems

[brettshollenberger]

@caitp that makes sense :) I'll push up the simplified block and let me know where this should go from there. Appreciate your consideration on this!

[IgorMinar]

free up references with evaluatedObjects.length = 0

[IgorMinar]

nevermind, that won't do anything. ignore :)

[IgorMinar]

alright, so I still have an issue with the log(n) of the algorithm used.

instead of indexOf, I propose that we use hashKey that comes from apis.js in angular source tree. We pass all objects into the function, that will stamp them with uuid if needed, once the objects are stamped, we can read the uuid from obj.$$hashKey and store the value in an object hash. afterwards we can do cheap hasOwnProperty(uuid) check to see if we've seen this object before during the traversal needed during filtering.

[caitp]

@IgorMinar I recall we opted not to support this (I think it was mentioned in one of the other issues regarding circular references), am I mis-remembering?

[caitp]

@brettshollenberger I was discussing this with Igor today, and I think the conclusion was that the right thing to do is write a custom filter for this.

The main issue is that it's not clear how common of a use-case it is to have data structures being iterated over which have circular references to begin with, and because it's not clear how common it is, it would be bad to penalize likely more common cases with guards against circular references.

So, I would suggest putting together a solution and publishing it on bower or npm as a module, so that other developers could benefit from your hard work.

Sorry it's taken so long to get here, and the PR is appreciated regardless. I'm sure people might appreciate having this as a third party module instead.