AngularJS is throwing "Error: key.charAt is not a function" on Firefox when $filter is iterating over some kind of strange Object

[seriesoftubes]

Summary
When $filter is used against an array of custom objects, I received this console error in 1.5:
Error: key.charAt is not a function
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20587:16
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20587:42
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20587:42
createPredicateFn/predicateFn@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20562:12
filterFilter/<@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.5.11/angular.js:20526:12

And in 1.6:
Error: key.charAt is not a function
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20564:16
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20564:42
deepCompare@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20564:42
createPredicateFn/predicateFn@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20539:12
filterFilter/<@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:20503:12
anonymous/fn@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js line 15156 > Function:2:489
regularInterceptedExpression@https://cdnjs.cloudflare.com/ajax/libs/angular.js/1.6.1/angular.js:16264:55

Steps to repro:
I saw this bug when iterating over a goog.structs.Map, and noticed that key was actually a number. Sorry, I can't reproduce this without figuring out how to pull in closure on plnkr. And it is still hard for me to believe that a for (var key in obj) loop can produce non-string keys, so I don't know how else to possibly reproduce it.

The best I can say is that this definitely was an issue on >= Angular 1.5 on Firefox (but not Chrome). I'm not sure about IE.

Possible fix:
checking for typeof key === 'string'
here: https://github.com/angular/angular.js/blob/master/src/ng/filter/filter.js#L229

[gkalpak]

If we can't rely on Object keys being strings, where is this world headed?

I assume that somehow iterating over the keys of a goog.struct.Map with for ... in returns the actual Map keys (either via iterator or getKeys), which can be of any type.
(If my assumption is correct, Closure isn't palying very nice here imo 😞).

[seriesoftubes]

I think that must be it. I'm not familiar with iterators' behavior, but my goog.structs.Map did have numeric keys in the this.keys_ array, and I guess each one of those was being returned by the iterator.

[gkalpak]

How about we change key.charAt(0) with key[0]. Would that work for you (as in "not throw errors"), @seriesoftubes?

[lgalfaso]

Or someone needs to poke johnlenz
https://github.com/google/closure-library/blob/master/closure/goog/structs/map.js#L305

[seriesoftubes]

key[0] would work for me personally, but would not work for keys that are null or undefined.

[gkalpak]

but would not work for keys that are null or undefined

True!

I am a little on the fence with this one. I don't want to introduce extra work for every filtering operation, just to support some dubious, ultra-edgy usecase, where keys are not strings 😕

@seriesoftubes, if you want to put together a PR (using key.charAt && key.charAt(0) ...), we could definitely consider it. (BTW, I don't think there is any way to test this 😛)

[seriesoftubes]

I agree-- key[0] should be perfectly fine in the vast majority of cases. I have no idea why they chose .charAt(0) in the first place. Apparently key[0] is roughly identical performance-wise to key.charAt(0) and at least on modern Chrome, the output is consistent too, even with UTF8 characters.

I wish I had more time to file a proper PR-- I opened this issue hoping that there would be an easier way for someone else to fix it.

[gkalpak]

No worries! Soemone will take care of it.

So, if anyone is up for a little PR, without a way to test this (so no tests to be added), adding a key.charAt check to this line is as easy as it can get:

-if ((key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator, anyPropertyKey, true)) {
+if (key.charAt && (key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator, anyPropertyKey, true)) {

Writing the commit message (and explaining how an Objects key can be a non-String value) will be the tough part 😛