fix(filterFilter): Fix filtering using an object expression when the filter value is undefined #10424

caseyhoward · 2014-12-11T18:32:13Z

googlebot · 2014-12-11T18:32:14Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

caitp · 2014-12-11T19:21:47Z

src/ng/filter/filter.js

@@ -151,6 +151,10 @@ function createPredicateFn(expression, comparator, matchAgainstAnyProp) {
    comparator = equals;
  } else if (!isFunction(comparator)) {
    comparator = function(actual, expected) {
+      if (angular.isUndefined(expected)) {


no need for the angular.

caseyhoward · 2014-12-11T22:33:06Z

I agree with the changes you suggested and created a new pull request #10427 (not sure if that was the correct thing to do).

I also added a tests for having objects at a deeper level.

googlebot · 2014-12-11T22:33:08Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

caitp · 2014-12-11T22:36:38Z

I agree with the changes you suggested and created a new pull request #10427 (not sure if that was the correct thing to do).

In the future, please just push the commit to the same branch, it will update the pull request.

caitp · 2014-12-11T22:39:02Z

In fact, you can still do that now and close the newer PR, it will be easier

caseyhoward · 2014-12-11T22:49:04Z

Ok, I'll do that. I just wasn't sure if you wanted it in multiple commits or not.

caitp · 2014-12-11T22:51:16Z

It's a single-commit if you squash it together and force push, which we do when merging anyway

caseyhoward · 2014-12-11T23:06:13Z

Ok, thanks. I was wondering how you guys merged things. I updated it.

caitp · 2014-12-11T23:08:14Z

Looks good to me --- you need to sign the CLA and then we can land this. I hope that's not a problem

caseyhoward · 2014-12-11T23:12:34Z

@googlebot I did author the commit

caitp · 2014-12-11T23:18:19Z

@caseyhoward it is confused because you submitted a pull request with commits pushed to a different user/org. But I see that you have signed the CLA, so it is okay.

gkalpak · 2014-12-12T11:09:24Z

test/ng/filter/filterSpec.js

+
+  it('should not filter when property is undefined', function() {
+    var items = [{name: 'a'}, {name: 'abc'}, {deep: {name: 'abc'}}];
+    expect(filter(items, {deep: {name: undefined}})).toEqual([{deep: {name: 'abc'}}]);


I think those two test-cases could be on the same it block. (wdyt @caitp ?)

It would be good to have one more test-case for matching an non-primitive property. E.g. something like:

var items = [{deep: {name: {first: 'John', last: 'Doe'}}}]; expect(filter(items, {deep: {name: undefined}})).toEqual([items[0]]);

(It could as well be incorporated into the above test-case.)

gkalpak · 2014-12-12T11:15:42Z

@caitp: There are basically two options, so we need to decide which one we'd like to go with:

Let the comparator decide if an undefined property matches against an item's property and have the default comparator decide that undefined matched anything.
Ignore propeties of the expression object that have a value of undefined.

If we decide to go with option (1), then this PR is fine.
If we decide to go with option (2), then we need a different implementation.

gkalpak · 2014-12-12T11:54:28Z

Regarding my previous comment: The old implementation actually implemented option (2) (ignoring expression properties with a type of undefined).

caitp · 2014-12-12T13:28:50Z

I think version 2 is a breaking change, because the version which uses equals() would be affected by that (unless I'm misreading you)

gkalpak · 2014-12-12T14:10:05Z

Considering that option 2 was the one implemented by the pre-1.3.6 version of filterFilter, I don't see how it would be a breakign change.

It would probably be the reasonable thing to do, but not a breaking change.

caitp · 2014-12-12T14:26:19Z

I don't see how it was implemented pre-1.3.6 --- the comparator functions differ between objectEquality mode and custom predicate modes, if the change is implemented before the comparator is called, then it affects objectEquality mode too --- which is incorrect. It is a breaking change to do it that way

gkalpak · 2014-12-12T14:39:43Z

I don't see how it was implemented pre-1.3.6

pre-1.3.6 expression properties of type undefined were ignored (totally ignored, as if they didn't existed). This is unrelated to the comparator (default, sctrict or custom). I.e. {prop: undefined} was treated as {}.

if the change is implemented before the comparator is called, then it affects objectEquality mode too

It does. It also affects custom comparators (e.g. I might want a comparator that matches undefined with null or whatever). This is incorrect (imo), but it is how it was implemented pre-1.3.6.

which is incorrect. It is a breaking change to do it that way

Unfortunately, we have to choose only one of: "Being correct (implementation-wise)" and "not being a breaking change".

caitp · 2014-12-12T14:44:35Z

Unfortunately, for minor releases, we have to choose "not being a breaking change" --- we can't keep doing these "break peoples apps in a patch release" things :\

gkalpak · 2014-12-12T14:53:31Z

So, this PR is not good for merging. @caseyhoward: since you worked on this one, would you like to submit another PR that implements option (2) ? If not, I can submit it sometime tomorrow.

Just to make sure it's clear: We need an implementation that treats undefined properties of the expression object as if they were not existing at all.

caitp · 2014-12-12T14:56:07Z

angular.js/src/ng/filter/filter.js

Line 200 in 9fa73cb

if (typeof expression[path] === 'undefined') return;

I guess this is what we took out

gkalpak · 2014-12-12T15:10:14Z

This is the line that "did it" on pre-1.3.6. This is not what we took out. We re-implemented the logic, because (as you can see), it only made predicates for the 1st level of expression's properties (=== no proper handling of deep objects).

gkalpak · 2014-12-12T15:21:08Z

@caseyhoward: If you try to tackle this (without giving it much thought), this is what I think should need changing:

Line 120:

if (!isArray(array)) return array;   --->   if (!isArray(array) || !isDefined(expression)) return array;

Line 199:

if (isFunction(expectedVal)) {   --->   if (isUndefined(expectedVal) || isFunction(expectedVal)) {

…filter value is undefined Fixes angular#10419

…filter value is undefined Fixes #10419 Closes #10424

…filter value is undefined Fixes angular#10419 Closes angular#10424

zuzusik · 2015-03-20T17:45:15Z

src/ng/filter/filter.js

@@ -196,7 +196,7 @@ function deepCompare(actual, expected, comparator, matchAgainstAnyProp) {
      } else if (expectedType === 'object') {
        for (key in expected) {
          var expectedVal = expected[key];
-          if (isFunction(expectedVal)) {
+          if (isFunction(expectedVal) || isUndefined(expectedVal)) {


This broke things in our project. It changed the behaviour of $filter. I don't think it is OK to change the behaviour in minor releases.

@zuzusik I was under the impression that c62fa6b was only merged to 1.4.x beta so a release where we've got breaking changes. Do you see it on any of the 1.3.x branches as well?

@pkozlowski-opensource It was successfully landed to 1.3.15, look at the changelog

@pkozlowski-opensource here it is: 63b9956

@zuzusik - This change was actually fixing a regression that occurred between 1.3.5 and 1.3.6 (see #10419). I am sorry that it took so long for us to fix this regression that you started using the filter in its regressed state.

…filter value is undefined Fixes angular#10419 Closes angular#10424

googlebot added the cla: no label Dec 11, 2014

caitp reviewed Dec 11, 2014
View reviewed changes

caseyhoward force-pushed the fix-10419 branch from b95d6c9 to e93024f Compare December 11, 2014 23:04

gkalpak reviewed Dec 12, 2014
View reviewed changes

caseyhoward force-pushed the fix-10419 branch from e93024f to 5718e12 Compare December 12, 2014 18:20

fix(filterFilter): Fix filtering using an object expression when the …

df5f775

…filter value is undefined Fixes angular#10419

caseyhoward force-pushed the fix-10419 branch from 5718e12 to df5f775 Compare December 12, 2014 18:26

btford removed this from the 1.3.8 milestone Dec 19, 2014

petebacondarwin modified the milestones: 1.4.0-beta.0, 1.4.0-beta.1 / 1.3.10 Jan 15, 2015

pkozlowski-opensource modified the milestones: 1.4.0-beta.1 / 1.3.10, 1.4.0-beta.2 / 1.3.11 Jan 20, 2015

caitp modified the milestones: 1.4.0-beta.2 / 1.3.11, 1.4.0-beta.3 / 1.3.12 Jan 27, 2015

petebacondarwin modified the milestones: 1.4.0-beta.3 / 1.3.12, 1.4.0-beta.4 / 1.3.13, 1.4.0-beta.5 / 1.3.14 Feb 3, 2015

petebacondarwin modified the milestones: 1.4.0-beta.5 / 1.3.14, 1.4.0-beta.6 / 1.3.15 Feb 24, 2015

petebacondarwin pushed a commit that referenced this pull request Mar 2, 2015

fix(filterFilter): Fix filtering using an object expression when the …

63b9956

…filter value is undefined Fixes #10419 Closes #10424

petebacondarwin closed this in c62fa6b Mar 2, 2015

hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015

fix(filterFilter): Fix filtering using an object expression when the …

71e19d6

…filter value is undefined Fixes angular#10419 Closes angular#10424

zuzusik reviewed Mar 20, 2015
View reviewed changes

netman92 pushed a commit to netman92/angular.js that referenced this pull request Aug 8, 2015

fix(filterFilter): Fix filtering using an object expression when the …

c93f5f6

…filter value is undefined Fixes angular#10419 Closes angular#10424

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(filterFilter): Fix filtering using an object expression when the filter value is undefined #10424

fix(filterFilter): Fix filtering using an object expression when the filter value is undefined #10424

caseyhoward commented Dec 11, 2014

googlebot commented Dec 11, 2014

caitp Dec 11, 2014

caseyhoward commented Dec 11, 2014

googlebot commented Dec 11, 2014

caitp commented Dec 11, 2014

caitp commented Dec 11, 2014

caseyhoward commented Dec 11, 2014

caitp commented Dec 11, 2014

caseyhoward commented Dec 11, 2014

caitp commented Dec 11, 2014

caseyhoward commented Dec 11, 2014

caitp commented Dec 11, 2014

gkalpak Dec 12, 2014

gkalpak Dec 12, 2014

gkalpak commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

gkalpak commented Dec 12, 2014

zuzusik Mar 20, 2015

pkozlowski-opensource Mar 20, 2015

zuzusik Mar 20, 2015

zuzusik Mar 20, 2015

petebacondarwin Mar 21, 2015

fix(filterFilter): Fix filtering using an object expression when the filter value is undefined #10424

fix(filterFilter): Fix filtering using an object expression when the filter value is undefined #10424

Conversation

caseyhoward commented Dec 11, 2014

googlebot commented Dec 11, 2014

Choose a reason for hiding this comment

caseyhoward commented Dec 11, 2014

googlebot commented Dec 11, 2014

caitp commented Dec 11, 2014

caitp commented Dec 11, 2014

caseyhoward commented Dec 11, 2014

caitp commented Dec 11, 2014

caseyhoward commented Dec 11, 2014

caitp commented Dec 11, 2014

caseyhoward commented Dec 11, 2014

caitp commented Dec 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

caitp commented Dec 12, 2014

gkalpak commented Dec 12, 2014

gkalpak commented Dec 12, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment