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

jacksgt · 2020-11-13T10:37:58Z

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

google-cla · 2020-11-13T10:38:02Z

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

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

jacksgt · 2020-11-13T10:48:51Z

@googlebot I signed it!

tbpg

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

google-cloud-go/firestore/integration_test.go

Lines 652 to 660 in 581bf92

    
           {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:]},

tritone · 2020-11-13T16:24:38Z

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

google-cloud-go/firestore/integration_test.go

Lines 652 to 660 in 581bf92

{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?

tritone

Looks good to me. @BenWhitehead @crwilcox FYI

tbpg · 2020-11-13T16:40:44Z

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. 😃

jacksgt · 2020-11-14T10:35:09Z

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

firestore/query.go

tbpg

One more test case then LGTM.

tbpg · 2020-11-17T15:39:35Z

firestore/integration_test.go

+		{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]},


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

jacksgt · 2020-11-17T15:42:50Z

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...)

tbpg · 2020-11-17T15:47:06Z

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

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.

tbpg

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)

tbpg · 2020-11-17T16:21:34Z

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.

jacksgt · 2020-11-17T18:29:43Z

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!

tbpg · 2020-11-17T20:03:54Z

firestore/integration_test.go

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


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:]},

tbpg

Thank you for implementing this!

jacksgt · 2020-11-17T20:59:40Z

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

tbpg · 2020-11-17T21:51:27Z

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

jacksgt · 2020-11-18T06:59:11Z

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!

tritone · 2020-11-18T14:35:24Z

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!

…oogleapis#3207)" This reverts commit 5c44019. Reverting due to integration test failure. Fixes googleapis#3223

tbpg · 2020-11-18T18:23:50Z

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

jacksgt requested review from tritone and a team as code owners November 13, 2020 10:37

google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Nov 13, 2020

product-auto-label bot added the api: firestore Issues related to the Firestore API. label Nov 13, 2020

google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Nov 13, 2020

tbpg suggested changes Nov 13, 2020

View reviewed changes

tritone approved these changes Nov 13, 2020

View reviewed changes

tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 13, 2020

jacksgt force-pushed the new-firestore-operators branch from 3c00c2e to 12ca3f8 Compare November 14, 2020 10:32

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 14, 2020

AlisskaPie reviewed Nov 17, 2020

View reviewed changes

firestore/query.go Outdated Show resolved Hide resolved

firestore/query.go Outdated Show resolved Hide resolved

jacksgt force-pushed the new-firestore-operators branch from 12ca3f8 to 30aec42 Compare November 17, 2020 15:11

tbpg suggested changes Nov 17, 2020

View reviewed changes

tbpg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2020

jacksgt force-pushed the new-firestore-operators branch from 30aec42 to 59a87e0 Compare November 17, 2020 16:09

tbpg suggested changes Nov 17, 2020

View reviewed changes

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

b5017ca

tbpg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2020

tbpg suggested changes Nov 17, 2020

View reviewed changes

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

882b373

tbpg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2020

Merge branch 'master' into new-firestore-operators

db251bc

tbpg approved these changes Nov 17, 2020

View reviewed changes

tbpg added automerge: exact Summon MOG for automerging, but approvals need to be against the latest commit kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Nov 17, 2020

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Nov 17, 2020

tbpg merged commit 5c44019 into googleapis:master Nov 17, 2020

gcf-merge-on-green bot removed the automerge: exact Summon MOG for automerging, but approvals need to be against the latest commit label Nov 17, 2020

tritone added a commit to tritone/google-cloud-go that referenced this pull request Nov 18, 2020

Revert "feat(firestore): support "!=" and "not-in" query operators (g…

77c7e3c

…oogleapis#3207)" This reverts commit 5c44019. Reverting due to integration test failure. Fixes googleapis#3223

tritone mentioned this pull request Nov 18, 2020

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jacksgt commented Nov 13, 2020

google-cla bot commented Nov 13, 2020

jacksgt commented Nov 13, 2020

tbpg left a comment

tritone commented Nov 13, 2020

tritone left a comment

tbpg commented Nov 13, 2020

jacksgt commented Nov 14, 2020

tbpg left a comment

tbpg Nov 17, 2020

jacksgt commented Nov 17, 2020

tbpg commented Nov 17, 2020

tbpg left a comment

tbpg commented Nov 17, 2020

jacksgt commented Nov 17, 2020

tbpg Nov 17, 2020

tbpg left a comment

jacksgt commented Nov 17, 2020

tbpg commented Nov 17, 2020

jacksgt commented Nov 18, 2020

tritone commented Nov 18, 2020

tbpg commented Nov 18, 2020

	{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:]},

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

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

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

Conversation

jacksgt commented Nov 13, 2020

google-cla bot commented Nov 13, 2020

What to do if you already signed the CLA

Individual signers

Corporate signers

jacksgt commented Nov 13, 2020

tbpg left a comment

Choose a reason for hiding this comment

tritone commented Nov 13, 2020

tritone left a comment

Choose a reason for hiding this comment

tbpg commented Nov 13, 2020

jacksgt commented Nov 14, 2020

tbpg left a comment

Choose a reason for hiding this comment

tbpg Nov 17, 2020

Choose a reason for hiding this comment

jacksgt commented Nov 17, 2020

tbpg commented Nov 17, 2020

tbpg left a comment

Choose a reason for hiding this comment

tbpg commented Nov 17, 2020

jacksgt commented Nov 17, 2020

tbpg Nov 17, 2020

Choose a reason for hiding this comment

tbpg left a comment

Choose a reason for hiding this comment

jacksgt commented Nov 17, 2020

tbpg commented Nov 17, 2020

jacksgt commented Nov 18, 2020

tritone commented Nov 18, 2020

tbpg commented Nov 18, 2020