Merge

Author: schmidt-sebastian

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/CheckForNewReleaseClient.java

@@ -37,7 +37,6 @@
 import java.util.concurrent.Executors;
 
 class CheckForNewReleaseClient {
-  private static final int NEW_RELEASE_THREAD_POOL_SIZE = 4;
   private static final String TAG = "CheckForNewReleaseClient:";
 
   private final FirebaseApp firebaseApp;
@@ -56,8 +55,7 @@ class CheckForNewReleaseClient {
     this.firebaseApp = firebaseApp;
     this.firebaseAppDistributionTesterApiClient = firebaseAppDistributionTesterApiClient;
     this.firebaseInstallationsApi = firebaseInstallationsApi;
-    // TODO: verify if this is best way to use executorservice here
-    this.checkForNewReleaseExecutor = Executors.newFixedThreadPool(NEW_RELEASE_THREAD_POOL_SIZE);
+    this.checkForNewReleaseExecutor = Executors.newSingleThreadExecutor();
     this.releaseIdentifierStorage =
         new ReleaseIdentifierStorage(firebaseApp.getApplicationContext());
   }

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/FirebaseAppDistribution.java

@@ -23,6 +23,7 @@
 import android.content.Context;
 import androidx.annotation.GuardedBy;
 import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 import com.google.android.gms.common.internal.Preconditions;
 import com.google.android.gms.tasks.Task;
@@ -35,24 +36,28 @@
 import com.google.firebase.installations.FirebaseInstallationsApi;
 
 public class FirebaseAppDistribution {
+  private static final int UNKNOWN_RELEASE_FILE_SIZE = -1;
 
   private final FirebaseApp firebaseApp;
   private final TesterSignInClient testerSignInClient;
   private final CheckForNewReleaseClient checkForNewReleaseClient;
   private final UpdateAppClient updateAppClient;
   private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
-  private static final int UNKNOWN_RELEASE_FILE_SIZE = -1;
+  private final SignInStorage signInStorage;
 
-  @GuardedBy("updateTaskLock")
+  private final Object updateIfNewReleaseTaskLock = new Object();
+
+  @GuardedBy("updateIfNewReleaseTaskLock")
   private UpdateTaskImpl cachedUpdateIfNewReleaseTask;
 
-  private final Object updateTaskLock = new Object();
-  private Task<AppDistributionRelease> cachedCheckForNewReleaseTask;
+  private final Object cachedNewReleaseLock = new Object();
 
+  @GuardedBy("cachedNewReleaseLock")
   private AppDistributionReleaseInternal cachedNewRelease;
+
+  private Task<AppDistributionRelease> cachedCheckForNewReleaseTask;
   private AlertDialog updateDialog;
   private boolean updateDialogShown;
-  private final SignInStorage signInStorage;
 
   /** Constructor for FirebaseAppDistribution */
   @VisibleForTesting
@@ -126,8 +131,8 @@ public static FirebaseAppDistribution getInstance(@NonNull FirebaseApp app) {
    * to complete the download and installation.
    */
   @NonNull
-  public synchronized UpdateTask updateIfNewReleaseAvailable() {
-    synchronized (updateTaskLock) {
+  public UpdateTask updateIfNewReleaseAvailable() {
+    synchronized (updateIfNewReleaseTaskLock) {
       if (cachedUpdateIfNewReleaseTask != null && !cachedUpdateIfNewReleaseTask.isComplete()) {
         return cachedUpdateIfNewReleaseTask;
       }
@@ -163,7 +168,8 @@ public synchronized UpdateTask updateIfNewReleaseAvailable() {
                       Constants.ErrorMessages.NETWORK_ERROR,
                       FirebaseAppDistributionException.Status.NETWORK_FAILURE));
             });
-    synchronized (updateTaskLock) {
+
+    synchronized (updateIfNewReleaseTaskLock) {
       return cachedUpdateIfNewReleaseTask;
     }
   }
@@ -218,15 +224,15 @@ public synchronized Task<AppDistributionRelease> checkForNewRelease() {
    * new release is cached from checkForNewRelease
    */
   @NonNull
-  public synchronized UpdateTask updateApp() {
+  public UpdateTask updateApp() {
     return updateApp(false);
   }
 
   /**
    * Overloaded updateApp with boolean input showDownloadInNotificationsManager. Set to true for
    * basic configuration and false for advanced configuration.
    */
-  private synchronized UpdateTask updateApp(boolean showDownloadInNotificationManager) {
+  private UpdateTask updateApp(boolean showDownloadInNotificationManager) {
     if (!isTesterSignedIn()) {
       UpdateTaskImpl updateTask = new UpdateTaskImpl();
       updateTask.setException(
@@ -235,7 +241,9 @@ private synchronized UpdateTask updateApp(boolean showDownloadInNotificationMana
       return updateTask;
     }
 
-    return this.updateAppClient.updateApp(cachedNewRelease, showDownloadInNotificationManager);
+    synchronized (cachedNewReleaseLock) {
+      return this.updateAppClient.updateApp(cachedNewRelease, showDownloadInNotificationManager);
+    }
   }
 
   /** Returns true if the App Distribution tester is signed in */
@@ -245,7 +253,7 @@ public boolean isTesterSignedIn() {
 
   /** Signs out the App Distribution tester */
   public void signOutTester() {
-    this.cachedNewRelease = null;
+    setCachedNewRelease(null);
     this.signInStorage.setSignInStatus(false);
   }
 
@@ -263,13 +271,17 @@ void onActivityDestroyed(@NonNull Activity activity) {
   }
 
   @VisibleForTesting
-  void setCachedNewRelease(AppDistributionReleaseInternal newRelease) {
-    this.cachedNewRelease = newRelease;
+  void setCachedNewRelease(@Nullable AppDistributionReleaseInternal newRelease) {
+    synchronized (cachedNewReleaseLock) {
+      this.cachedNewRelease = newRelease;
+    }
   }
 
   @VisibleForTesting
   AppDistributionReleaseInternal getCachedNewRelease() {
-    return this.cachedNewRelease;
+    synchronized (cachedNewReleaseLock) {
+      return this.cachedNewRelease;
+    }
   }
 
   private UpdateTaskImpl showUpdateAlertDialog(AppDistributionRelease newRelease) {
@@ -302,7 +314,7 @@ private UpdateTaskImpl showUpdateAlertDialog(AppDistributionRelease newRelease)
         AlertDialog.BUTTON_POSITIVE,
         context.getString(R.string.update_yes_button),
         (dialogInterface, i) -> {
-          synchronized (updateTaskLock) {
+          synchronized (updateIfNewReleaseTaskLock) {
             // show download progress in notification manager
             updateApp(true)
                 .addOnProgressListener(this::postProgressToCachedUpdateIfNewReleaseTask)
@@ -316,7 +328,7 @@ private UpdateTaskImpl showUpdateAlertDialog(AppDistributionRelease newRelease)
         context.getString(R.string.update_no_button),
         (dialogInterface, i) -> {
           dialogInterface.dismiss();
-          synchronized (updateTaskLock) {
+          synchronized (updateIfNewReleaseTaskLock) {
             postProgressToCachedUpdateIfNewReleaseTask(
                 UpdateProgress.builder()
                     .setApkFileTotalBytes(UNKNOWN_RELEASE_FILE_SIZE)
@@ -331,16 +343,16 @@ private UpdateTaskImpl showUpdateAlertDialog(AppDistributionRelease newRelease)
 
     updateDialog.show();
     updateDialogShown = true;
-    synchronized (updateTaskLock) {
+    synchronized (updateIfNewReleaseTaskLock) {
       return cachedUpdateIfNewReleaseTask;
     }
   }
 
   private void setCachedUpdateIfNewReleaseCompletionError(FirebaseAppDistributionException e) {
-    synchronized (updateTaskLock) {
+    synchronized (updateIfNewReleaseTaskLock) {
       safeSetTaskException(cachedUpdateIfNewReleaseTask, e);
-      dismissUpdateDialog();
     }
+    dismissUpdateDialog();
   }
 
   private void setCachedUpdateIfNewReleaseCompletionError(
@@ -353,16 +365,16 @@ private void setCachedUpdateIfNewReleaseCompletionError(
   }
 
   private void postProgressToCachedUpdateIfNewReleaseTask(UpdateProgress progress) {
-    synchronized (updateTaskLock) {
+    synchronized (updateIfNewReleaseTaskLock) {
       cachedUpdateIfNewReleaseTask.updateProgress(progress);
     }
   }
 
   private void setCachedUpdateIfNewReleaseResult() {
-    synchronized (updateTaskLock) {
+    synchronized (updateIfNewReleaseTaskLock) {
       safeSetTaskResult(cachedUpdateIfNewReleaseTask);
-      dismissUpdateDialog();
     }
+    dismissUpdateDialog();
   }
 
   private void dismissUpdateDialog() {

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/TesterSignInClient.java

@@ -26,6 +26,7 @@
 import android.content.Intent;
 import android.content.pm.ResolveInfo;
 import android.net.Uri;
+import androidx.annotation.GuardedBy;
 import androidx.annotation.NonNull;
 import androidx.annotation.VisibleForTesting;
 import androidx.browser.customtabs.CustomTabsIntent;
@@ -42,16 +43,19 @@
 
 class TesterSignInClient {
   private static final String TAG = "TesterSignIn:";
-
-  private TaskCompletionSource<Void> signInTaskCompletionSource = null;
-
-  private final String SIGNIN_REDIRECT_URL =
+  private static final String SIGNIN_REDIRECT_URL =
       "https://appdistribution.firebase.google.com/pub/testerapps/%s/installations/%s/buildalerts?appName=%s&packageName=%s";
+
   private final FirebaseApp firebaseApp;
   private final FirebaseInstallationsApi firebaseInstallationsApi;
   private final SignInStorage signInStorage;
   private final FirebaseAppDistributionLifecycleNotifier lifecycleNotifier;
 
+  private final Object signInTaskLock = new Object();
+
+  @GuardedBy("signInTaskLock")
+  private TaskCompletionSource<Void> signInTaskCompletionSource = null;
+
   private AlertDialog alertDialog;
 
   TesterSignInClient(
@@ -113,36 +117,42 @@ private void onActivityDestroyed(Activity activity) {
   }
 
   @NonNull
-  public synchronized Task<Void> signInTester() {
+  public Task<Void> signInTester() {
 
     if (signInStorage.getSignInStatus()) {
       LogWrapper.getInstance().v(TAG + "Tester is already signed in.");
       return Tasks.forResult(null);
     }
 
-    if (this.isCurrentlySigningIn()) {
-      LogWrapper.getInstance()
-          .v(TAG + "Detected In-Progress sign in task. Returning the same task.");
-      return signInTaskCompletionSource.getTask();
-    }
-    Activity currentActivity = lifecycleNotifier.getCurrentActivity();
-    if (currentActivity == null) {
-      LogWrapper.getInstance().e(TAG + "No foreground activity found.");
-      return Tasks.forException(
-          new FirebaseAppDistributionException(
-              ErrorMessages.APP_BACKGROUNDED,
-              FirebaseAppDistributionException.Status.UPDATE_NOT_AVAILABLE));
-    }
-    signInTaskCompletionSource = new TaskCompletionSource<>();
+    synchronized (signInTaskLock) {
+      if (this.isCurrentlySigningIn()) {
+        LogWrapper.getInstance()
+            .v(TAG + "Detected In-Progress sign in task. Returning the same task.");
+        return signInTaskCompletionSource.getTask();
+      }
+      Activity currentActivity = lifecycleNotifier.getCurrentActivity();
+      if (currentActivity == null) {
+        LogWrapper.getInstance().e(TAG + "No foreground activity found.");
+        return Tasks.forException(
+            new FirebaseAppDistributionException(
+                ErrorMessages.APP_BACKGROUNDED,
+                FirebaseAppDistributionException.Status.UPDATE_NOT_AVAILABLE));
+      }
 
-    alertDialog = getSignInAlertDialog(currentActivity);
-    alertDialog.show();
+      signInTaskCompletionSource = new TaskCompletionSource<>();
 
-    return signInTaskCompletionSource.getTask();
+      alertDialog = getSignInAlertDialog(currentActivity);
+      alertDialog.show();
+
+      return signInTaskCompletionSource.getTask();
+    }
   }
 
   private boolean isCurrentlySigningIn() {
-    return signInTaskCompletionSource != null && !signInTaskCompletionSource.getTask().isComplete();
+    synchronized (signInTaskLock) {
+      return signInTaskCompletionSource != null
+          && !signInTaskCompletionSource.getTask().isComplete();
+    }
   }
 
   private AlertDialog getSignInAlertDialog(Activity currentActivity) {
@@ -190,7 +200,9 @@ public void onClick(DialogInterface dialogInterface, int i) {
   }
 
   private void setSignInTaskCompletionError(FirebaseAppDistributionException e) {
-    safeSetTaskException(signInTaskCompletionSource, e);
+    synchronized (signInTaskLock) {
+      safeSetTaskException(signInTaskCompletionSource, e);
+    }
     dismissAlertDialog();
   }
 
@@ -201,7 +213,9 @@ private void setCanceledAuthenticationError() {
   }
 
   private void setSuccessfulSignInResult() {
-    safeSetTaskResult(signInTaskCompletionSource, null);
+    synchronized (signInTaskLock) {
+      safeSetTaskResult(signInTaskCompletionSource, null);
+    }
     dismissAlertDialog();
   }
 

firebase-app-distribution/src/main/java/com/google/firebase/app/distribution/UpdateApkClient.java

@@ -74,7 +74,7 @@ public UpdateApkClient(
     this.installApkClient = installApkClient;
   }
 
-  public synchronized UpdateTaskImpl updateApk(
+  public UpdateTaskImpl updateApk(
       @NonNull AppDistributionReleaseInternal newRelease, boolean showDownloadNotificationManager) {
     synchronized (updateTaskLock) {
       if (cachedUpdateTask != null && !cachedUpdateTask.isComplete()) {

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/IndexingTest.java

@@ -49,10 +49,10 @@ public void tearDown() {
   }
 
   @Test
-  public void testCanConfigureIndices() throws ExecutionException, InterruptedException {
+  public void testCanConfigureIndexes() throws ExecutionException, InterruptedException {
     FirebaseFirestore db = testFirestore();
     Task<Void> indexTask =
-        db.configureIndices(
+        db.setIndexConfiguration(
             "{\n"
                 + "  \"indexes\": [\n"
                 + "    {\n"
@@ -89,7 +89,7 @@ public void testCanConfigureIndices() throws ExecutionException, InterruptedExce
   public void testBadJsonDoesNotCrashClient() {
     FirebaseFirestore db = testFirestore();
 
-    Assert.assertThrows(IllegalArgumentException.class, () -> db.configureIndices("{,"));
+    Assert.assertThrows(IllegalArgumentException.class, () -> db.setIndexConfiguration("{,"));
   }
 
   @Test
@@ -98,7 +98,7 @@ public void testBadIndexDoesNotCrashClient() {
     Assert.assertThrows(
         IllegalArgumentException.class,
         () ->
-            db.configureIndices(
+            db.setIndexConfiguration(
                 "{\n"
                     + "  \"indexes\": [\n"
                     + "    {\n"

firebase-firestore/src/main/java/com/google/firebase/firestore/FirebaseFirestore.java

@@ -297,13 +297,14 @@ public FirebaseApp getApp() {
    * automatically start using the index once the index entries have been written.
    *
    * <p>The method accepts the JSON format exported by the Firebase CLI (`firebase
-   * firestore:indexes`). If the JSON format is invalid, this method rejects the returned task.
+   * firestore:indexes`). If the JSON format is invalid, this method throws an exception.
    *
    * @param json The JSON format exported by the Firebase CLI.
    * @return A task that resolves once all indices are successfully configured.
+   * @throws IllegalArgumentException if the JSON format is invalid
    */
   @VisibleForTesting
-  Task<Void> configureIndices(String json) {
+  Task<Void> setIndexConfiguration(String json) {
     ensureClientConfigured();
 
     // Preconditions.checkState(BuildConfig.ENABLE_INDEXING, "Indexing support is not yet

firebase-firestore/src/main/java/com/google/firebase/firestore/local/DefaultQueryEngine.java

@@ -21,6 +21,7 @@
 import com.google.firebase.firestore.core.Query;
 import com.google.firebase.firestore.model.Document;
 import com.google.firebase.firestore.model.DocumentKey;
+import com.google.firebase.firestore.model.FieldIndex.IndexOffset;
 import com.google.firebase.firestore.model.SnapshotVersion;
 import com.google.firebase.firestore.util.Logger;
 import java.util.Collections;
@@ -98,7 +99,8 @@ && needsRefill(
     // Retrieve all results for documents that were updated since the last limbo-document free
     // remote snapshot.
     ImmutableSortedMap<DocumentKey, Document> updatedResults =
-        localDocumentsView.getDocumentsMatchingQuery(query, lastLimboFreeSnapshotVersion);
+        localDocumentsView.getDocumentsMatchingQuery(
+            query, IndexOffset.create(lastLimboFreeSnapshotVersion));
 
     // We merge `previousResults` into `updateResults`, since `updateResults` is already a
     // ImmutableSortedMap. If a document is contained in both lists, then its contents are the same.
@@ -168,6 +170,6 @@ private ImmutableSortedMap<DocumentKey, Document> executeFullCollectionScan(Quer
     if (Logger.isDebugEnabled()) {
       Logger.debug(LOG_TAG, "Using full collection scan to execute query: %s", query.toString());
     }
-    return localDocumentsView.getDocumentsMatchingQuery(query, SnapshotVersion.NONE);
+    return localDocumentsView.getDocumentsMatchingQuery(query, IndexOffset.NONE);
   }
 }