Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(filter): throw error if not used with an array #10352

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/content/error/filter/notarray.ngdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
@ngdoc error
@name filter:notarray
@fullName Not an array
@description

This error occurs when {@link ng.filter filter} is not used with an array.
Filter must be used with an array so a subset of items can be returned.
The array can be initialized asynchronously so null or undefined won't throw this error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I think it's good to show causes of the error in code snippets, as well as suggested fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I'll try to add it when I find time this week. Thanks!

8 changes: 7 additions & 1 deletion src/ng/filter/filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,13 @@
*/
function filterFilter() {
return function(array, expression, comparator) {
if (!isArray(array)) return array;
if (!isArray(array)) {
if (array == null) {
return array;
} else {
throw minErr('filter')('notarray', 'Expected array but received: {0}', array);
}
}

var predicateFn;
var matchAgainstAnyProp;
Expand Down
31 changes: 30 additions & 1 deletion test/ng/filter/filterSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ describe('Filter: filter', function() {
expect(filter(items, expr, true).length).toBe(1);
expect(filter(items, expr, true)[0]).toBe(items[0]);

// Inherited function proprties
// Inherited function properties
function Item(text) {
this.text = text;
}
Expand Down Expand Up @@ -323,6 +323,35 @@ describe('Filter: filter', function() {
});


it('should throw an error when is not used with an array', function() {
var item = {'not': 'array'};
expect(function() { filter(item, {}); }).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like some extra test cases here --- for instance, one with Object.create(null), one with a non-function toString and valueOf method --- These will work just fine currently, but if the implementation of minErr changes they might need some refactoring later on.

Otherwise it looks pretty good, I'd land this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @caitp! I've updated with three new test cases, but I'm not sure if they are what you meant. So please check it and let me know. Cheers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sort of --- they can be in the same it() block as the first test you wrote --- testing that it throws a specific $minErr error, rather than the TypeError that will be thrown if we change the implementation of minerr

toThrowMinErr('filter', 'notarray', 'Expected array but received: {"not":"array"}');

item = Object.create(null);
expect(function() { filter(item, {}); }).
toThrowMinErr('filter', 'notarray', 'Expected array but received: {}');

item = {
toString: null,
valueOf: null
};
expect(function() { filter(item, {}); }).
toThrowMinErr('filter', 'notarray', 'Expected array but received: {"toString":null,"valueOf":null}');
});


it('should return undefined when the array is undefined', function() {
expect(filter(undefined, {})).toBeUndefined();
});


it('should return null when the value of the array is null', function() {
var item = null;
expect(filter(item, {})).toBe(null);
});


describe('should support comparator', function() {

it('not consider `object === "[object Object]"` in non-strict comparison', function() {
Expand Down
2 changes: 1 addition & 1 deletion test/ngScenario/dslSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ describe("angular.scenario.dsl", function() {
});

it('should match bindings by substring match', function() {
compile('<pre ng-bind="foo.bar | filter"></pre>', 'binding value');
compile('<pre ng-bind="foo.bar | lowercase"></pre>', 'binding value');
$root.dsl.binding('foo . bar');
expect($root.futureResult).toEqual('binding value');
});
Expand Down