fix(filterFilter): filter deep object by string

[shuhei]

Before #9757 and 1.3.6, filterFilter with string expression used to check object's properties recursively. Now it checks only object's top level properties, which is a breaking change to existing applications.

Here is a test that this pull request makes passable.

  it('should filter deep object by string', function() {
    var items = [{person: {name: 'Annet', email: 'annet@example.com'}},
                 {person: {name: 'Billy', email: 'me@billy.com'}},
                 {person: {name: 'Joan', email: {home: 'me@joan.com', work: 'joan@example.net'}}}];
    expect(filter(items, 'me@joan').length).toBe(1);
    expect(filter(items, 'joan@example').length).toBe(1);
  });

[googlebot]

CLAs look good, thanks!

[gkalpak]

This was deliberate and was implemented as part of the algorithm described in #9757 (comment) (near the bottom of the comment):

Special cases:

• For any k === '$' (at a specific level), the corresponding (sub)item must have at least one property (regardless its name) so that expr[k] matches item[xyz].
• A boolean/number/string expr at the top level only is equivalent to {$: expr}. (This does not hold for deeper nested sub-expressions.)
• ...

Key phrase is "the corresponding (sub)item", meaning that $: xyz matches properties on the same level.

There is even a test that ensures $ behaves like this.

So, basically, this works as expected (imo), since:

• filter:'xyz' translates to filter:{$: 'xyz'} and
• having {$: 'xyz'} match an item that has a property, superstring of xyz, nested X levels deep, will be confusing and unexpected.

---

That said, I was planning (but somehow forgot) to propose a new special property name (e.g. $$) that does exactly that: Match any property at the same level or deeper.

WDYT, @caitp, @petebacondarwin ?

[caitp]

@gkalpak I think we should fix this, I didn't notice that change at all. No breaking changes please. I'm not sure this fix is what I'd go with since it is damned unreadable though

[gkalpak]

@caitp: Let's don't have another long debate on what is/isn't a breaking change and focus on what we want implemented instead 😄

_{Previously, deep expression objects weren't really supported, so the semantics of `$` were more straight-forward.}

Basically, I see three possible options (feel free to suggest others):

1. Make $ much any property on the same level or deeper.
   Currently it only matches properties on the same level.
2. Introduce a special case, where expression -> 'some string' matches any property at any level.
   Currently it is equivalent to {$: 'some string'}, which only matches properties on the same level.
3. Introduce a special key-name (e.g. $$) that behaves similar to $ expect for matching on deeper levels as well.
   Currently there is no possibility to much on same level or deeper.

_{(If we choose the 3rd option, we can either **(a)** make `expression -> 'some string'` be transformed to `{$$: 'some string'}` or **(b)** leave the current behaviour and enable users explicitly use `{$$: 'some string'}` if they want "deep matching".)}

FWIW, I hate option (1) and favor option (3); probably (3a) to avoid "breaking broken expected behaviour".

[caitp]

well it is a breaking change, we can't do that in 1.3 =\

[gkalpak]

@caitp: Sure, I give up; it's a breaking change.

If you could point out which of the options above is the indicated solution (or suggest something else), I would be very glad to submit a fix 😕

[caitp]

How about, $ applies to the current and all subsequent levels of the object, as it would have before, while $$ if ever implemented, applies to just the one

[petebacondarwin]

Maybe I am missing something here but I would suggest that this is actually a regression that needs to be fixed.

If you simply pass in a string to filterFilter then it should behave as before - i.e. match any object value deeply. I don't see why we should be forced by the implementation to change this behaviour. If necessary query expressions that are purely strings could be given a specialized code path if we can't achieve what we want by translating the string to some object containing a $ key.

[caitp]

@petebacondarwin you aren't missing anything, that's exactly the issue --- whether it's called a regression or a breaking change. We need to fix this for monday :<

[gkalpak]

I will submit a fix later today, implementing @caitp's suggestion of having $ apply to the current and all subsequent levels of the object.

[petebacondarwin]

@gkalpak - great!! Thanks

[shuhei]

I've updated the code to have $ apply to the current and all subsequent levels of the object. How about this?

[gkalpak]

@shuhei: Your current implementation makes all string properties in the expression object to match any property (as if their key were $). E.g. the following test would fail:

var items = [{level1: {level2: 'test'}}];
var expr = {level1: 'test'};
expect(filter(items, expr).length).toBe(0);

[shuhei]

@gkalpak Thanks for pointing it out. But should the test pass in v1.3.x? It fails in v1.3.5 at least.

IMHO, everything including $ should check the same level and deeper. $ also checks any properties at the same level.

[gkalpak]

@shuhei: Well, imho no property should match deeper levels, but that's just me.

The test did indeed fail on v1.3.5 (imo because filterFilter was kind of broken back then; but it's debatable 😄)

@caitp, wdyt ? Should the above test still fail ? (It did fail up until v1.3.5 😞)

[caitp]

Yeah, I'm pretty sure that was a bug in the previous implementation, it is not documented as behaving like that. We will need to include notice of this in the fixed version though.

[shuhei]

@gkalpak @caltp OK, I will be happy as long as filterFilter works recursively by string.

[shuhei]

So, should I keep this PR open until #10408 is merged? Or should I close this now in favor of #10408?

[caitp]

keep it open, we'll close on merge

[gkalpak]

@caitp: What do you mean by "include notice of this" ? In the docs ? In the commit message ? (Not a breaking change I presume.)

[caitp]

@gkalpak yes we'll want it in the changelog, so a breaking change (I think we can let this one go since it was technically a bug, but if people depend on it they'll need to know) --- but also a snippet in the api docs would be good to clarify it