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

feat(firestore): support "!=" and "not-in" query operators #3207

Merged
merged 4 commits into from Nov 17, 2020

Conversation

@jacksgt
Copy link
Contributor

@jacksgt jacksgt commented Nov 13, 2020

Firestore Version 7.21.0 - September 17, 2020 - introduced support for
the "!=" (NOT_EQUAL) and "not-in" (NOT_IN) query operators:
https://firebase.google.com/support/release-notes/js#version_7210_-_september_17_2020

Query operator documentation:
https://firebase.google.com/docs/firestore/query-data/queries

This patch adds support for these two new operators and adds test
cases for them.

Closes #2974 #2974

@jacksgt jacksgt requested review from tritone and as code owners Nov 13, 2020
@google-cla
Copy link

@google-cla google-cla bot commented Nov 13, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Loading

@jacksgt
Copy link
Contributor Author

@jacksgt jacksgt commented Nov 13, 2020

@googlebot I signed it!

Loading

@google-cla google-cla bot added cla: yes and removed cla: no labels Nov 13, 2020
Copy link
Collaborator

@tbpg tbpg left a comment

Thank you! Looks good! Would you mind adding an integration test?

{q, wants},
{q.Where("q", ">", 1), wants[2:]},
{q.WherePath([]string{"q"}, ">", 1), wants[2:]},
{q.Offset(1).Limit(1), wants[1:2]},
{q.StartAt(1), wants[1:]},
{q.StartAfter(1), wants[2:]},
{q.EndAt(1), wants[:2]},
{q.EndBefore(1), wants[:1]},
{q.LimitToLast(2), wants[1:]},

Loading

@tritone
Copy link
Collaborator

@tritone tritone commented Nov 13, 2020

Thank you! Looks good! Would you mind adding an integration test?

{q, wants},
{q.Where("q", ">", 1), wants[2:]},
{q.WherePath([]string{"q"}, ">", 1), wants[2:]},
{q.Offset(1).Limit(1), wants[1:2]},
{q.StartAt(1), wants[1:]},
{q.StartAfter(1), wants[2:]},
{q.EndAt(1), wants[:2]},
{q.EndBefore(1), wants[:1]},
{q.LimitToLast(2), wants[1:]},

I actually don't see most operators tested in the integration test-- maybe we can deal with that separately?

Loading

Copy link
Collaborator

@tritone tritone left a comment

Looks good to me. @BenWhitehead @crwilcox FYI

Loading

@tbpg
Copy link
Collaborator

@tbpg tbpg commented Nov 13, 2020

I actually don't see most operators tested in the integration test-- maybe we can deal with that separately?

I'd prefer to do it now. Existing poor coverage is not a good reason to avoid new coverage. 😃

Loading

@jacksgt jacksgt force-pushed the new-firestore-operators branch from 3c00c2e to 12ca3f8 Nov 14, 2020
@jacksgt
Copy link
Contributor Author

@jacksgt jacksgt commented Nov 14, 2020

Hey, I added some integration tests for value operators. I hope understood the testing logic correctly.
Can the CI run these tests?

Loading

firestore/query.go Outdated Show resolved Hide resolved
Loading
firestore/query.go Outdated Show resolved Hide resolved
Loading
@jacksgt jacksgt force-pushed the new-firestore-operators branch from 12ca3f8 to 30aec42 Nov 17, 2020
Copy link
Collaborator

@tbpg tbpg left a comment

One more test case then LGTM.

Loading

{q.Where("q", "<", 1), wants[:1]},
{q.Where("q", "==", 1), wants[1:2]},
{q.Where("q", "!=", 0), wants[1:]},
{q.Where("q", ">=", 1), wants[1:]},
{q.Where("q", "<=", 1), wants[:2]},
Copy link
Collaborator

@tbpg tbpg Nov 17, 2020

Choose a reason for hiding this comment

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

Thank you for adding these! Would you mind doing one more for not-in?

Loading

@jacksgt
Copy link
Contributor Author

@jacksgt jacksgt commented Nov 17, 2020

