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

rsgowman · 2020-02-12T20:14:45Z

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.

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

rsgowman · 2020-02-12T20:53:05Z

src/main/java/com/google/firebase/auth/FirebaseUserManager.java

@@ -171,6 +178,37 @@ UserRecord getUserByPhoneNumber(String phoneNumber) throws FirebaseAuthException
    return new UserRecord(response.getUsers().get(0), jsonFactory);
  }

+  /**
+   * @pre identifiers != null


These are verified in the calling code. (As an alternative, we could just repeat the validation here.)

Let's drop the pre annotations. That's not really a convention we use. It is understood that the arguments passed to this class have already been validated. If you want add a class-level javadoc to indicate that explicitly.

rsgowman · 2020-02-12T20:56:13Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

+   * don't run into quota exceeded errors.
+   */
+  // TODO(rsgowman): When/if the rate limit is relaxed, eliminate this helper.
+  private ApiFuture<DeleteUsersResult> slowDeleteUsersAsync(List<String> uids) throws Exception {


We may need to do something similar for the other ports... There seems to be some elasticity such that the full test suite can often pass, but not if you run just the delete users tests in quick succession (as I did while testing).

rsgowman · 2020-02-12T20:57:49Z

src/test/java/com/google/firebase/auth/GetUsersIT.java

@@ -0,0 +1,152 @@
+/*


junit4 doesn't allow nested tests (and therefore doesn't allow nested setup/teardown functions.) The recommendation is to just split into a separate file, which is what I've done here.

hiranya911

Thanks @rsgowman. Looks pretty great. Just pointed out a few tidbits to improve on.

Adding @egilmorez to review the docs portion.

src/main/java/com/google/firebase/auth/DeleteUsersResult.java

src/main/java/com/google/firebase/auth/EmailIdentifier.java

src/main/java/com/google/firebase/auth/FirebaseAuth.java

hiranya911 · 2020-02-18T18:32:05Z

src/main/java/com/google/firebase/auth/FirebaseUserManager.java

@@ -171,6 +178,37 @@ UserRecord getUserByPhoneNumber(String phoneNumber) throws FirebaseAuthException
    return new UserRecord(response.getUsers().get(0), jsonFactory);
  }

+  /**
+   * @pre identifiers != null


Let's drop the pre annotations. That's not really a convention we use. It is understood that the arguments passed to this class have already been validated. If you want add a class-level javadoc to indicate that explicitly.

src/main/java/com/google/firebase/auth/UserRecord.java

src/main/java/com/google/firebase/auth/internal/GetAccountInfoRequest.java

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

src/test/java/com/google/firebase/auth/GetUsersIT.java

egilmorez · 2020-02-18T19:11:38Z

src/main/java/com/google/firebase/auth/DeleteUsersResult.java

+
+  /**
+   * Returns the number of users that were deleted successfully (possibly zero). Users that did
+   * not exist prior to calling deleteUsersAsync() will be considered to be successfully


Suggest code font/link for the method and present tense:

{@link deleteUsersAsync()} are considered

egilmorez · 2020-02-18T19:13:28Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+   * <p>There are no ordering guarantees; in particular, the nth entry in the users result list is
+   * not guaranteed to correspond to the nth entry in the input parameters list.
+   *
+   * <p>Only a maximum of 100 identifiers may be supplied. If more than 100 identifiers are


Suggest just "A maximum . . . "

egilmorez · 2020-02-18T19:14:29Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+   * not guaranteed to correspond to the nth entry in the input parameters list.
+   *
+   * <p>Only a maximum of 100 identifiers may be supplied. If more than 100 identifiers are
+   * supplied, this method will immediately throw an IllegalArgumentException.


Suggest "this method throws an {@link ...}

egilmorez · 2020-02-18T19:15:48Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+   * <p>There are no ordering guarantees; in particular, the nth entry in the users result list is
+   * not guaranteed to correspond to the nth entry in the input parameters list.
+   *
+   * <p>Only a maximum of 100 identifiers may be supplied. If more than 100 identifiers are


If we say "specified" above, this should probably work that way too.

Same on the present tense/link format in line 635.

egilmorez · 2020-02-18T19:16:55Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+   * Non-existing users will be considered to be successfully deleted, and will therefore be counted
+   * in the DeleteUsersResult.getSuccessCount() value.
+   *
+   * <p>Only a maximum of 1000 identifiers may be supplied. If more than 1000 identifiers are


Similar changes here and next line as in 634 and 635.

egilmorez · 2020-02-18T19:18:40Z

src/main/java/com/google/firebase/auth/GetUsersResult.java

+  }
+
+  /**
+   * Set of user records, corresponding to the set of users that were requested. Only users


Suggest omitting this comma.

egilmorez · 2020-02-18T19:22:12Z

src/main/java/com/google/firebase/auth/internal/BatchDeleteResponse.java

+import java.util.List;
+
+/**
+ * Represents the response from identity toolkit for a batch delete request.


Should this be "the identity toolkit?"

The article would normally be omitted only if this is a name, like uppercase "Identity Toolkit"

Maybe? I think I just copied it from UploadAccountResponse. Other locations use 'Google identity toolkit' (HttpErrorResponse, GetAccountInfoResponse, DownloadAccountResponse.) FirebaseUserManager uses 'Google Identity Toolkit'.

I've switched to 'Google Identity Toolkit'.

(Though note that this is an internal object, so shouldn't show up in the public api docs regardless.)

egilmorez · 2020-02-18T19:22:42Z

src/main/java/com/google/firebase/auth/internal/BatchDeleteResponse.java

+    @Key("message")
+    private String message;
+
+    // A 'localId' field also exists here, but is not currently exposed in the admin sdk.


Suggest caps for "Admin SDK."

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

egilmorez · 2020-02-18T19:25:14Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

+        .setPassword("password"));
+
+    try {
+      // New users should not have a lastRefreshTimestamp set.


Suggest @link or @code for all instances of this.

This isn't a javadoc comment; is it conventional to use these types of annotations for inline docs like this?

i.e.: clang-format src/test/java/com/google/firebase/auth/FirebaseAuthIT.java \ --lines=140:215 --lines=328:356 --lines=822:838 -i

…sers

…nitializeApp

hiranya911

Thanks @rsgowman. LGTM 👍

cbonnie

Added some comments, otherwise LGTM (reviewing on behalf of egilmore@).

cbonnie · 2020-03-09T15:44:21Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+    }
+    return false;
+  }
+
  /**
   * Gets a page of users starting from the specified {@code pageToken}. Page size will be


"Page size is limited to..."

cbonnie · 2020-03-09T15:47:22Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+  /**
+   * Deletes the users specified by the given identifiers.
+   *
+   * <p>Deleting a non-existing user won't generate an error. (i.e. this method is idempotent.)


Suggest:

"Deleting a non-existing user does not generate an error (the method is idempotent). Non-existing users are considered to be successfully deleted and are therefore included in the DeleteUsersResult.getSuccessCount() value."

cbonnie · 2020-03-09T15:50:57Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+   * <p>A maximum of 1000 identifiers may be supplied. If more than 1000 identifiers are
+   * supplied, this method throws an {@link IllegalArgumentException}.
+   *
+   * <p>This API is currently rate limited at the server to 1 QPS. If you exceed this, you may get a


Suggest:

"This API has a rate limit of 1 QPS. Exceeding the limit may result in a quota exceeded error. If you want to delete more than 1000 users, we suggest adding a delay to ensure you don't exceed the limit."

cbonnie · 2020-03-09T15:53:27Z

src/main/java/com/google/firebase/auth/FirebaseAuth.java

+      }
+    };
+  }
+
  /**
   * Imports the provided list of users into Firebase Auth. At most 1000 users can be imported at a
   * time. This operation is optimized for bulk imports and will ignore checks on identifier


Suggest:

"Imports the provided list of users into Firebase Auth. You can import a maximum of 1000 users at a time. This operation is optimized for bulk imports and ignores checks on user identifiers."

(Not completely sure about the user identifiers part, I'm assuming that's what the operation would've checked for)

I changed the first two sentences as per your suggestion. I've changed the last to "This operation is optimized for bulk imports and does not check identifier uniqueness which could result in duplications".

cbonnie · 2020-03-09T15:54:53Z

src/main/java/com/google/firebase/auth/internal/GetAccountInfoRequest.java

+import java.util.List;
+
+/**
+ * Represents the request to lookup account information.


"look up" (verb)

cbonnie · 2020-03-09T15:55:19Z

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

+    assertEquals(0, result.getFailureCount());
+    assertTrue(result.getErrors().isEmpty());
+
+    // Delete the user again, ensuring that everything still counts as a success.


Suggest: "Delete the user again to ensure that..."

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

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java

+  }
+
+  /**
+   * The {@code batchDelete} endpoint is currently rate limited to 1qps. Use this test helper to


"The {@code batchDelete} endpoint has a rate limit of 1 QPS. Use this test helper to ensure you don't exceed the quota."

…sers

hiranya911

LGTM 👍

egilmorez

Thanks!

* Bump netty.version from 4.1.34.Final to 4.1.45.Final (#373) Bumps `netty.version` from 4.1.34.Final to 4.1.45.Final. Updates `netty-codec-http` from 4.1.34.Final to 4.1.45.Final - [Release notes](https://github.com/netty/netty/releases) - [Commits](netty/netty@netty-4.1.34.Final...netty-4.1.45.Final) Updates `netty-handler` from 4.1.34.Final to 4.1.45.Final - [Release notes](https://github.com/netty/netty/releases) - [Commits](netty/netty@netty-4.1.34.Final...netty-4.1.45.Final) Updates `netty-transport` from 4.1.34.Final to 4.1.45.Final - [Release notes](https://github.com/netty/netty/releases) - [Commits](netty/netty@netty-4.1.34.Final...netty-4.1.45.Final) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat(auth): Add bulk get/delete methods (#365) 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. RELEASE NOTE: Added getUsers() and deleteUsers() APIs for retrieving and deleting user accounts in bulk. * chore: Setting the version of the Maven Javadoc plugin (#412) * [chore] Release 6.13.0 (#413) * [chore] Release 6.13.0 take 2 (#414) * Upgated the gpg keys * Added temp verify script * Disabled tty for gpg import * Removing temp verification script * Updated publish commands * [chore] Release 6.13.0 take 3 (#415) * [chore] Release 6.13.0 take 4 (#416) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: rsgowman <rich@gowman.noip.me>

rsgowman self-assigned this Feb 12, 2020

rsgowman force-pushed the rsgowman/bulk_get_users branch 2 times, most recently from b9c332d to 9f0c4ee Compare February 12, 2020 20:41

rsgowman added 3 commits February 12, 2020 15:51

Bulk get users

74c76ea

add lastRefreshTimestamp

1615fb3

Bulk delete users

69b37ae

rsgowman force-pushed the rsgowman/bulk_get_users branch from 9f0c4ee to 69b37ae Compare February 12, 2020 20:52

rsgowman requested a review from hiranya911 February 12, 2020 20:58

rsgowman assigned hiranya911 and unassigned rsgowman Feb 12, 2020

rsgowman commented Feb 12, 2020

View reviewed changes

oops; forgot to add this to the commit

93a043c

hiranya911 suggested changes Feb 18, 2020

View reviewed changes

hiranya911 assigned rsgowman and unassigned hiranya911 Feb 18, 2020

hiranya911 requested a review from egilmorez February 18, 2020 18:57

hiranya911 assigned egilmorez Feb 18, 2020

egilmorez reviewed Feb 18, 2020

View reviewed changes

src/test/java/com/google/firebase/auth/FirebaseAuthIT.java Outdated Show resolved Hide resolved

egilmorez reviewed Feb 18, 2020

View reviewed changes

rsgowman added 2 commits February 20, 2020 10:45

review feedback

9bd3813

clang-format over FirebaseAuthIT.java

df3519e

i.e.: clang-format src/test/java/com/google/firebase/auth/FirebaseAuthIT.java \ --lines=140:215 --lines=328:356 --lines=822:838 -i

rsgowman removed their assignment Feb 20, 2020

rsgowman assigned hiranya911 Feb 20, 2020

rsgowman added 2 commits February 20, 2020 11:35

Merge remote-tracking branch 'origin/master' into rsgowman/bulk_get_u…

727b15b

…sers

Switch unit tests to use .setCredentials() when calling FirebaseApp.i…

a67aef8

…nitializeApp

hiranya911 approved these changes Feb 20, 2020

View reviewed changes

docs typo

cf7026e

hiranya911 removed their assignment Feb 21, 2020

rsgowman unassigned egilmorez Feb 26, 2020

cbonnie approved these changes Mar 9, 2020

View reviewed changes

rsgowman added 2 commits May 7, 2020 14:22

Merge remote-tracking branch 'origin/master' into rsgowman/bulk_get_u…

844d751

…sers

doc fixups

5afc278

hiranya911 approved these changes May 8, 2020

View reviewed changes

hiranya911 assigned rsgowman May 8, 2020

egilmorez approved these changes May 12, 2020

View reviewed changes

rsgowman merged commit f8052c7 into master May 12, 2020

rsgowman deleted the rsgowman/bulk_get_users branch May 12, 2020 20:56

rsgowman added the release-note label May 12, 2020

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

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

Conversation

rsgowman commented Feb 12, 2020 • edited by hiranya911

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egilmorez Feb 18, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

cbonnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiranya911 left a comment

Choose a reason for hiding this comment

egilmorez left a comment

Choose a reason for hiding this comment

rsgowman commented Feb 12, 2020 •

edited by hiranya911

egilmorez Feb 18, 2020 •

edited