Regression in filterFilter 1.3.6 changes when using a custom comparator function

[dcartwright]

When I upgraded from an earlier version of 1.3.x (1.3.0) to the latest version (1.3.15), several uses of filterFilter were broken. I traced the regression back to the changes that were implemented in 1.3.6 (specifically I think it was f7cf846). I believe this same issue probably effects 1.4.x as well, although I have not tested it.

In my use case, I have a filter that looks like this:
ng-repeat="... | filter:{Slot: slot}:modelEquals"

What's happening here is that Slot, the property, is a reference to a model object on the array elements. This then gets compared to slot, the iterator variable, from a wrapping ng-repeat. The comparison function is modelEquals, which does a comparison based on the object prototype and ID property. The specifics of the comparison function are not relevant because my problem is that the comparison function is never called!

Here are two jsfiddles that reproduce the problem with as little complexity as I could manage and still reproduce the problem:

1.3.6 - This shows the problem. You can see that my custom comparison function is never called.
http://jsfiddle.net/thcjthcy/17/

1.3.5 - This is the exact same functional code (just different explanatory text), but referencing 1.3.5. Notice that my comparison function is called as expected.
http://jsfiddle.net/thcjthcy/16/

I debugged into the 1.3.6+ code a bit, and I think the problem comes when the deepCompare function recurses whenever the expected value is an object (https://github.com/angular/angular.js/blob/v1.3.15/src/ng/filter/filter.js#L216). I believe I understand why it does this (wants to support comparisons of nested properties), however in my case the expected value is a complex object, and the comparison seems to get lost in the woods, so to speak, and my comparison function is never called. In the 1.3.5 code, since I don't believe it recurses, my comparison function is called with the top-level expected and actual object values.

I believe this is a bug because it doesn't match the documentation, or at least my understanding of it. Specifically:

https://github.com/angular/angular.js/blob/v1.3.15/src/ng/filter/filter.js#L32
Note that a named property will match properties on the same level only, while the special $ property will match properties on the same level or deeper
I am using a named property, and yet the code is recursing to a deeper level instead of calling my comparison function on the top-level property.

For the time being, my work-around is to stay on 1.3.5. If this is just how filterFilter is supposed to work now, and there won't be an "opposite of $" kind of marker that lets me short-circuit the deep comparison, I would probably work around this by writing a filter function which takes the property name and expected value and does the key indexing into the array item (actual) manually. This isn't a huge amount of work, but I wanted to see if the built-in filterFilter was supposed to be able to support this use case before I ditched it.

[Narretz]

@gkalpak as the filter master in residence, would you mind taking a look? ;)

[gkalpak]

On it ;)

[gkalpak]

@dcartwright, this is a result of filterFilter properly supporting deep expression objects and items since 1.3.6. As you correctly recognized, your custom objects are traversed deeply by filterFilter, which can lead to unexpected results with custom objects.

I don't think you need to create a custom filter, since you can either deeply match right from the expression object (which was not possible pre v1.3.6) or using a custom function as an expression.
If you post your exact usecase (preferrably on a runnable fiddle), I will be glad to take a look and recommend a solution.
_{(Even a complex example is OK, as long as it demonstrates the issue.)}

[petebacondarwin]

@Narretz I popped this in the Purgatory milestone which is for issues that are awaiting response from the OP. The Ice Box milestone is for features that we haven't completely decided against but don't intend to implement in the near future.

[dcartwright]

@gkalpak I think maybe we just aren't seeing eye-to-eye on the correct behavior of the filter expression. To me, it's a bit weird/unexpected for a named property to recurse to sub-objects. I don't need or want deep matching, especially if it breaks shallow matching, and it seems like the documentation (at least the part I quoted above) suggests that deep matching is only employed with the special "$" key. I guess I'm a bit confused about what deep matching even means - it looks through the entire object graph of my iterated objects looking for any property named "foo"? Does it only look at foo.foo.foo..., or does it look for a property named "foo" anywhere in the entire object graph? Isn't the latter hopelessly inefficient in the general case where we just need basic shallow matching?

[gkalpak]

Since 1.3.6, filterFilter supports deep expression objects. This means that an expression object can be an arbitrarily deep object (i.e. with objects as properties) and this allows it to match against "deep" array items (i.e. items that are objects whose properties might be objects etc), without the need of a custom comparator.

