feat(auth): Add bulk get/delete methods #325

rsgowman · 2020-01-15T19:19:41Z

This PR allows callers to retrieve a list of users by unique identifier (uid, email, phone, federated provider uid) as well as to delete a list of users.

Resolves #138

RELEASE NOTE: Added GetUsers() and DeleteUsers() APIs for retrieving and deleting user accounts in bulk.

hiranya911

I've provided some high-level feedback on the implementation and unit tests. The implementation looks correct, just needs a bit polishing to fit Go conventions. Will look at the integration tests in the next round.

auth/user_mgt.go

auth/user_mgt_test.go

auth/user_mgt.go

auth/user_mgt_test.go

And inline the implementation.

hiranya911

Looks pretty good. I added some comments on how we can improve the implementation and reduce some of the repetition. Also added comments on integration tests.

auth/user_mgt.go

auth/user_mgt_test.go

integration/auth/user_mgt_test.go

This reverts commit ae498f7.

hiranya911

Thanks for making the changes. Implementation looks pretty solid. Just a few comments on error reporting in the test cases.

auth/user_mgt.go

auth/user_mgt_test.go

integration/auth/user_mgt_test.go

hiranya911

Looks great. Just a comment on godoc convention.

auth/user_mgt.go

hiranya911

Thanks. LGTM 👍

If the API proposal is in an approved state, feel free to merge.

egilmorez

Some style things to look at. Thanks!

egilmorez · 2020-02-19T16:48:43Z

auth/user_mgt.go

