New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filterFilter compare custom objects in 1.3.6 #10464

Closed
m7r opened this Issue Dec 15, 2014 · 21 comments

Comments

Projects
None yet
5 participants
@m7r
Contributor

m7r commented Dec 15, 2014

First off all I am a big fan of the changed behavior as I can do finally filtering complex data as I could in angular 1.0.x

Only the default comparator should not return false if actual or expected is an object.

My use case are mongodb ObjectIds with a custom toString function.
From angular 1.0 to 1.3.5 the filterFilter could compare this custom object with a string or another ObjectId.

I know I can use my one comparator but i think the new comparator is a bit to picky.
A string like '[object' is in my opinion a user error.

WDYT?

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 15, 2014

Member

@m7r: There have been a couple of regressions reported and fixes are submitted for those, but I can't really tell if your usecase falls under one of those categories (as I can't quite understand what is the issue here).

Could you give an example (i.e. input, expected output, actual output) illustrating the problem/regression or better yet a live reproduction using plunkr / jsfiddle / ... ?

Member

gkalpak commented Dec 15, 2014

@m7r: There have been a couple of regressions reported and fixes are submitted for those, but I can't really tell if your usecase falls under one of those categories (as I can't quite understand what is the issue here).

Could you give an example (i.e. input, expected output, actual output) illustrating the problem/regression or better yet a live reproduction using plunkr / jsfiddle / ... ?

@m7r

This comment has been minimized.

Show comment
Hide comment
@m7r

m7r Dec 15, 2014

Contributor

@gkalpak http://plnkr.co/edit/fTbKNaot13K6v81xDwRL

Switch the angular version to see better what I mean.

Contributor

m7r commented Dec 15, 2014

@gkalpak http://plnkr.co/edit/fTbKNaot13K6v81xDwRL

Switch the angular version to see better what I mean.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 15, 2014

Member

@m7r: I see what you mean. I personally don't think that the default comparator (which is supposed to match substrings case-insensitively) should match a string with an Object (and yes, that sounds like a good use-case for a custom comparator imo 😄), but let's see what others think.

/ping @caitp

Member

gkalpak commented Dec 15, 2014

@m7r: I see what you mean. I personally don't think that the default comparator (which is supposed to match substrings case-insensitively) should match a string with an Object (and yes, that sounds like a good use-case for a custom comparator imo 😄), but let's see what others think.

/ping @caitp

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 15, 2014

Contributor

We have to support objects in the default comparator, for the case when no predicate is supplied.

However, this only applies when the two items being compared are both objects, it does not compare a string with an object.

Contributor

caitp commented Dec 15, 2014

We have to support objects in the default comparator, for the case when no predicate is supplied.

However, this only applies when the two items being compared are both objects, it does not compare a string with an object.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 15, 2014

Member

(Not sure what you mean by "predicate", so I assume "expression"; correct me if I'm wrong.)

If no predicate is supplied, then the original array is returned (as was the case in pre-1.3.6). We don't need the comparator for that (since there's nothing to compare with) ?

When two we have to compare two objects they are compared by deepCompare and not the comparator.

So, if I understand correctly, you agree that we shouldn't convert objects to strings for comparing against a string ?

Note, that an object might have a custom toString method, as in @m7r's use-case.

Another option might be to check if the object has a `toString()` method that is not Object's `toString()` method and use it if one is found.
Member

gkalpak commented Dec 15, 2014

(Not sure what you mean by "predicate", so I assume "expression"; correct me if I'm wrong.)

If no predicate is supplied, then the original array is returned (as was the case in pre-1.3.6). We don't need the comparator for that (since there's nothing to compare with) ?

When two we have to compare two objects they are compared by deepCompare and not the comparator.

So, if I understand correctly, you agree that we shouldn't convert objects to strings for comparing against a string ?

Note, that an object might have a custom toString method, as in @m7r's use-case.

Another option might be to check if the object has a `toString()` method that is not Object's `toString()` method and use it if one is found.
@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 15, 2014

Contributor

If no predicate is supplied, then the original array is returned (as was the case in pre-1.3.6). We don't need the comparator for that (since there's nothing to compare with) ?

No, that only applies if the predicate is undefined --- and that is actually the incorrect behaviour, we need to fix that.