For example, given an array of persons, where each person looks like this:

{
  firstname: <firstname>,
  lastname: <lastname>,
  address: {
    city: <city>,
    street: <street>,
    number: <number>
  },
  ...
}

you can search for persons in city 'X' by doing:

var persons = [...];
var expr = {address: {city: 'X'}};
filterFilter(persons, expr);

which basically means: Get me all items that have an address property which is an object which has a city property whose value matches 'X'.
In this context, "matches" means "the comparator considers them equal" (which for the default comparator means substring matching).

One could say that the expression object defines a blueprint for the structure of the items and the comparator (either default, strict or custom) is used to compare the values on the "leaf-nodes".

In this sense, matching properties on the same level means:

If I give you an expression like {foo: {bar: xyz}} I am looking for objects that have a foo property, which is an object with a bar property whose value matches xyz (the comparator decides what "matches" means).

Matching properties on the same level or deeper means:

If I give you an expression like {foo: {bar: xyz}} I am looking for objects that have a foo property, which is an object with a bar property whose value matches xyz OR which is an object which has a property that matches xyz (or is an object that has a property that matches etc). (Again the comparator decides what "matches" means).
_{bar is used for illustration purposes. Only $ has this deep-matching behaviour.}

E.g.:

var items = [{foo: {bar: 'baz'}}, {foo: {bar: {qux: 'baz'}}}];
filterFilter(items, {foo: {bar: 'baz'}});   // matches only items[0]
filterFilter(items, {foo: {$: 'baz'}});     // matches both items[0] and items[1]

[dcartwright]

I see - so my problem is that I'm not writing a one-off nested object in the template, I'm actually comparing to a complex domain object as the expected value, and because it's an object it is triggering the recursion code - that makes more sense. It's unfortunate but probably unavoidable that we can't differentiate the two use cases via the value part of the expression itself - since one could imagine a bound "template" object in the style you are discussing.

Maybe something on the key name that indicates whether deep matching should be used? I think arguably that would be better for triggering this new deep matching behavior for backwards compatibility, but an option to turn it off would also work for me. For example, what about a convention in the same style as '!' is used for "not", but prefixing '$' meaning "deep match the value of this key". So you could do filter:{$foo : {nested: 'value'}}, but without the '$' it would keep backwards compatibility? At this point it's possible the backwards compatibility argument cuts both ways, so I could also see a "don't do deep matching" prefix.

If that's not going to be a supported use case for filterFilter, I can always write a custom filter function that takes the relevant comparison object and property name as parameters. I should clarify on your previous offer of assistance that my "modelEquals" implementation ultimately defers to a function on the model objects to determine equality (this is a requirement for me), so I don't think there's any way that a naive deep match is going to work for me - I really have to have a custom comparator.

[gkalpak]

Changing the current default behaviour of diving into objects would be an unnecessary breaking change, which I don't like.
But I would definitely like having a way to indicate an object should not be traversed (a new feature essentially).

I am not sure what the best approach would be, though.

A few (not so good) ideas came into mind, but the only reasonable one seems to be:

Special character in the property name (of the expression object).
E.g. {foo: {'bar|': someObj}} would mean: Find items with a foo property which is an object with a bar property and let the comparator decide if the value of bar matches someObj.

Cons: No way to actually have a special-charactered property (e.g. bar| in this example). I doubt it will affect anyone in practice, though.

I would like to hear other ideas.

---

@dcartwright, regarding your usecase (and the current filterFilter implementation), I can't really suggest a better alternative if you don't provide more info (such as an example of the slot object's structure and your custom comparator).

[mansiganatra]

Our team faced a similar issue while upgrading from 1.2.15 to 1.3.15. We wrote our custom filter function to do the desired operation.
@dcartwright I agree, the comparison is getting lost in the woods.
Also whether is it possible to modify the behavior of the function in a way, to search first level properties first, keeping track of properties which are objects with properties at each level of recursion. If a match is found in the first level itself, no need to traverse the next level of objects. It would essentially mean to have trade-off between BFS and DFS.

[gkalpak]

Given that any change would increase the complexity of the filterFilter, this doesn't seem to affect many people and there are work-arounds available (e.g. custom filter/comparator), I am going to move this to the icebox.