Introducing Synchronous Methods to Public APIs #146

hiranya911 · 2018-02-26T21:07:31Z

We are now approaching the final phase of the thread model refactoring work we started last year (#55). So far we have deprecated the Task API and all the methods that return a Task. This PR removes those deprecated public methods, and adds a set of synchronous methods in their place. For instance FirebaseAuth will now have:

String createCustomToken(String uid) // Added in this PR
ApiFuture<String> createCustomTokenAsync(String uid) // Already exists

This is a breaking change, and hence will not be merged/released anytime soon. Instead I'll use this PR as a way to set up the v6 branch, and develop other breaking changes planned for the next major release on top it.

go/firebase-admin-java-sync

schmidt-sebastian

Have you thought of just calling the async methods directly from the synchronous methods? That would get rid of the "...Op()" methods and could drastically reduce the testing code in this PR while retaining the same test coverage.

schmidt-sebastian · 2018-02-28T18:47:40Z

src/main/java/com/google/firebase/FirebaseApp.java

+    try {
+      return initializeApp(getOptionsFromEnvironment(), name);
+    } catch (IOException e) {
+      throw new IllegalArgumentException("Failed to load settings from the environment", e);


Can you be more descriptive here? Maybe something like 'from you system's environment variables'?

schmidt-sebastian · 2018-02-28T18:50:11Z

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

+   * @return A Firebase custom token string.
+   * @throws IllegalArgumentException If the specified uid is null or empty, or if the app has not
+   *     been initialized with service account credentials.
+   * @throws FirebaseAuthException If an error occurs while minting the custom token.


"retrieving" instead of minting? "Minting" seems like an internal term.

Changed to "generating"

schmidt-sebastian · 2018-02-28T19:03:55Z

src/main/java/com/google/firebase/database/DatabaseReference.java

   */
  public ApiFuture<Void> setValueAsync(Object value) {
-    return new TaskToApiFuture<>(setValue(value));
+    return setValueInternal(value, PriorityUtilities.parsePriority(this.path, null), null);


Are we planning on either renaming these back to setValue() or on adding blocking methods?

There are no plans to introduce blocking methods to database, since the implementation is asynchronous all the way down to the core. Developers can wait on the futures if they need blocking-like semantics.

schmidt-sebastian · 2018-02-28T19:09:08Z

src/test/java/com/google/firebase/internal/CallableOperationTest.java

+        if (TEST_FIREBASE_THREAD.equals(threadName)) {
+          return threadName;
+        }
+        return SAME_THREAD;


Should this just return the thread name? Or a boolean/enum with the result of the thread check?

schmidt-sebastian · 2018-02-28T19:16:54Z

src/test/java/com/google/firebase/testing/GenericFunction.java

+/**
+ * A generic operation that can be called with arbitrary arguments.
+ */
+public interface GenericFunction<T> {


I don't see any code that uses more than one argument. If I remember correctly, this Admin SDK is also moving to Java 7. Can you then just use https://docs.oracle.com/javase/8/docs/api/java/util/function/Function.html?

There are some multi arg function calls in FCM tests. But you're right. Once we are on java8 we should be able to clean this up significantly with native Function interface and lambdas. Specifically, I plan to replace most anonymous CallableOp instances with lambdas.

hiranya911 · 2018-03-01T22:43:56Z

Made the suggested changes. As for calling async methods from sync ones, we are trying to provide a stronger guarantee here than merely providing blocking semantics. We are trying to ensure that blocking methods actually execute on the calling thread, and are guaranteed to not spawn any new threads under the hood. This means developers can easily plug the SDK into thread-constrained runtimes (GAE, other J2EE containers etc) and run everything on runtime-provisioned threads if they want to.

auth.createCustomToken(uid); // Runs on the calling thread
auth.createCustomTokenAsync(uid).get(); // Runs on SDK thread pool. Just blocks the calling thread.

avishalom

LGTM but a nit.

avishalom · 2018-03-08T02:38:29Z

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

   *
   * @param uid The UID to store in the token. This identifies the user to other Firebase services
-   *     (Realtime Database, Storage, etc.). Should be less than 128 characters.
+   *     (Firebase Realtime Database, Firebase Auth, etc.).


Any reason to drop the length qualifier? (good either way)

Added it back.

avishalom · 2018-03-08T03:29:41Z

src/main/java/com/google/firebase/messaging/FirebaseMessaging.java

-    checkTopic(topic);
-
-    return ImplFirebaseTrampolines.submitCallable(app, new Callable<TopicManagementResponse>() {
+  private CallableOperation<String, FirebaseMessagingException> makeSendRequest(


Could this be named sendOp to be in line with the rest of the CallableOperations names?

(and topicManagementOp , below)

Good idea. Changed to sendOp and manageTopicOp

avishalom · 2018-03-08T03:35:30Z

src/main/java/com/google/firebase/FirebaseApp.java

+    try {
+      return initializeApp(getOptionsFromEnvironment(), name);
+    } catch (IOException e) {
+      throw new IllegalArgumentException(


Change javadocs to IllegalArgumentException.

…onsistency

hiranya911 · 2018-03-08T19:39:25Z

DO NOT MERGE (BREAKING CHANGES)

* Removed the deprecated FirebaseCredential API * Removing the deprecated Task API

* Dropping support for GAE 7 * Removed GaeThreadFactory, GaeExecutorService and RevivingScheduledExecutor * Removed the deprecated FirebaseCredential API * Removing GAE java7 related APIs (GaeThreadFactory, RevivingScheduledExecutor) * Removed GaePlatform implementation * Added FirebaseScheduledExecutor * Updated documentation * Some minor nits from code reviews * Calling super method in DefaultRunLoop executor

* Dropping support for GAE 7 * Removed GaeThreadFactory, GaeExecutorService and RevivingScheduledExecutor * Removed the deprecated FirebaseCredential API * Removing GAE java7 related APIs (GaeThreadFactory, RevivingScheduledExecutor) * Removed GaePlatform implementation * Added FirebaseScheduledExecutor * Updated documentation * Removing LogWrapper API * Removing PrefixedLogger * Removing test config file * Updated CHANGELOG * Minor clean ups pointed out in the code review

dimipaun · 2018-05-10T04:06:35Z

Hi @hiranya911, as I commented already, removing the Task based API precludes us from sharing code between Android and the Server. This is a non-trivial problem, and it completely breaks a lot of our testing harness that shares code from Android.

hiranya911 · 2018-05-10T05:53:31Z

@dimipaun This is not a use case we intend to support. FWIW, we did not intend to support it in the past either. It sort of worked that way due to the similarity in the API signatures. But we certainly don't want developers to rely on that behavior. We want the Admin SDK, and the Android SDK to evolve independently. Specifically, we want the Admin SDK to follow the conventions established by the other server-side gcloud libraries. Therefore supporting an adhoc Task API is both awkward and expensive for us.

To be honest I'm not sure how you even got the same code to work in these 2 environments. The Task interface in Android and Admin SDK belong to different packages. So at least their import statements should be different. They are really 2 different classes, that happened to have the same short name, and the same public API surface. That is not really code sharing. It is relying on the syntactic similarity between two different APIs, which is always risky.

You may have noticed that this change is now in effect in the 6.0.0 release. You can continue to use the 5.x.x versions of the SDK while you figure out a migration plan. I would highly encourage you to migrate your server-side components to use the ApiFuture interface. If syntactic similarity with Task is critical, you can try implementing your own Task wrapper around the futures (you can even try to implement the actual Task interface from Android gms, and attempt actual code sharing with compiler support).

hiranya911 added 3 commits February 21, 2018 17:28

Adding the sync APIs for FirebaseAuth

fd3dd9d

Added more tests; Added sync APIs for FirebaseMessaging

ac3453b

Removing Task references from database, iid and fcm APIs

ded167e

hiranya911 requested review from avishalom and schmidt-sebastian February 26, 2018 21:07

hiranya911 assigned schmidt-sebastian Feb 26, 2018

Fixing a typo

765f209

schmidt-sebastian reviewed Feb 28, 2018

View reviewed changes

Minor code clean up

01ce79e

Merged with master (auth unit test improvements)

3913688

avishalom approved these changes Mar 8, 2018

View reviewed changes

hiranya911 assigned hiranya911 and unassigned schmidt-sebastian Mar 8, 2018

Updated javadocs; Renamed internal helpers of FirebaseMessaging for c…

d5ec1c3

…onsistency

hiranya911 added 9 commits March 9, 2018 09:32

Removed the deprecated FirebaseCredential API (#149)

f8f33b4

Removing the Task API (#152)

27eddb6

* Removed the deprecated FirebaseCredential API * Removing the deprecated Task API

Merged with master

3979c02

merged with master

ef036bb

Merge branch 'master' into v6

5ede505

Merged with master

dbc1492

Minor API doc fixes (#171)

e0e2ecf

hiranya911 merged commit 93d0810 into master May 7, 2018

hiranya911 deleted the v6 branch May 8, 2018 20:04

Introducing Synchronous Methods to Public APIs #146

Introducing Synchronous Methods to Public APIs #146

Uh oh!

Conversation

hiranya911 commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schmidt-sebastian 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!

hiranya911 commented Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avishalom 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!

avishalom Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avishalom Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avishalom Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hiranya911 commented Mar 8, 2018

DO NOT MERGE (BREAKING CHANGES)

Uh oh!

dimipaun commented May 10, 2018

Uh oh!

hiranya911 commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hiranya911 commented Feb 26, 2018 •

edited

Loading

hiranya911 commented Mar 1, 2018 •

edited

Loading

avishalom Mar 8, 2018 •

edited

Loading

avishalom Mar 8, 2018 •

edited

Loading

avishalom Mar 8, 2018 •

edited

Loading