(Predicate, historically, was one or more comparators --- reflecting properties of the object, or custom functions --- where the result of the filter would require each predicate comparator to return true --- you've sort of mangled this a bit with your refactoring)

Contributor

caitp commented Dec 15, 2014

If no predicate is supplied, then the original array is returned (as was the case in pre-1.3.6). We don't need the comparator for that (since there's nothing to compare with) ?

No, that only applies if the predicate is undefined --- and that is actually the incorrect behaviour, we need to fix that.

(Predicate, historically, was one or more comparators --- reflecting properties of the object, or custom functions --- where the result of the filter would require each predicate comparator to return true --- you've sort of mangled this a bit with your refactoring)

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 15, 2014

Member

AFAIKT, predicate was one or more filtering functions (and there was also a comparator).
No there is the predicateFn :)

No, that only applies if the predicate is undefined

Hm...so, what do you mean by "no predicate is supplied" then ?

and that is actually the incorrect behaviour, we need to fix that.

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a rpoperty value.)

Member

gkalpak commented Dec 15, 2014

AFAIKT, predicate was one or more filtering functions (and there was also a comparator).
No there is the predicateFn :)

No, that only applies if the predicate is undefined

Hm...so, what do you mean by "no predicate is supplied" then ?

and that is actually the incorrect behaviour, we need to fix that.

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a rpoperty value.)

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 15, 2014

Contributor

AFAIKT, predicate was one or more filtering functions (and there was also a comparator).
No there is the predicateFn :)

You're mistaken --- the predicate functions were either constructed based on input, or were already functions. There is predicateFn, but there used to be multiple predicateFns, not just one. So the idea is, for a generated predicate, if the properties it is comparing are objects, it needs to handle them specially.

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a rpoperty value.)

Because we need to reverse the array if it's asked for

Hm...so, what do you mean by "no predicate is supplied" then ?

If the value is null, you'll get a very different behaviour

Contributor

caitp commented Dec 15, 2014

AFAIKT, predicate was one or more filtering functions (and there was also a comparator).
No there is the predicateFn :)

You're mistaken --- the predicate functions were either constructed based on input, or were already functions. There is predicateFn, but there used to be multiple predicateFns, not just one. So the idea is, for a generated predicate, if the properties it is comparing are objects, it needs to handle them specially.

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a rpoperty value.)

Because we need to reverse the array if it's asked for

Hm...so, what do you mean by "no predicate is supplied" then ?

If the value is null, you'll get a very different behaviour

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 15, 2014

Member

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a property value.)

Because we need to reverse the array if it's asked for

Hm...not sure why/how it can be asked to reverse the array. filterFilter should not change the order.

Hm...so, what do you mean by "no predicate is supplied" then ?

If the value is null, you'll get a very different behaviour

Could you give an example ? We are probably talking about different things, because I see the same behaviour whether the expression is null or undefined.

Member

gkalpak commented Dec 15, 2014

Why is it incorrect ? What should happen instead.
(Note: We had a PR recently to make sure undefined is ignored, both as an expression and as a property value.)

Because we need to reverse the array if it's asked for

Hm...not sure why/how it can be asked to reverse the array. filterFilter should not change the order.

Hm...so, what do you mean by "no predicate is supplied" then ?

If the value is null, you'll get a very different behaviour

Could you give an example ? We are probably talking about different things, because I see the same behaviour whether the expression is null or undefined.

@caitp

This comment has been minimized.

Show comment
Hide comment
@caitp

caitp Dec 15, 2014

Contributor

Hm...not sure why/how it can be asked to reverse the array. filterFilter should not change the order.

You're right, thinking of orderBy

Contributor

caitp commented Dec 15, 2014

Hm...not sure why/how it can be asked to reverse the array. filterFilter should not change the order.

You're right, thinking of orderBy

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 16, 2014

Member

So, @caitp, regarding the original question, I see 3 options:

  1. comparator(<Object>, <Primitive>) ---> always false
    This is the current behaviour.
  2. comparator(<Object>, <Primitive>) ---> comparator(<Object>.toString(), <Primitive>)
    Will incorrectly allow unexpected matches (e.g. the string object to match an Object).
  3. comparator(<Object>, <Primitive>) ---> (<Object>.toString === Object.prototype.toString) ? false : /* Same as option (2) */
    Possibly combining the best of two worlds (but at a tiny extra cost).

Which on should we go by ?

Member

gkalpak commented Dec 16, 2014