Is it true that I cannot see the kokoro test results? It just says "Expected - Waiting for status to be reported" (maybe I'm just blind...)

Loading

@tbpg
Copy link
Collaborator

@tbpg tbpg commented Nov 17, 2020

The tests just haven't run on the latest commit yet. I triggered them again.

Loading

Firestore Version 7.21.0 - September 17, 2020 - introduced support for
the "!=" (NOT_EQUAL) and "not-in" (NOT_IN) query operators:
https://firebase.google.com/support/release-notes/js#version_7210_-_september_17_2020

Query operator documentation:
https://firebase.google.com/docs/firestore/query-data/queries

This patch adds support for these two new operators and adds test
cases for them.
@jacksgt jacksgt force-pushed the new-firestore-operators branch from 30aec42 to 59a87e0 Nov 17, 2020
Copy link
Collaborator

@tbpg tbpg left a comment

Looks like a couple other tests are failing as a result of this change:

=== RUN   TestQueryToProto
    query_test.go:81: firestore: must use '==' when comparing NaN
--- FAIL: TestQueryToProto (0.00s)
=== RUN   TestQueryToProtoErrors
    query_test.go:492: query 1 "{c:0xc0003b9aa0 path:projects/P/databases/DB/documents/C parentPath:projects/P/databases/DB/documents collectionID:C selection:[] filters:[{fieldPath:[x] op:!= value:1}] orders:[] offset:0 limit:<nil> limitToLast:false startVals:[] endVals:[] startDoc:<nil> endDoc:<nil> startBefore:false endBefore:false err:<nil> allDescendants:false}": got nil, want error
--- FAIL: TestQueryToProtoErrors (0.00s)

Loading

@tbpg
Copy link
Collaborator

@tbpg tbpg commented Nov 17, 2020

Also, we'll take care of squashing and editing the commit message when we merge the PR. So, you can add commits as you go, rather than force pushing every time.

Loading

@jacksgt
Copy link
Contributor Author

@jacksgt jacksgt commented Nov 17, 2020

Looks like a couple other tests are failing as a result of this change:

=== RUN   TestQueryToProto
    query_test.go:81: firestore: must use '==' when comparing NaN
--- FAIL: TestQueryToProto (0.00s)
=== RUN   TestQueryToProtoErrors
    query_test.go:492: query 1 "{c:0xc0003b9aa0 path:projects/P/databases/DB/documents/C parentPath:projects/P/databases/DB/documents collectionID:C selection:[] filters:[{fieldPath:[x] op:!= value:1}] orders:[] offset:0 limit:<nil> limitToLast:false startVals:[] endVals:[] startDoc:<nil> endDoc:<nil> startBefore:false endBefore:false err:<nil> allDescendants:false}": got nil, want error
--- FAIL: TestQueryToProtoErrors (0.00s)

Yes, I saw that. Should be fixed now.

Also, we'll take care of squashing and editing the commit message when we merge the PR. So, you can add commits as you go, rather than force pushing every time.

Cool, thanks!

Loading

{q.Where("q", "in", [0, 1]), wants[:2]},
{q.Where("q", "not-in", [0, 1]), wants[2:]},
Copy link
Collaborator

@tbpg tbpg Nov 17, 2020

Choose a reason for hiding this comment

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

Looks like you need to fully initialize the slice here. Something like:

Suggested change
{q.Where("q", "in", [0, 1]), wants[:2]},
{q.Where("q", "not-in", [0, 1]), wants[2:]},
{q.Where("q", "in", []int{0, 1}), wants[:2]},
{q.Where("q", "not-in", []int{0, 1}), wants[2:]},

Loading

tbpg
tbpg approved these changes Nov 17, 2020
Copy link
Collaborator

@tbpg tbpg left a comment

Thank you for implementing this!

Loading

@jacksgt
Copy link
Contributor Author

@jacksgt jacksgt commented Nov 17, 2020

Thank you for your kind support and pushing me all the way through! :-)

Loading

@tbpg tbpg merged commit 5c44019 into googleapis:master Nov 17, 2020
3 checks passed
Loading
@tbpg
Copy link
Collaborator

@tbpg tbpg commented Nov 17, 2020

@jacksgt would you mind testing with a pseudo-version to make sure it's all working well for you before we tag a release?

Loading

@jacksgt
Copy link
Contributor Author

@jacksgt jacksgt commented Nov 18, 2020

Yes, sure!
I have updated this module to the latest version:

go get cloud.google.com/go/firestore@master
go: cloud.google.com/go/firestore master => v1.3.1-0.20201118043013-99f7212bd70b
go: downloading cloud.google.com/go/firestore v1.3.1-0.20201118043013-99f7212bd70b
go: downloading google.golang.org/genproto v0.0.0-20201117123952-62d171c70ae1
go: downloading github.com/google/go-cmp v0.5.3
go: downloading golang.org/x/tools v0.0.0-20201117021029-3c3a81204b10

And ran my application again - it works!

Loading

@tritone
Copy link
Collaborator

@tritone tritone commented Nov 18, 2020

Yes, sure!
I have updated this module to the latest version:

go get cloud.google.com/go/firestore@master
go: cloud.google.com/go/firestore master => v1.3.1-0.20201118043013-99f7212bd70b
go: downloading cloud.google.com/go/firestore v1.3.1-0.20201118043013-99f7212bd70b
go: downloading google.golang.org/genproto v0.0.0-20201117123952-62d171c70ae1
go: downloading github.com/google/go-cmp v0.5.3
go: downloading golang.org/x/tools v0.0.0-20201117021029-3c3a81204b10

And ran my application again - it works!

Thanks for confirming!

Loading

tritone added a commit to tritone/google-cloud-go that referenced this issue Nov 18, 2020
…oogleapis#3207)"

This reverts commit 5c44019.

Reverting due to integration test failure.

Fixes googleapis#3223
@tbpg
Copy link
Collaborator

@tbpg tbpg commented Nov 18, 2020

Glad it worked! This needed a small update to make the integration test pass, but otherwise seems to be working well.

Loading

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

Successfully merging this pull request may close these issues.

5 participants