filter doesn't work when methods are added to Object.prototype

[habfast]

$filter("filter")([{"name":"foo"}], "foo") will return [] if properties/methods are added to Object.prototype because the check isn't done as it should.

This simple change fixes that.

[caitp]

can you give an example of what you mean by "doesn't work"? This patch looks fishy, and is missing tests. If I get an understanding of what the issue is, we can look at getting this fixed up and mergeable

[habfast]

Have you read my comment? I gave an example:

I expect $filter("filter")([{"name":"foo"}], "foo") to return [{"name":"foo"}].

If I write Object.prototype.noop = function noop(){}; then the same statement now returns []

I provide a screenshot from chrome's dev tools

[habfast]

This applies at least to angular 1.2.16, 1.3.0 and 1.3.2.

[caitp]

yes, I did read your comment, it's really unclear what you're talking about =)

Even the devtools screenshot isn't really helpful, because devtools behave pretty differently from a regular application.

Please provide an actual application demo where this causes problems, you can use that reproduction to help write a test case for your fix, and we can use that to make the fix better.

[habfast]

Does a plunker work for you?

It's related to this:
https://docs.angularjs.org/api/ng/filter/filter

[caitp]

I created a reproduction here http://jsbin.com/govefegoxu/1/ --- so it's valid, but I don't believe this is the right fix.

[habfast]

Ha thanks for preceding me.

I am curious to hear what's the right fix. I was told it is best practice to wrap all for-in loops with if-hasOwnProperty.

[caitp]

I am curious to hear what's the right fix. I was told it is best practice to wrap all for-in loops with if-hasOwnProperty.

This would mean that the predicates will never ever check properties higher in the prototype chain, which works in some cases, but not so much others.

For instance, if you add a string to the prototype chain, this won't happen --- we'll deal with it correctly. The fact that it's a function seems to be the weird thing.

I think the case where we want to check if it's an own property is probably here: https://github.com/angular/angular.js/blob/master/src/ng/filter/filter.js#L186

[caitp]

and even there, we should probably just not add functions to the predicates list, since we don't handle them anyways.

[habfast]

Makes a lot of sense. I still think this should be somehow fixed, as this looks resolutely like a bug to me.

Is there anything else I can do to help?

[caitp]

Absolutely! first, lets write a test case in https://github.com/angular/angular.js/blob/master/test/ng/filter/filterSpec.js, and lets experiment with different approaches to fixing this which won't limit the use of filters to own-properties.

@petebacondarwin unless you think we should limit to own properties, but this would be a breaking change.

[mary-poppins]

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

[petebacondarwin]

Let's avoid breaking changes.
Developers could always write a completely new filter that only checked "own properties" if they wanted.

[caitp]

yeah --- so lets pick a different approach to fixing this which doesn't avoid setting up predicates for not-own properties

[gkalpak]

(Sorry if I am stating the obvious, but (based on the discussion above) I am not sure if it is clear.)

The issue with setting properties on Object.prototype is that the items will have additional properties, but the fact that the expression object will have additional (inherited) properties.

This test highlights the issue:

  it('should not consider the expression\'s inherited properties', function() {
    Object.prototype.noop = noop;

    var items = [
      {text: 'hello'},
      {text: 'goodbye'},
      {text: 'kittens'},
      {text: 'puppies'}
    ];

    var expr = Object.create(null);
    expr.text = 'hell';

    expect(filter(items, expr).length).toBe(1);
    expect(filter(items, expr)[0]).toBe(items[0]);

    expect(filter(items, {text: 'hell'}).length).toBe(1);
    expect(filter(items, {text: 'hell'})[0]).toBe(items[0]);
  });

---

Needless to say that there is fantastic PR that makes the filter filter so much awesomer in so many ways (including solving this issue "for free"): #9757
_{Just saying...}

[habfast]

I think what would be best is that your PR gets integrated, and this one just closed.

[habfast]

@realityking I've read a whole lot of js ressources, and I haven't often come across anything that says it's bad. I mean, even after googling it, most people don't seem to say it's such a bad thing: http://sugarjs.com/native