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

filter Filter will convert null and undefined to strings #11432

Closed
rince1013 opened this issue Mar 25, 2015 · 10 comments
Closed

filter Filter will convert null and undefined to strings #11432

rince1013 opened this issue Mar 25, 2015 · 10 comments

Comments

@rince1013
Copy link

filterFilter will convert null and undefined to strings and use that to filter
This makes the default comparator unusable with arrays that have fields with null or undefined values.
This seems to be a regression - it works as expected in <=1.3.5

Here is plnkr
http://plnkr.co/edit/P67JlZv83ea8J5ZFw9Ce?p=preview

@Narretz
Copy link
Contributor

Narretz commented Mar 26, 2015

I don't see it, maybe I don't understand the problem.
If you have search.name = null, it will filter correctly. If you have search.name = undefined, it will not match something with name: undefined , neither <= 1.3.5 nor after.

@Narretz Narretz added this to the Purgatory milestone Mar 26, 2015
@pkozlowski-opensource
Copy link
Member

This sounds somehow similar to #11312 - different filter but the same root cause.

@rince1013
Copy link
Author

Maybe this update plunker will make it a bit clearer
http://plnkr.co/edit/f7trY8?p=preview

I have two values in the array where name is not a string - one is null and one is undefined

[{name:null, phone:'The value of name is null'},
{name:undefined, phone:'The value of name is undefined'},
{name:'Mike', phone:'555-4321'},
{name:'Adam', phone:'555-5678'},
{name:'Julie', phone:'555-8765'},
{name:'Juliette', phone:'555-5678'}]

If I search this array for names with 'u', you would expect only Julie and Juliette to return. But, also the null and undefined results are returned. The filter filter is treating null and undefined as strings, and then using the converted string value to determine if the array matches the search.

If I change the Angular version to 1.3.5, the filter filter works as expected

@rince1013
Copy link
Author

And here is the offending line
https://github.com/angular/angular.js/blob/master/src/ng/filter/filter.js#L180
in function createPredicateFn

actual = lowercase('' + actual);

This line implicitly converts null into "null" and undefined into "undefined"

@hamfastgamgee
Copy link

Agree that it's very similar to my bug, #11312. Sadly, the broken code isn't shared. :)

@pkozlowski-opensource
Copy link
Member

Yeh, this is a valid one.

@gkalpak
Copy link
Member

gkalpak commented Mar 26, 2015

I always have a hard time differentiating between bug fixes and breaking changes when it comes to filterFilter. So, if I understand correctly, this is considered a bug/regression.

The desired behaviour is:

  1. null item property | any string filter expression -> NO MATCH
  2. null item property | null filter expression -> MATCH
  3. undefined item property | any string filter expression -> NO MATCH
  4. undefined item property | undefined filter expression -> (irrelevant; no filtering anyway)

(Currently, (1) and (3) match if the filter expression is a subtring of "null"/"undefined" respectively.)

Do I miss something ?

@hamfastgamgee
Copy link

I think it's probably expected behavior that null/undefined filter
expressions will match everything as they do today. (I don't think that's
directly stated in gkalpak's post.)

@gkalpak
Copy link
Member

gkalpak commented Mar 27, 2015

They don't do that today (nor did they do it pre 1.3.6) and I don't think they (both) should.

An undefined property of the filter expression is indeed ignored, so it will effectively match anything, since there is no filtering going on (this is mentioned in my previous comment and is what happens today).

A null property of the filter expression, should imo only match against a null property. I don't think null should match anything, since it is specific value and there are cases where you want to find nulls. (I also think this has been discussed and agreed upon during/after the v1.3.6 refactoring of filterFilter, but I am not 100% sure.)
Note, that null did not match anything before the v1.3.6 refactoring either.

@hamfastgamgee
Copy link

I think I got confused and probably was conflating explicit null in a filter with the empty string. Yes, I agree that what you say here.

gkalpak added a commit to gkalpak/angular.js that referenced this issue Mar 27, 2015
Included fixes:

* Do not convert `null`/`undefined` to strings for substring matching in
  non-strict comparison mode. Prevents `null`/`undefined` from being
  matched against e.g. 'u'.
* Let `null` (as a top-level filter expression) match "deeply" (as do
  booleans, numbers and strings).
  E.g. let `filterFilter(arr, null)` match an item like `{someProp: null}`.

Closes angular#11432
gkalpak added a commit to gkalpak/angular.js that referenced this issue Mar 30, 2015
Included fixes:

* Do not convert `null`/`undefined` to strings for substring matching in
  non-strict comparison mode. Prevents `null`/`undefined` from being
  matched against e.g. 'u'.
* Let `null` (as a top-level filter expression) match "deeply" (as do
  booleans, numbers and strings).
  E.g. let `filterFilter(arr, null)` match an item like `{someProp: null}`.

Closes angular#11432
gkalpak added a commit to gkalpak/angular.js that referenced this issue Mar 30, 2015
Included fixes:

* Do not convert `null`/`undefined` to strings for substring matching in
  non-strict comparison mode. Prevents `null`/`undefined` from being
  matched against e.g. 'u'.
* Let `null` (as a top-level filter expression) match "deeply" (as do
  booleans, numbers and strings).
  E.g. let `filterFilter(arr, null)` match an item like `{someProp: null}`.

Closes angular#11432
netman92 pushed a commit to netman92/angular.js that referenced this issue Aug 8, 2015
Included fixes:

* Do not convert `null`/`undefined` to strings for substring matching in
  non-strict comparison mode. Prevents `null`/`undefined` from being
  matched against e.g. 'u'.
* Let `null` (as a top-level filter expression) match "deeply" (as do
  booleans, numbers and strings).
  E.g. let `filterFilter(arr, null)` match an item like `{someProp: null}`.

Closes angular#11432
Closes angular#11445
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants