Review

Author: schmidt-sebastian

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

@@ -15,7 +15,7 @@
 package com.google.firebase.firestore.local;
 
 import static com.google.firebase.firestore.util.Assert.hardAssert;
-import static com.google.firebase.firestore.util.Util.advanceIterator;
+import static com.google.firebase.firestore.util.Util.diffCollections;
 import static java.util.Arrays.asList;
 
 import android.util.SparseArray;
@@ -51,10 +51,8 @@
 import com.google.protobuf.ByteString;
 import java.util.ArrayList;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -791,44 +789,12 @@ public void configureFieldIndexes(List<FieldIndex> newFieldIndexes) {
     persistence.runTransaction(
         "Configure indexes",
         () -> {
-          List<FieldIndex> existingIndexes = new ArrayList<>(indexManager.getFieldIndexes());
-          Collections.sort(existingIndexes, FieldIndex.SEMANTIC_COMPARATOR);
-          Iterator<FieldIndex> existingIt = existingIndexes.iterator();
-          @Nullable FieldIndex existingValue = advanceIterator(existingIt);
-
-          List<FieldIndex> updatedIndexes = new ArrayList<>(newFieldIndexes);
-          Collections.sort(updatedIndexes, FieldIndex.SEMANTIC_COMPARATOR);
-          Iterator<FieldIndex> updatedIt = updatedIndexes.iterator();
-          @Nullable FieldIndex updatedValue = advanceIterator(updatedIt);
-
-          while (existingValue != null || updatedValue != null) {
-            boolean deleted = false;
-            boolean updated = false;
-
-            if (existingValue != null && updatedValue != null) {
-              int cmp = FieldIndex.SEMANTIC_COMPARATOR.compare(existingValue, updatedValue);
-              if (cmp < 0) {
-                deleted = true;
-              } else if (cmp > 0) {
-                updated = true;
-              }
-            } else if (existingValue != null) {
-              deleted = true;
-            } else {
-              updated = true;
-            }
-
-            if (deleted) {
-              indexManager.deleteFieldIndex(existingValue);
-              existingValue = advanceIterator(existingIt);
-            } else if (updated) {
-              indexManager.addFieldIndex(updatedValue);
-              updatedValue = advanceIterator(updatedIt);
-            } else {
-              existingValue = advanceIterator(existingIt);
-              updatedValue = advanceIterator(updatedIt);
-            }
-          }
+          diffCollections(
+              indexManager.getFieldIndexes(),
+              newFieldIndexes,
+              FieldIndex.SEMANTIC_COMPARATOR,
+              indexManager::addFieldIndex,
+              indexManager::deleteFieldIndex);
         });
   }
 

firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java

@@ -28,6 +28,7 @@
 import io.grpc.StatusRuntimeException;
 import java.security.SecureRandom;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.Iterator;
@@ -98,18 +99,8 @@ public static int compareMixed(double doubleValue, long longValue) {
     return NumberComparisonHelper.firestoreCompareDoubleWithLong(doubleValue, longValue);
   }
 
-  @SuppressWarnings("unchecked")
-  private static final Comparator COMPARABLE_COMPARATOR =
-      new Comparator<Comparable<?>>() {
-        @Override
-        public int compare(Comparable left, Comparable right) {
-          return left.compareTo(right);
-        }
-      };
-
-  @SuppressWarnings("unchecked")
   public static <T extends Comparable<T>> Comparator<T> comparator() {
-    return COMPARABLE_COMPARATOR;
+    return Comparable::compareTo;
   }
 
   public static FirebaseFirestoreException exceptionFromStatus(Status error) {
@@ -249,9 +240,94 @@ public static StringBuilder repeatSequence(
     return sb;
   }
 
+  /**
+   * Compares two collections for equality using the provided comparator. The method computes the
+   * intersection and invokes `onAdd` for every element that is in `after` but not `before`.
+   * `onRemove` is invoked for every element in `before` but missing from `after`.
+   *
+   * <p>The method creates a copy of both `before` and `after` and runs in O(n log n), where n is
+   * the size of the two lists.
+   *
+   * @param before The elements that exist in the original set.
+   * @param after The elements to diff against the original set.
+   * @param comparator The comparator to use to define equality between elements.
+   * @param onAdd A function to invoke for every element that is part of `after` but not `before`.
+   * @param onRemove A function to invoke for every element that is part of `before` but not
+   *     `after`.
+   */
+  public static <T> void diffCollections(
+      Collection<T> before,
+      Collection<T> after,
+      Comparator<T> comparator,
+      Consumer<T> onAdd,
+      Consumer<T> onRemove) {
+    List<T> beforeEntries = new ArrayList<>(before);
+    Collections.sort(beforeEntries, comparator);
+    Iterator<T> beforeIt = beforeEntries.iterator();
+    @Nullable T beforeValue = advanceIterator(beforeIt);
+
+    List<T> afterEntries = new ArrayList<>(after);
+    Collections.sort(afterEntries, comparator);
+    Iterator<T> afterIt = afterEntries.iterator();
+    @Nullable T afterValue = advanceIterator(afterIt);
+
+    // Walk through the two sets at the same time, using the ordering defined by `comparator`.
+    while (beforeValue != null || afterValue != null) {
+      boolean added = false;
+      boolean removed = false;
+
+      if (beforeValue != null && afterValue != null) {
+        int cmp = comparator.compare(beforeValue, afterValue);
+        if (cmp < 0) {
+          // The element was removed if the next element in our ordered walkthrough is only in
+          // `before`.
+          removed = true;
+        } else if (cmp > 0) {
+          // The element was added if the next element in our ordered walkthrough is only in
+          // `after`.
+          added = true;
+        }
+      } else if (beforeValue != null) {
+        removed = true;
+      } else {
+        added = true;
+      }
+
+      if (added) {
+        onAdd.accept(afterValue);
+        afterValue = advanceIterator(afterIt);
+      } else if (removed) {
+        onRemove.accept(beforeValue);
+        beforeValue = advanceIterator(beforeIt);
+      } else {
+        beforeValue = advanceIterator(beforeIt);
+        afterValue = advanceIterator(afterIt);
+      }
+    }
+  }
+
+  /**
+   * Compares two collections for equality using their natural ordering. The method computes the
+   * intersection and invokes `onAdd` for every element that is in `after` but not `before`.
+   * `onRemove` is invoked for every element in `before` but missing from `after`.
+   *
+   * <p>The method creates a copy of both `before` and `after` and runs in O(n log n), where n is
+   * the size of the two lists.
+   *
+   * @param before The elements that exist in the original set.
+   * @param after The elements to diff against the original set.
+   * @param onAdd A function to invoke for every element that is part of `after` but not `before`.
+   * @param onRemove A function to invoke for every element that is part of `before` but not
+   *     `after`.
+   */
+  public static <T extends Comparable<T>> void diffCollections(
+      Collection<T> before, Collection<T> after, Consumer<T> onAdd, Consumer<T> onRemove) {
+    diffCollections(before, after, Comparable::compareTo, onAdd, onRemove);
+  }
+
   /** Returns the next element from the iterator or `null` if none available. */
   @Nullable
-  public static <T> T advanceIterator(Iterator<T> it) {
+  private static <T> T advanceIterator(Iterator<T> it) {
     return it.hasNext() ? it.next() : null;
   }
 }

firebase-firestore/src/test/java/com/google/firebase/firestore/util/UtilTest.java

@@ -14,10 +14,15 @@
 
 package com.google.firebase.firestore.util;
 
+import static com.google.common.truth.Truth.assertThat;
 import static org.junit.Assert.assertEquals;
 
 import com.google.firebase.firestore.testutil.TestUtil;
 import com.google.protobuf.ByteString;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -35,4 +40,38 @@ public void testToDebugString() {
         Util.toDebugString(
             TestUtil.byteString(0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 0xA, 0xB, 0xC, 0xD, 0xE, 0xF)));
   }
+
+  @Test
+  public void testDiffCollectionsWithMissingElement() {
+    List<String> before = Arrays.asList("a", "b", "c");
+    List<String> after = Arrays.asList("a", "b");
+    validateDiffCollection(before, after);
+  }
+
+  @Test
+  public void testDiffCollectionsWithAddedElement() {
+    List<String> before = Arrays.asList("a", "b");
+    List<String> after = Arrays.asList("a", "b", "c");
+    validateDiffCollection(before, after);
+  }
+
+  @Test
+  public void testDiffCollectionsWithoutOrdering() {
+    List<String> before = Arrays.asList("b", "a");
+    List<String> after = Arrays.asList("a", "b");
+    validateDiffCollection(before, after);
+  }
+
+  @Test
+  public void testDiffCollectionsWithEmptyLists() {
+    validateDiffCollection(Collections.singletonList("a"), Collections.emptyList());
+    validateDiffCollection(Collections.emptyList(), Collections.singletonList("a"));
+    validateDiffCollection(Collections.emptyList(), Collections.emptyList());
+  }
+
+  private void validateDiffCollection(List<String> before, List<String> after) {
+    List<String> result = new ArrayList<>(before);
+    Util.diffCollections(before, after, result::add, result::remove);
+    assertThat(result).containsExactlyElementsIn(after);
+  }
 }