From 471018265dd185428abec1412e53c1b9abdc1032 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Thu, 23 Oct 2014 12:59:22 +0300 Subject: [PATCH 1/4] fix(filterFilter): correctly handle deep expression objects Previously, trying to use a deep expression object (i.e. an object whose properties can be objects themselves) did not work correctly. This commit refactors `filterFilter`, making it simpler and adding support for filtering collections of arbitrarily deep objects. Closes #7323 Closes #9698 --- src/ng/filter/filter.js | 155 +++++++++++++++-------------------- test/ng/filter/filterSpec.js | 68 +++++++++++++++ 2 files changed, 136 insertions(+), 87 deletions(-) diff --git a/src/ng/filter/filter.js b/src/ng/filter/filter.js index 868199cf22cf..a9b130689cac 100644 --- a/src/ng/filter/filter.js +++ b/src/ng/filter/filter.js @@ -119,104 +119,85 @@ function filterFilter() { return function(array, expression, comparator) { if (!isArray(array)) return array; - var comparatorType = typeof(comparator), - predicates = []; - - predicates.check = function(value, index) { - for (var j = 0; j < predicates.length; j++) { - if (!predicates[j](value, index)) { - return false; - } - } - return true; - }; - - if (comparatorType !== 'function') { - if (comparatorType === 'boolean' && comparator) { - comparator = function(obj, text) { - return angular.equals(obj, text); - }; - } else { - comparator = function(obj, text) { - if (obj && text && typeof obj === 'object' && typeof text === 'object') { - for (var objKey in obj) { - if (objKey.charAt(0) !== '$' && hasOwnProperty.call(obj, objKey) && - comparator(obj[objKey], text[objKey])) { - return true; - } - } - return false; - } - text = ('' + text).toLowerCase(); - return ('' + obj).toLowerCase().indexOf(text) > -1; - }; - } - } + var predicateFn; - var search = function(obj, text) { - if (typeof text === 'string' && text.charAt(0) === '!') { - return !search(obj, text.substr(1)); - } - switch (typeof obj) { - case 'boolean': - case 'number': - case 'string': - return comparator(obj, text); - case 'object': - switch (typeof text) { - case 'object': - return comparator(obj, text); - default: - for (var objKey in obj) { - if (objKey.charAt(0) !== '$' && search(obj[objKey], text)) { - return true; - } - } - break; - } - return false; - case 'array': - for (var i = 0; i < obj.length; i++) { - if (search(obj[i], text)) { - return true; - } - } - return false; - default: - return false; - } - }; switch (typeof expression) { + case 'object': + // Replace `{$: 'xyz'}` with `'xyz'` and fall through + var keys = Object.keys(expression); + if ((keys.length === 1) && (keys[0] === '$')) expression = expression.$; + // jshint -W086 case 'boolean': case 'number': case 'string': - // Set up expression object and fall through - expression = {$:expression}; - // jshint -W086 - case 'object': // jshint +W086 - for (var key in expression) { - (function(path) { - if (typeof expression[path] === 'undefined') return; - predicates.push(function(value) { - return search(path == '$' ? value : (value && value[path]), expression[path]); - }); - })(key); - } + predicateFn = createPredicateFn(expression, comparator); break; case 'function': - predicates.push(expression); + predicateFn = expression; break; default: return array; } - var filtered = []; - for (var j = 0; j < array.length; j++) { - var value = array[j]; - if (predicates.check(value, j)) { - filtered.push(value); - } - } - return filtered; + + return array.filter(predicateFn); }; } + +// Helper functions for `filterFilter` +function createPredicateFn(expression, comparator) { + var predicateFn; + + if (comparator === true) { + comparator = equals; + } else if (!isFunction(comparator)) { + comparator = function(actual, expected) { + actual = ('' + actual).toLowerCase(); + expected = ('' + expected).toLowerCase(); + return actual.indexOf(expected) !== -1; + }; + } + + predicateFn = function(item) { + return deepCompare(item, expression, comparator); + }; + + return predicateFn; +} + +function deepCompare(actual, expected, comparator) { + var actualType = typeof actual; + var expectedType = typeof expected; + + if (expectedType === 'function') { + return expected(actual); + } else if ((expectedType === 'string') && (expected.charAt(0) === '!')) { + return !deepCompare(actual, expected.substring(1), comparator); + } else if (actualType === 'array') { + return actual.some(function(item) { + return deepCompare(item, expected, comparator); + }); + } + + switch (actualType) { + case 'object': + if (expectedType === 'object') { + return Object.keys(expected).every(function(key) { + var actualVal = (key === '$') ? actual : actual[key]; + var expectedVal = expected[key]; + return deepCompare(actualVal, expectedVal, comparator); + }); + } else { + return Object.keys(actual).some(function(key) { + return (key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator); + }); + } + break; + default: + if (expectedType === 'object') { + return false; + } + + return comparator(actual, expected); + } +} diff --git a/test/ng/filter/filterSpec.js b/test/ng/filter/filterSpec.js index 0508a696fc21..dfb2ccf569e9 100644 --- a/test/ng/filter/filterSpec.js +++ b/test/ng/filter/filterSpec.js @@ -98,6 +98,19 @@ describe('Filter: filter', function() { }); + it('should support deep expression objects with multiple properties', function() { + var items = [{person: {name: 'Annet', email: 'annet@example.com'}}, + {person: {name: 'Billy', email: 'me@billy.com'}}, + {person: {name: 'Joan', email: 'joan@example.net'}}, + {person: {name: 'John', email: 'john@example.com'}}, + {person: {name: 'Rita', email: 'rita@example.com'}}]; + var expr = {person: {name: 'Jo', email: '!example.com'}}; + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)).toEqual([items[2]]); + }); + + it('should match any properties for given "$" property', function() { var items = [{first: 'tom', last: 'hevery'}, {first: 'adam', last: 'hevery', alias: 'tom', done: false}, @@ -110,6 +123,19 @@ describe('Filter: filter', function() { }); + it('should match any properties in the nested object for given deep "$" property', function() { + var items = [{person: {name: 'Annet', email: 'annet@example.com'}}, + {person: {name: 'Billy', email: 'me@billy.com'}}, + {person: {name: 'Joan', email: 'joan@example.net'}}, + {person: {name: 'John', email: 'john@example.com'}}, + {person: {name: 'Rita', email: 'rita@example.com'}}]; + var expr = {person: {$: 'net'}}; + + expect(filter(items, expr).length).toBe(2); + expect(filter(items, expr)).toEqual([items[0], items[2]]); + }); + + it('should support boolean properties', function() { var items = [{name: 'tom', current: true}, {name: 'demi', current: false}, @@ -129,6 +155,7 @@ describe('Filter: filter', function() { expect(filter(items, '!isk')[0]).toEqual(items[1]); }); + describe('should support comparator', function() { it('as equality when true', function() { @@ -177,5 +204,46 @@ describe('Filter: filter', function() { expr = 10; expect(filter(items, expr, comparator)).toEqual([items[2], items[3]]); }); + + + it('and use it correctly with deep expression objects', function() { + var items = [ + {id: 0, details: {email: 'admin@example.com', role: 'admin'}}, + {id: 1, details: {email: 'user1@example.com', role: 'user'}}, + {id: 2, details: {email: 'user2@example.com', role: 'user'}} + ]; + var expr, comp; + + expr = {details: {email: 'user@example.com', role: 'adm'}}; + expect(filter(items, expr)).toEqual([]); + + expr = {details: {email: 'admin@example.com', role: 'adm'}}; + expect(filter(items, expr)).toEqual([items[0]]); + + expr = {details: {email: 'admin@example.com', role: 'adm'}}; + expect(filter(items, expr, true)).toEqual([]); + + expr = {details: {email: 'admin@example.com', role: 'admin'}}; + expect(filter(items, expr, true)).toEqual([items[0]]); + + expr = {details: {email: 'user', role: 'us'}}; + expect(filter(items, expr)).toEqual([items[1], items[2]]); + + expr = {id: 0, details: {email: 'user', role: 'us'}}; + expect(filter(items, expr)).toEqual([]); + + expr = {id: 1, details: {email: 'user', role: 'us'}}; + expect(filter(items, expr)).toEqual([items[1]]); + + comp = function(actual, expected) { + return isString(actual) && isString(expected) && (actual.indexOf(expected) === 0); + }; + + expr = {details: {email: 'admin@example.com', role: 'admn'}}; + expect(filter(items, expr, comp)).toEqual([]); + + expr = {details: {email: 'admin@example.com', role: 'adm'}}; + expect(filter(items, expr, comp)).toEqual([items[0]]); + }); }); }); From d9b710a984f8b65f16cead20c918e38a03640715 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 11 Nov 2014 13:29:19 +0200 Subject: [PATCH 2/4] test(filter): test expression object with inherited properties Related to #9984 --- src/ng/filter/filter.js | 2 +- test/ng/filter/filterSpec.js | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/ng/filter/filter.js b/src/ng/filter/filter.js index a9b130689cac..ca010cd28d65 100644 --- a/src/ng/filter/filter.js +++ b/src/ng/filter/filter.js @@ -125,7 +125,7 @@ function filterFilter() { case 'object': // Replace `{$: 'xyz'}` with `'xyz'` and fall through var keys = Object.keys(expression); - if ((keys.length === 1) && (keys[0] === '$')) expression = expression.$; + if ((keys.length === 1) && (keys[0] === '$')) expression = expression.$; // jshint -W086 case 'boolean': case 'number': diff --git a/test/ng/filter/filterSpec.js b/test/ng/filter/filterSpec.js index dfb2ccf569e9..c6079cf253ed 100644 --- a/test/ng/filter/filterSpec.js +++ b/test/ng/filter/filterSpec.js @@ -156,6 +156,26 @@ describe('Filter: filter', function() { }); + it('should not consider the expression\'s inherited properties', function() { + Object.prototype.noop = noop; + + var items = [ + {text: 'hello'}, + {text: 'goodbye'}, + {text: 'kittens'}, + {text: 'puppies'} + ]; + + expect(filter(items, {text: 'hell'}).length).toBe(1); + expect(filter(items, {text: 'hell'})[0]).toBe(items[0]); + + expect(filter(items, 'hell').length).toBe(1); + expect(filter(items, 'hell')[0]).toBe(items[0]); + + delete(Object.prototype.noop); + }); + + describe('should support comparator', function() { it('as equality when true', function() { From 999ab013b4ecc14f5cbad94bb86e68fffd4f0c44 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Tue, 18 Nov 2014 17:26:15 +0200 Subject: [PATCH 3/4] fix(filterFilter): ignore function properties and account for inherited properties Closes #9984 --- src/ng/filter/filter.js | 55 +++++++------ test/ng/filter/filterSpec.js | 150 +++++++++++++++++++++++++++++++++-- 2 files changed, 174 insertions(+), 31 deletions(-) diff --git a/src/ng/filter/filter.js b/src/ng/filter/filter.js index ca010cd28d65..f6a108759655 100644 --- a/src/ng/filter/filter.js +++ b/src/ng/filter/filter.js @@ -122,20 +122,15 @@ function filterFilter() { var predicateFn; switch (typeof expression) { - case 'object': - // Replace `{$: 'xyz'}` with `'xyz'` and fall through - var keys = Object.keys(expression); - if ((keys.length === 1) && (keys[0] === '$')) expression = expression.$; - // jshint -W086 + case 'function': + predicateFn = expression; + break; case 'boolean': case 'number': + case 'object': case 'string': - // jshint +W086 predicateFn = createPredicateFn(expression, comparator); break; - case 'function': - predicateFn = expression; - break; default: return array; } @@ -152,8 +147,8 @@ function createPredicateFn(expression, comparator) { comparator = equals; } else if (!isFunction(comparator)) { comparator = function(actual, expected) { - actual = ('' + actual).toLowerCase(); - expected = ('' + expected).toLowerCase(); + actual = lowercase('' + actual); + expected = lowercase('' + expected); return actual.indexOf(expected) !== -1; }; } @@ -165,15 +160,15 @@ function createPredicateFn(expression, comparator) { return predicateFn; } -function deepCompare(actual, expected, comparator) { +function deepCompare(actual, expected, comparator, keyWasDollar) { var actualType = typeof actual; var expectedType = typeof expected; - if (expectedType === 'function') { - return expected(actual); - } else if ((expectedType === 'string') && (expected.charAt(0) === '!')) { + if ((expectedType === 'string') && (expected.charAt(0) === '!')) { return !deepCompare(actual, expected.substring(1), comparator); } else if (actualType === 'array') { + // In case `actual` is an array, consider it a match + // if any of it's items matches `expected` return actual.some(function(item) { return deepCompare(item, expected, comparator); }); @@ -181,16 +176,28 @@ function deepCompare(actual, expected, comparator) { switch (actualType) { case 'object': - if (expectedType === 'object') { - return Object.keys(expected).every(function(key) { - var actualVal = (key === '$') ? actual : actual[key]; - var expectedVal = expected[key]; - return deepCompare(actualVal, expectedVal, comparator); - }); + var key; + if (keyWasDollar || (expectedType !== 'object')) { + for (key in actual) { + if ((key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator)) { + return true; + } + } + return false; } else { - return Object.keys(actual).some(function(key) { - return (key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator); - }); + for (key in expected) { + var expectedVal = expected[key]; + if (isFunction(expectedVal)) { + continue; + } + + var keyIsDollar = key === '$'; + var actualVal = keyIsDollar ? actual : actual[key]; + if (!deepCompare(actualVal, expectedVal, comparator, keyIsDollar)) { + return false; + } + } + return true; } break; default: diff --git a/test/ng/filter/filterSpec.js b/test/ng/filter/filterSpec.js index c6079cf253ed..12ea5519db23 100644 --- a/test/ng/filter/filterSpec.js +++ b/test/ng/filter/filterSpec.js @@ -136,6 +136,19 @@ describe('Filter: filter', function() { }); + it('should respect the nesting level of "$"', function() { + var items = [{supervisor: 'me', person: {name: 'Annet', email: 'annet@example.com'}}, + {supervisor: 'me', person: {name: 'Billy', email: 'me@billy.com'}}, + {supervisor: 'me', person: {name: 'Joan', email: 'joan@example.net'}}, + {supervisor: 'me', person: {name: 'John', email: 'john@example.com'}}, + {supervisor: 'me', person: {name: 'Rita', email: 'rita@example.com'}}]; + var expr = {$: {$: 'me'}}; + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)).toEqual([items[1]]); + }); + + it('should support boolean properties', function() { var items = [{name: 'tom', current: true}, {name: 'demi', current: false}, @@ -156,23 +169,146 @@ describe('Filter: filter', function() { }); - it('should not consider the expression\'s inherited properties', function() { - Object.prototype.noop = noop; + it('should ignore function properties in items', function() { + // Own function properties + var items = [ + {text: 'hello', func: noop}, + {text: 'goodbye'}, + {text: 'kittens'}, + {text: 'puppies'} + ]; + var expr = {text: 'hello'}; + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)[0]).toBe(items[0]); + expect(filter(items, expr, true).length).toBe(1); + expect(filter(items, expr, true)[0]).toBe(items[0]); + + // Inherited function proprties + function Item(text) { + this.text = text; + } + Item.prototype.func = noop; + + items = [ + new Item('hello'), + new Item('goodbye'), + new Item('kittens'), + new Item('puppies') + ]; + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)[0]).toBe(items[0]); + expect(filter(items, expr, true).length).toBe(1); + expect(filter(items, expr, true)[0]).toBe(items[0]); + }); + + + it('should ignore function properties in expression', function() { + // Own function properties var items = [ {text: 'hello'}, {text: 'goodbye'}, {text: 'kittens'}, {text: 'puppies'} ]; + var expr = {text: 'hello', func: noop}; + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)[0]).toBe(items[0]); + expect(filter(items, expr, true).length).toBe(1); + expect(filter(items, expr, true)[0]).toBe(items[0]); + + // Inherited function proprties + function Expr(text) { + this.text = text; + } + Expr.prototype.func = noop; + + expr = new Expr('hello'); + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)[0]).toBe(items[0]); + expect(filter(items, expr, true).length).toBe(1); + expect(filter(items, expr, true)[0]).toBe(items[0]); + }); + + + it('should consider inherited properties in items', function() { + function Item(text) { + this.text = text; + } + Item.prototype.doubleL = 'maybe'; + + var items = [ + new Item('hello'), + new Item('goodbye'), + new Item('kittens'), + new Item('puppies') + ]; + var expr = {text: 'hello', doubleL: 'perhaps'}; + + expect(filter(items, expr).length).toBe(0); + expect(filter(items, expr, true).length).toBe(0); + + expr = {text: 'hello', doubleL: 'maybe'}; + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)[0]).toBe(items[0]); + expect(filter(items, expr, true).length).toBe(1); + expect(filter(items, expr, true)[0]).toBe(items[0]); + }); + + + it('should consider inherited properties in expression', function() { + function Expr(text) { + this.text = text; + } + Expr.prototype.doubleL = true; + + var items = [ + {text: 'hello', doubleL: true}, + {text: 'goodbye'}, + {text: 'kittens'}, + {text: 'puppies'} + ]; + var expr = new Expr('e'); + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)[0]).toBe(items[0]); + + expr = new Expr('hello'); + + expect(filter(items, expr, true).length).toBe(1); + expect(filter(items, expr)[0]).toBe(items[0]); + }); + + + it('should not be affected by `Object.prototype` when using a string expression', function() { + Object.prototype.someProp = 'oo'; + + var items = [ + createMap(), + createMap(), + createMap(), + createMap() + ]; + items[0].someProp = 'hello'; + items[1].someProp = 'goodbye'; + items[2].someProp = 'kittens'; + items[3].someProp = 'puppies'; + + // Affected by `Object.prototype` + expect(filter(items, {}).length).toBe(1); + expect(filter(items, {})[0]).toBe(items[1]); - expect(filter(items, {text: 'hell'}).length).toBe(1); - expect(filter(items, {text: 'hell'})[0]).toBe(items[0]); + expect(filter(items, {$: 'll'}).length).toBe(0); - expect(filter(items, 'hell').length).toBe(1); - expect(filter(items, 'hell')[0]).toBe(items[0]); + // Not affected by `Object.prototype` + expect(filter(items, 'll').length).toBe(1); + expect(filter(items, 'll')[0]).toBe(items[0]); - delete(Object.prototype.noop); + delete Object.prototype.someProp; }); From 5d40a898997de16680dbd89cce50b996a82e3e45 Mon Sep 17 00:00:00 2001 From: Georgios Kalpakas Date: Mon, 24 Nov 2014 16:32:06 +0200 Subject: [PATCH 4/4] fix(filterFilter): don't match primitive sub-expressions against any prop Basically, implement the logic detailed in the 2nd point of https://github.com/angular/angular.js/pull/9757#issuecomment-63544399 --- src/ng/filter/filter.js | 37 ++++++++++++++++++++++-------------- test/ng/filter/filterSpec.js | 20 ++++++++++++++++++- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/src/ng/filter/filter.js b/src/ng/filter/filter.js index f6a108759655..a739a1ca21c7 100644 --- a/src/ng/filter/filter.js +++ b/src/ng/filter/filter.js @@ -120,6 +120,7 @@ function filterFilter() { if (!isArray(array)) return array; var predicateFn; + var matchAgainstAnyProp; switch (typeof expression) { case 'function': @@ -127,9 +128,12 @@ function filterFilter() { break; case 'boolean': case 'number': - case 'object': case 'string': - predicateFn = createPredicateFn(expression, comparator); + matchAgainstAnyProp = true; + //jshint -W086 + case 'object': + //jshint +W086 + predicateFn = createPredicateFn(expression, comparator, matchAgainstAnyProp); break; default: return array; @@ -140,13 +144,18 @@ function filterFilter() { } // Helper functions for `filterFilter` -function createPredicateFn(expression, comparator) { +function createPredicateFn(expression, comparator, matchAgainstAnyProp) { var predicateFn; if (comparator === true) { comparator = equals; } else if (!isFunction(comparator)) { comparator = function(actual, expected) { + if (isObject(actual) || isObject(expected)) { + // Prevent an object to be considered equal to a string like `'[object'` + return false; + } + actual = lowercase('' + actual); expected = lowercase('' + expected); return actual.indexOf(expected) !== -1; @@ -154,37 +163,37 @@ function createPredicateFn(expression, comparator) { } predicateFn = function(item) { - return deepCompare(item, expression, comparator); + return deepCompare(item, expression, comparator, matchAgainstAnyProp); }; return predicateFn; } -function deepCompare(actual, expected, comparator, keyWasDollar) { +function deepCompare(actual, expected, comparator, matchAgainstAnyProp) { var actualType = typeof actual; var expectedType = typeof expected; if ((expectedType === 'string') && (expected.charAt(0) === '!')) { - return !deepCompare(actual, expected.substring(1), comparator); + return !deepCompare(actual, expected.substring(1), comparator, matchAgainstAnyProp); } else if (actualType === 'array') { // In case `actual` is an array, consider it a match - // if any of it's items matches `expected` + // if ANY of it's items matches `expected` return actual.some(function(item) { - return deepCompare(item, expected, comparator); + return deepCompare(item, expected, comparator, matchAgainstAnyProp); }); } switch (actualType) { case 'object': var key; - if (keyWasDollar || (expectedType !== 'object')) { + if (matchAgainstAnyProp) { for (key in actual) { if ((key.charAt(0) !== '$') && deepCompare(actual[key], expected, comparator)) { return true; } } return false; - } else { + } else if (expectedType === 'object') { for (key in expected) { var expectedVal = expected[key]; if (isFunction(expectedVal)) { @@ -198,13 +207,13 @@ function deepCompare(actual, expected, comparator, keyWasDollar) { } } return true; + } else { + return comparator(actual, expected); } break; + case 'function': + return false; default: - if (expectedType === 'object') { - return false; - } - return comparator(actual, expected); } } diff --git a/test/ng/filter/filterSpec.js b/test/ng/filter/filterSpec.js index 12ea5519db23..7e14f5f567f4 100644 --- a/test/ng/filter/filterSpec.js +++ b/test/ng/filter/filterSpec.js @@ -136,6 +136,17 @@ describe('Filter: filter', function() { }); + it('should respect the depth level of a "$" property', function() { + var items = [{person: {name: 'Annet', email: 'annet@example.com'}}, + {person: {name: 'Billy', email: 'me@billy.com'}}, + {person: {name: 'Joan', email: {home: 'me@joan.com', work: 'joan@example.net'}}}]; + var expr = {person: {$: 'net'}}; + + expect(filter(items, expr).length).toBe(1); + expect(filter(items, expr)).toEqual([items[0]]); + }); + + it('should respect the nesting level of "$"', function() { var items = [{supervisor: 'me', person: {name: 'Annet', email: 'annet@example.com'}}, {supervisor: 'me', person: {name: 'Billy', email: 'me@billy.com'}}, @@ -314,6 +325,13 @@ describe('Filter: filter', function() { describe('should support comparator', function() { + it('not consider `object === "[object Object]"` in non-strict comparison', function() { + var items = [{test: {}}]; + var expr = '[object'; + expect(filter(items, expr).length).toBe(0); + }); + + it('as equality when true', function() { var items = ['misko', 'adam', 'adamson']; var expr = 'adam'; @@ -395,7 +413,7 @@ describe('Filter: filter', function() { return isString(actual) && isString(expected) && (actual.indexOf(expected) === 0); }; - expr = {details: {email: 'admin@example.com', role: 'admn'}}; + expr = {details: {email: 'admin@example.com', role: 'min'}}; expect(filter(items, expr, comp)).toEqual([]); expr = {details: {email: 'admin@example.com', role: 'adm'}};