Skip to content
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

Queries with null don't work as expected #704

Closed
bogacg opened this issue Nov 30, 2016 · 13 comments · Fixed by #772
Closed

Queries with null don't work as expected #704

bogacg opened this issue Nov 30, 2016 · 13 comments · Fixed by #772

Comments

@bogacg
Copy link

bogacg commented Nov 30, 2016

Version info

Angular: 2.2.3

Firebase: 3.6.1

AngularFire: 2.0.0-beta.6

How to reproduce these conditions

Stackoverflow: AngularFire2: query if child exists

Firebase Google Group: AngularFire2 query with null doesn't work

Steps to set up and reproduce Sample data and security rules

See Stackoverflow question above.

Expected behavior

Queries with given ordeByChild: "property" and endAt : null is expected to filter out lists where those poperties are null/don't exist.

Actual behavior

Whole list is returned with no filtering.

As @cartant pointed out, check could be done here just against undefined maybe and not to null.

@antti-pyykkonen
Copy link

Can confirm.

Currently using null just ignores that part of the query, so for example using equalTo: null will just return the whole list.

Is there a way we could allow null value checking and also resetting certain query properties?

@katowulf
Copy link
Contributor

Thanks for the report. Does look like this could be fixed. Please include a minimal repro of the issue or a failing test case we can use as a marker for verifying and fixing.

All issues have to follow the issue template provided, including a minimal repro or failing test case. This is critical to keep things streamlined and manageable.

@bogacg
Copy link
Author

bogacg commented Nov 30, 2016

I'm very displeased such a good project managed in such bad manner lately. The error case is very simple to reproduce. I can't believe people managing the project don't have sample projects at hand to test stuff out.
You guys are like just trying to push people out from this report section on purpose.

@katowulf I'm not planing to spare more time on this and creating a repro. So please close this (if you want) and lets keep this project buggy so that in future people can choose alternative frameworks and cloud services for their projects.

@katowulf
Copy link
Contributor

@bogacg There are around 500 issues reported across the Firebase repositories (not counting the plethora specific to Angular projects) daily. Each takes around 15-30m to reproduce. As you can imagine, those add up, and most turn out to be red herrings. This one looks valid, as you've noted. But, just help a brother out and follow the rules by saving us some time and energy.

@cartant
Copy link
Contributor

cartant commented Dec 1, 2016

@katowulf There is a Plunker that reproduces the issue here. The relevant code is in app/app.component.ts.

The fix would be pretty minor; all that should be required is for this line:

if (utils.isPresent(query.equalTo)) {

to be changed to:

if (query.equalTo !== undefined) {

If I have time, tomorrow, I'll submit a PR.

@katowulf
Copy link
Contributor

katowulf commented Dec 1, 2016

Thanks. I think that the misleading method name here has probably resulted in this obscure bug. We should probably proceed as follows:

  • Remove utils.isPresent() (which is a misleading name)
  • Create utils.isEmpty(<any>: val) (still not perfectly happy with this name; suggestions welcome)
  • Change current usages (about 5) of utils.isPresent() to !utils.isEmpty()
  • Add utils.hasKey(Object: o, String: key), which can just return o[key] !== undefined
  • Set this particular usage to if( utils.hasKey(query, 'equalTo') )

@cartant
Copy link
Contributor

cartant commented Dec 2, 2016

@katowulf Regarding utils.isEmpty, lodash has a similar function named isNil. Would that name be preferred? Or isNullish?

cartant added a commit to cartant/angularfire that referenced this issue Dec 10, 2016
isPresent is a misleading name (see angular#704) as it considers options
specified as null to not be present.
cartant added a commit to cartant/angularfire that referenced this issue Dec 10, 2016
Values passed for equalTo, startAt and endAt should be allowed to be
null.

Closes angular#704.
katowulf pushed a commit that referenced this issue Dec 12, 2016
isPresent is a misleading name (see #704) as it considers options
specified as null to not be present.
@bogacg
Copy link
Author

bogacg commented Feb 4, 2017

This issue still persists with latest versions:
Plunkr

@cartant
Copy link
Contributor

cartant commented Feb 5, 2017

@bogacg Indeed, it is still broken. Apparently, I missed replacing the isNil calls in the query observable with calls to hasKey - so the equalTo is dropped from the query before it's later tested to be passed to the SDK. I will fix it and will add some tests, this time.

@cartant
Copy link
Contributor

cartant commented Feb 5, 2017

@bogacg See #809.

@leeandro0
Copy link

i will have the issue, null here does not work

      this.notmybooks = angfire.database.list('/user-books', {
        query: {
          orderByChild: 'likedby/tzROGF1Tk4NcobrTE70ZKQzoKom1',
          equalTo: null
        }
      });

@marcellokabora
Copy link

@leeandro0 try equalTo: undefined, this work for me.

@aturowsk
Copy link

My quick this is shown below.

  this.notmybooks = angfire.database.list('/user-books', {
    query: {
      orderByChild: 'likedby/tzROGF1Tk4NcobrTE70ZKQzoKom1',
      equalTo: book==null ?"" : book //fix null values returning whole list
    }
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants