Filter path fix #6009

IgorMinar · 2014-01-28T01:37:05Z

caitp · 2014-01-28T01:48:42Z

test/ng/filter/filterSpec.js

@@ -60,6 +60,16 @@ describe('Filter: filter', function() {
    expect(filter(items, {first:'misko', last:'hevery'})[0]).toEqual(items[0]);
  });

+
+  iit('should support predicat object with dots in the name', function() {


iit tests should not be checked in IMO

yeah. my bad :)

… paths Closes angular#6005

IgorMinar · 2014-01-28T05:21:26Z

sorry.. I pushed this as I was getting off the shuttle so it contained the iit and extra commits from previous work. it should be all sorted out now.

#sloppyMe

caitp · 2014-01-28T05:35:43Z

Not a problem =) LGTM, but is this a breaking change? Looks like it impacts people who depend on crawling through nested objects, if people were using it that way.

IgorMinar · 2014-01-31T11:16:15Z

I realized that it's a breaking change, however this behavior was not intentional, we had no tests for it and it was never documented. for these reasons I think it's fine to make it.

bendowski · 2014-02-03T10:34:43Z

Is there any alternative? Can filterFilter support nested objects instead?

caitp · 2014-02-03T14:20:56Z

@benol the filterFilter no longer uses the expression parser, but what you could do is write your own comparator function, or alternatively just put together a new filter from scratch.

amj · 2014-02-03T18:24:29Z

Can't this do both? Nested objects seem like a more frequent use case than dotted property names, and the principle of least surprise would argue for at least trying both of them.

caitp · 2014-02-03T18:27:16Z

We'll have to discuss how that would work, it would be a problem to have an API which is sometimes an expression and sometimes a string. Consistency is important here.

amj · 2014-02-03T18:32:04Z

The filterFilter already takes strings, objects, or expressions. I agree
consistency is important -- so is versimilitude :)

On Mon, Feb 3, 2014 at 10:28 AM, Caitlin Potter [email protected]:

We'll have to discuss how that would work, it would be a problem to have
an API which is sometimes an expression and sometimes a string. Consistency
is important here.

Reply to this email directly or view it on GitHubhttps://github.com//pull/6009#issuecomment-33983966
.

bendowski · 2014-02-04T09:25:16Z

Just to clarify, I meant that filterFilter could support something like this:

clients | filter:{address: {town: 'London'}}

instead of the old

clients | filter:{'address.town': 'London'}

mgol · 2014-02-09T16:51:43Z

This commit broke our filter in one place in our app. Even if it was not tested, I feel that sufficiently enough people could depend on that it should be placed in the Breaking Changes section, it took me a while of selectively applying patches between 1.2.10 & 1.2.12 to figure out what's happening. I did look into the Breaking Changes section before manually testing it, if the info was there, it would save me a lot of time.

mgol · 2014-02-09T17:05:21Z

Just to clarify, I meant that filterFilter could support something like this:
clients | filter:{address: {town: 'London'}}
instead of the old
clients | filter:{'address.town': 'London'}

I like this proposal though I imagine it would increase implementation complexity & might slow parsing down. Is that why it doesn't work that way?

jrencz · 2014-02-10T08:59:02Z

maybe some kind of config on filterProvider to decide what kind of behavior one want filters to adopt?

mgol · 2014-02-10T10:28:18Z

maybe some kind of config on filterProvider to decide what kind of behavior one want filters to adopt?

I don't think it's a good idea to add up configuration for such cases; it's practically global state and if some modules start depending on one value and others on the other one, you're effectively not able to use both.

jrencz · 2014-02-10T11:26:38Z

then @benol's suggestion is the best that could be done in this case to regain nesting capabilities.

Or maybe

clients | filter:{'address.town': 'London'}

could be treated in different way than

clients | filter:{ address.town: 'London'}

with quotes it's a name, without - nested property (or some other notation dedicated to name-with-dots case)
That way both @gmiller2007 case (#6005) could be satisfied and backward compatibility could be preserved

caitp · 2014-02-11T14:00:18Z

In looking at @kueblboe's issue reproduction, it seems that deeply nested expression objects don't actually work, only the top level object is used. This should be easy to fix, but it's probably a bit of a user pain

kueblboe · 2014-02-11T14:09:01Z

Hey @caitp . Not sure what you mean with "a bit of a user pain". Are you going to support nested expression objects again or do I have to fix my application that uses it?

caitp · 2014-02-11T14:14:51Z

@kueblboe what I mean is, the suggested { 'propertyA': { 'propertyB': searchText } } style expression does not actually work, what ends up happening is the predicate for propertyA gets built, and the resulting comparator just essentially compares Object.toString() against Object.toString(), so they match 100% of the time.

This is not intended, so yeah we need to fix that, this fix will make it into the library for Valentines Day, I'm sure =)

kueblboe · 2014-02-11T14:18:47Z

OK, I'll wait for Valentines Day then. Thanks, @caitp for your quick replies.

Due to 339a165, it became impossible to filter nested properties of an object using the filterFilter. A proposed solution to this was to enable the use of nested predicate objects. This change enables the use of these nested predicate objects. Example: ```html <div ng-repeat="it in items | filter:{ address: { country: 'Canuckistan'}}"></div> ``` Or ```js $filter('filter')(items, { address: { country: 'Canuckistan' } }); ``` Closes angular#6215 Related to angular#6009

caitp · 2014-02-12T00:09:35Z

Merged~ It is possible/likely that there are still some situations where it matches 100% of the time but shouldn't, so please file a bug if you run into a related problem.

Due to 339a165, it became impossible to filter nested properties of an object using the filterFilter. A proposed solution to this was to enable the use of nested predicate objects. This change enables the use of these nested predicate objects. Example: ```html <div ng-repeat="it in items | filter:{ address: { country: 'Canuckistan'}}"></div> ``` Or ```js $filter('filter')(items, { address: { country: 'Canuckistan' } }); ``` Closes angular#6215 Related to angular#6009

shql · 2014-02-20T16:06:07Z

For us it was a breaking change as well. But how would you identify an undocumented feature as broken... thanks anyway! #6215 fixes this in the same second.

ghost assigned caitp Jan 28, 2014

caitp reviewed Jan 28, 2014
View reviewed changes

fix(filterFilter): don't interpret dots in predicate object fields as…

5ef8d1d

… paths Closes angular#6005

IgorMinar closed this in 339a165 Jan 31, 2014

kueblboe mentioned this pull request Feb 11, 2014

Regression in using dot notation in filter object #6213

Closed

caitp mentioned this pull request Feb 11, 2014

feat(filterFilter): support deeply nested predicate objects #6215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter path fix #6009

Filter path fix #6009

IgorMinar commented Jan 28, 2014

caitp Jan 28, 2014

IgorMinar Jan 28, 2014

IgorMinar commented Jan 28, 2014

caitp commented Jan 28, 2014

IgorMinar commented Jan 31, 2014

bendowski commented Feb 3, 2014

caitp commented Feb 3, 2014

amj commented Feb 3, 2014

caitp commented Feb 3, 2014

amj commented Feb 3, 2014

bendowski commented Feb 4, 2014

mgol commented Feb 9, 2014

mgol commented Feb 9, 2014

jrencz commented Feb 10, 2014

mgol commented Feb 10, 2014

jrencz commented Feb 10, 2014

caitp commented Feb 11, 2014

kueblboe commented Feb 11, 2014

caitp commented Feb 11, 2014

kueblboe commented Feb 11, 2014

caitp commented Feb 12, 2014

shql commented Feb 20, 2014

Filter path fix #6009

Filter path fix #6009

Conversation

IgorMinar commented Jan 28, 2014

caitp Jan 28, 2014

Choose a reason for hiding this comment

IgorMinar Jan 28, 2014

Choose a reason for hiding this comment

IgorMinar commented Jan 28, 2014

caitp commented Jan 28, 2014

IgorMinar commented Jan 31, 2014

bendowski commented Feb 3, 2014

caitp commented Feb 3, 2014

amj commented Feb 3, 2014

caitp commented Feb 3, 2014

amj commented Feb 3, 2014

bendowski commented Feb 4, 2014

mgol commented Feb 9, 2014

mgol commented Feb 9, 2014

jrencz commented Feb 10, 2014

mgol commented Feb 10, 2014

jrencz commented Feb 10, 2014

caitp commented Feb 11, 2014

kueblboe commented Feb 11, 2014

caitp commented Feb 11, 2014

kueblboe commented Feb 11, 2014

caitp commented Feb 12, 2014

shql commented Feb 20, 2014