+
+// A GetUsersResult represents the result of the GetUsers() API.
+type GetUsersResult struct {
+	// Set of UserRecords, corresponding to the set of users that were requested.


Suggest omitting this comma.

egilmorez · 2020-02-19T16:49:26Z

auth/user_mgt.go

+// result list is not guaranteed to correspond to the nth entry in the input
+// parameters list.
+//
+// Only a maximum of 100 identifiers may be supplied. If more than 100


Suggest "A maximum..."

egilmorez · 2020-02-19T16:49:46Z

auth/user_mgt.go

+// parameters list.
+//
+// Only a maximum of 100 identifiers may be supplied. If more than 100
+// identifiers are supplied, this method will immediately return an error.


Suggest "method returns an error."

egilmorez · 2020-02-19T16:50:23Z

auth/user_mgt.go

+// A DeleteUsersResult represents the result of the DeleteUsers() call.
+type DeleteUsersResult struct {
+	// The number of users that were deleted successfully (possibly zero). Users
+	// that did not exist prior to calling DeleteUsers() will be considered to be


Suggest "are considered"

egilmorez · 2020-02-19T16:51:25Z

auth/user_mgt.go

+	// The number of users that failed to be deleted (possibly zero).
+	FailureCount int
+
+	// A list of describing the errors that were encountered


Delete "of" if this list actually describes the errors. Otherwise, just "A list of errors encountered..."

Suggest checking whether this was copied into similar comment in other SDKs, and we just missed it :)

Oh; the type somehow got dropped. (Which is mildly redundant info in most ports since you can get it from the type signature; python being the obvious exception.) The other ports seem ok.

I've added the type to match the other ports. Alternatively, we could just remove the type everywhere (except python).

egilmorez · 2020-02-19T16:53:53Z

auth/user_mgt.go

+// DeleteUsers deletes the users specified by the given identifiers.
+//
+// Deleting a non-existing user won't generate an error. (i.e. this method is
+// idempotent.) Non-existing users will be considered to be successfully


Suggest "are considered"

egilmorez · 2020-02-19T16:54:13Z

auth/user_mgt.go

+//
+// Deleting a non-existing user won't generate an error. (i.e. this method is
+// idempotent.) Non-existing users will be considered to be successfully
+// deleted, and will therefore be counted in the DeleteUsersResult.SuccessCount


Suggest "are counted"

egilmorez · 2020-02-19T16:54:26Z

auth/user_mgt.go

+// deleted, and will therefore be counted in the DeleteUsersResult.SuccessCount
+// value.
+//
+// Only a maximum of 1000 identifiers may be supplied. If more than 1000


Suggest "A maximum..."

egilmorez · 2020-02-19T16:54:48Z

auth/user_mgt.go

+// value.
+//
+// Only a maximum of 1000 identifiers may be supplied. If more than 1000
+// identifiers are supplied, this method will immediately return an error.


Suggest "method returns an error"

* chore: Added Actions-based release workflow * Set GOPATH * Fixed working directory for tests * Decrypting credentials into the testdata directory * Added preflight and post check scripts

cbonnie

Left minor comments, otherwise LGTM (reviewing on behalf of egilmore@).

cbonnie · 2020-03-09T15:11:28Z

auth/user_mgt.go

+	// Maximum allowed number of users to batch get at one time.
+	maxGetAccountsBatchSize = 100
+
+	// Maximum allowed numberof users to batch delete at one time.


Add space between "number" and "of"

cbonnie · 2020-03-09T15:12:56Z

auth/user_mgt.go

@@ -34,6 +34,12 @@ const (
 	maxLenPayloadCC     = 1000
 	defaultProviderID   = "firebase"
 	idToolkitV1Endpoint = "https://identitytoolkit.googleapis.com/v1"
+
+	// Maximum allowed number of users to batch get at one time.


Suggest: "Maximum number of users allowed to batch get at a time."

Done. I've also applied this to the maxDeleteAccountsBatchSize comment immediately below.

cbonnie · 2020-03-09T15:14:33Z

auth/user_mgt.go

@@ -57,6 +63,9 @@ type UserInfo struct {
 type UserMetadata struct {
 	CreationTimestamp  int64
 	LastLogInTimestamp int64
+	// The time at which the user was last active (ID token refreshed), or 0 if
+	// the user was never active.
+	LastRefreshTimestamp int64
 }

 // UserRecord contains metadata associated with a Firebase user account.


UserRecord? Not familiar with the style guide for GO, please add back ticks if necessary.

It doesn't seem to be a thing; or at least, they don't do it here: https://blog.golang.org/godoc-documenting-go-code or here: https://golang.org/doc/effective_go.html#commentary

Nonetheless, we do seem to use backticks occasionally... though seemingly only when referring to some other function (and I suspect inconsistently at that.)

I've left this alone.

egilmorez

Thanks!

hiranya911

Looks good. Just a couple of clean ups needed.

hiranya911 · 2020-05-19T22:37:37Z

.github/scripts/publish_post_check.sh

@@ -0,0 +1,105 @@
+


Let's remove this along with stage.yml and publish.yml from the PR. They are no longer in the dev branch.

hiranya911 · 2020-05-19T22:39:18Z

auth/user_mgt.go

@@ -331,6 +340,7 @@ const (
 	unauthorizedContinueURI  = "unauthorized-continue-uri"
 	unknown                  = "unknown-error"
 	userNotFound             = "user-not-found"
+	maximumUserCountExceeded = "maximum-user-count-exceeded"


This shouldn't be needed. We don't use error codes for developer errors.

hiranya911

Thanks. LGTM 👍

rsgowman self-assigned this Jan 15, 2020

rsgowman added 6 commits January 15, 2020 15:10

Add bulk get method

a62a8e8

Add LastRefreshTimestamp field to users returned via GetAccountInfo rpc

7f11d80

Add DeleteUsers() bulk deletion method

2a212b3

golint

d82578b

%w -> %v. (Old versions don't support it?)

44dae7a

go vet

2c9da85

rsgowman force-pushed the rsgowman/bulk_get branch from 1a9b43f to 2c9da85 Compare January 15, 2020 20:10

chore: Removed Travis CI integration (#326)

43c7e77

rsgowman requested a review from hiranya911 January 16, 2020 20:16

rsgowman assigned hiranya911 and unassigned rsgowman Jan 16, 2020

hiranya911 suggested changes Jan 18, 2020

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Jan 18, 2020

rsgowman added 2 commits January 22, 2020 13:15

review feedback

48b371f

Remove matchesUserRecord interface method

ae498f7

And inline the implementation.

rsgowman assigned hiranya911 and unassigned rsgowman Jan 22, 2020

hiranya911 suggested changes Jan 22, 2020

View reviewed changes

rsgowman added 2 commits January 23, 2020 12:22

Revert "Remove matchesUserRecord interface method"

f665813

This reverts commit ae498f7.

review feedback 2

7937fff

hiranya911 suggested changes Jan 27, 2020

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Jan 27, 2020

review feedback 3

14c7264

rsgowman assigned hiranya911 and unassigned rsgowman Jan 28, 2020

hiranya911 suggested changes Jan 28, 2020

View reviewed changes

auth/user_mgt.go Outdated Show resolved Hide resolved

auth/user_mgt.go Outdated Show resolved Hide resolved

hiranya911 removed their assignment Jan 28, 2020

rsgowman assigned hiranya911 Jan 29, 2020

hiranya911 approved these changes Jan 29, 2020

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Jan 29, 2020

rsgowman assigned egilmorez Feb 18, 2020

egilmorez reviewed Feb 19, 2020

View reviewed changes

hiranya911 and others added 4 commits February 19, 2020 13:10

chore: Added Actions-based release workflow (#331)

aa4e7a4

* chore: Added Actions-based release workflow * Set GOPATH * Fixed working directory for tests * Decrypting credentials into the testdata directory * Added preflight and post check scripts

doc review feedback

87fd378

go fmt

4fa1fba

Merge remote-tracking branch 'origin/dev' into rsgowman/bulk_get

2f3994a

cbonnie approved these changes Mar 9, 2020

View reviewed changes

review feedback (docs)

4b82bdd

hiranya911 force-pushed the dev branch from 5e7858b to 4343fa8 Compare April 23, 2020 19:08

rsgowman and others added 2 commits May 19, 2020 16:00

Merge remote-tracking branch 'origin/dev' into rsgowman/bulk_get

4ac55e0

Tighten bounds of lastRefreshTime in the test (and fix bug!)

89b033f

egilmorez approved these changes May 19, 2020

View reviewed changes

lint/fmt

c73a300

hiranya911 suggested changes May 19, 2020

View reviewed changes

review feedback

0774722

rsgowman assigned hiranya911 and unassigned egilmorez and rsgowman May 20, 2020

hiranya911 approved these changes May 20, 2020

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 May 20, 2020

rsgowman merged commit 96a7111 into dev May 20, 2020

rsgowman deleted the rsgowman/bulk_get branch May 20, 2020 20:48

hiranya911 added the release-note label May 20, 2020

hiranya911 changed the title ~~Add bulk get/delete methods~~ feat(auth): Add bulk get/delete methods May 20, 2020

feat(auth): Add bulk get/delete methods #325

feat(auth): Add bulk get/delete methods #325

Uh oh!

Conversation

rsgowman commented Jan 15, 2020 • edited by hiranya911 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hiranya911 left a comment

Choose a reason for hiding this comment

Uh oh!

egilmorez left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rsgowman commented Jan 15, 2020 •

edited by hiranya911

Loading