So, @caitp, regarding the original question, I see 3 options:

  1. comparator(<Object>, <Primitive>) ---> always false
    This is the current behaviour.
  2. comparator(<Object>, <Primitive>) ---> comparator(<Object>.toString(), <Primitive>)
    Will incorrectly allow unexpected matches (e.g. the string object to match an Object).
  3. comparator(<Object>, <Primitive>) ---> (<Object>.toString === Object.prototype.toString) ? false : /* Same as option (2) */
    Possibly combining the best of two worlds (but at a tiny extra cost).

Which on should we go by ?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 17, 2014

Member

@gkalpak - so what you are saying is that in option 3, it will return false unless the object has overridden the meaning of toString? This is kind of nice because it will also work well with Date objects.

Let's go with option 3

Member

petebacondarwin commented Dec 17, 2014

@gkalpak - so what you are saying is that in option 3, it will return false unless the object has overridden the meaning of toString? This is kind of nice because it will also work well with Date objects.

Let's go with option 3

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 17, 2014

Member

I have added the PRs plz! label to this issue as this is a fairly straightforward fix. Please make sure you do the following:

  • comment on the issue to say that you are working on this issue
  • sign the CLA
  • add unit tests that demonstrate this problem
  • implement option 3 to fix the unit tests - I would break up the current if statement that checks the type of the obj param
  • update the docs to make it clear what happens if you are trying to match a string to an object
  • submit a pull request with a well written commit message
Member

petebacondarwin commented Dec 17, 2014

I have added the PRs plz! label to this issue as this is a fairly straightforward fix. Please make sure you do the following:

  • comment on the issue to say that you are working on this issue
  • sign the CLA
  • add unit tests that demonstrate this problem
  • implement option 3 to fix the unit tests - I would break up the current if statement that checks the type of the obj param
  • update the docs to make it clear what happens if you are trying to match a string to an object
  • submit a pull request with a well written commit message
@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 17, 2014

Member

@petebacondarwin: Do you mean we should leave this for a community member's first contribution (as it is a "good first PR") or do you want me to submit a PR ?

Member

gkalpak commented Dec 17, 2014

@petebacondarwin: Do you mean we should leave this for a community member's first contribution (as it is a "good first PR") or do you want me to submit a PR ?

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 17, 2014

Member

@gkalpak - if you have time go for it. I just wanted to put it out there as an option.

Member

petebacondarwin commented Dec 17, 2014

@gkalpak - if you have time go for it. I just wanted to put it out there as an option.

@m7r

This comment has been minimized.

Show comment
Hide comment
@m7r

m7r Dec 17, 2014

Contributor

@gkalpak I can look into it later today too.

Contributor

m7r commented Dec 17, 2014

@gkalpak I can look into it later today too.

@petebacondarwin

This comment has been minimized.

Show comment
Hide comment
@petebacondarwin

petebacondarwin Dec 17, 2014

Member

@gkalpak - let's give this one to @m7r - then perhaps you could review it?
Thanks.

Member

petebacondarwin commented Dec 17, 2014

@gkalpak - let's give this one to @m7r - then perhaps you could review it?
Thanks.

@m7r

This comment has been minimized.

Show comment
Hide comment
@m7r

m7r Dec 17, 2014

Contributor

@gkalpak could you have a look at the changes please?
I am not a native speaker are the comments and docs ok?
FYI the !isFunction is necessary for objects without prototype.

Contributor

m7r commented Dec 17, 2014

@gkalpak could you have a look at the changes please?
I am not a native speaker are the comments and docs ok?
FYI the !isFunction is necessary for objects without prototype.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 17, 2014

Member

@m7r: Sure, I'll take a look tomorrow.

Member

gkalpak commented Dec 17, 2014

@m7r: Sure, I'll take a look tomorrow.

@gkalpak

This comment has been minimized.

Show comment
Hide comment
@gkalpak

gkalpak Dec 18, 2014

Member

@m7r: I left some comments. They're mostly test-related; implementation-wise it looks good.

Member

gkalpak commented Dec 18, 2014

@m7r: I left some comments. They're mostly test-related; implementation-wise it looks good.

@m7r

This comment has been minimized.

Show comment
Hide comment
@m7r

m7r Dec 18, 2014

Contributor

@gkalpak Thanks for the comments I have updated my commit accordingly.

Contributor

m7r commented Dec 18, 2014

@gkalpak Thanks for the comments I have updated my commit accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment