resolve comments

Author: milaGGL

firebase-firestore/src/main/java/com/google/firebase/firestore/model/DatabaseId.java

@@ -60,10 +60,6 @@ public String getDatabaseId() {
     return databaseId;
   }
 
-  public String canonicalString() {
-    return "projects/" + projectId + "/databases/" + databaseId;
-  }
-
   @Override
   public String toString() {
     return "DatabaseId(" + projectId + ", " + databaseId + ")";

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/BloomFilter.java

@@ -122,8 +122,10 @@ private static long getLongLittleEndian(@NonNull byte[] bytes, int offset) {
    */
   private int getBitIndex(long hash1, long hash2, int hashIndex) {
     // Calculate hashed value h(i) = h1 + (i * h2).
-    // Even though we are interpreting hash1 and hash2 as unsigned, the addition and multiplication
-    // operators still perform the correct operation and give the desired overflow behavior.
+    // Even though we are interpreting hash1 and hash2 as unsigned, the addition and
+    // multiplication
+    // operators still perform the correct operation and give the desired overflow
+    // behavior.
     long combinedHash = hash1 + (hash2 * hashIndex);
     long modulo = unsignedRemainder(combinedHash, this.bitCount);
     return (int) modulo;
@@ -132,8 +134,8 @@ private int getBitIndex(long hash1, long hash2, int hashIndex) {
   /**
    * Calculate modulo, where the dividend and divisor are treated as unsigned 64-bit longs.
    *
-   * <p>The implementation is taken from <a
-   * href="https://github.com/google/guava/blob/553037486901cc60820ab7dcb38a25b6f34eba43/android/guava/src/com/google/common/primitives/UnsignedLongs.java">Guava</a>,
+   * <p>The implementation is taken from <a href=
+   * "https://github.com/google/guava/blob/553037486901cc60820ab7dcb38a25b6f34eba43/android/guava/src/com/google/common/primitives/UnsignedLongs.java">Guava</a>,
    * simplified to our needs.
    *
    * <p>

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/BloomFilterException.java

@@ -16,7 +16,7 @@
 
 import androidx.annotation.NonNull;
 
-public class BloomFilterException extends Exception {
+public class BloomFilterException extends RuntimeException {
   public BloomFilterException(@NonNull String detailMessage) {
     super(detailMessage);
   }

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/ExistenceFilter.java

@@ -26,7 +26,7 @@ public ExistenceFilter(int count) {
     this.count = count;
   }
 
-  public ExistenceFilter(int count, BloomFilter unchangedNames) {
+  public ExistenceFilter(int count, @Nullable BloomFilter unchangedNames) {
     this.count = count;
     this.unchangedNames = unchangedNames;
   }

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

@@ -945,7 +945,6 @@ public WatchChange decodeWatchChange(ListenResponse protoChange) {
         break;
       case FILTER:
         com.google.firestore.v1.ExistenceFilter protoFilter = protoChange.getFilter();
-        // TODO: implement existence filter parsing (see b/33076578)
         ExistenceFilter filter =
             new ExistenceFilter(protoFilter.getCount(), protoFilter.getUnchangedNames());
         int targetId = protoFilter.getTargetId();

firebase-firestore/src/main/java/com/google/firebase/firestore/remote/WatchChangeAggregator.java

@@ -235,12 +235,9 @@ private boolean applyBloomFilter(ExistenceFilterWatchChange watchChange, int cur
       bloomFilter =
           new BloomFilter(
               bitmap, unchangedNames.getBits().getPadding(), unchangedNames.getHashCount());
-    } catch (Exception e) {
+    } catch (BloomFilterException e) {
       if (e instanceof BloomFilterException) {
         Logger.warn("Firestore", "BloomFilter error: %s", e);
-
-      } else {
-        Logger.warn("Firestore", "Applying bloom filter failed: %s", e);
       }
       return false;
     }
@@ -259,8 +256,12 @@ private int filterRemovedDocuments(BloomFilter bloomFilter, int targetId) {
         targetMetadataProvider.getRemoteKeysForTarget(targetId);
     int removalCount = 0;
     for (DocumentKey key : existingKeys) {
+      DatabaseId databaseId = targetMetadataProvider.getDatabaseId();
       String documentPath =
-          targetMetadataProvider.getDatabaseId().canonicalString()
+          "projects/"
+              + databaseId.getProjectId()
+              + "/databases/"
+              + databaseId.getDatabaseId()
               + "/documents/"
               + key.getPath().canonicalString();
       if (!bloomFilter.mightContain(documentPath)) {

firebase-firestore/src/test/java/com/google/firebase/firestore/remote/BloomFilterTest.java

@@ -41,13 +41,13 @@ public class BloomFilterTest {
       "src/test/resources/bloom_filter_golden_test_data/";
 
   @Test
-  public void instantiateEmptyBloomFilter() throws BloomFilterException {
+  public void instantiateEmptyBloomFilter() {
     BloomFilter bloomFilter = new BloomFilter(new byte[0], 0, 0);
     assertEquals(bloomFilter.getBitCount(), 0);
   }
 
   @Test
-  public void instantiateNonEmptyBloomFilter() throws BloomFilterException {
+  public void instantiateNonEmptyBloomFilter() {
     {
       BloomFilter bloomFilter1 = new BloomFilter(new byte[] {1}, 0, 1);
       assertEquals(bloomFilter1.getBitCount(), 8);
@@ -124,23 +124,22 @@ public void constructorShouldThrowBFEIfPaddingIsTooLarge() {
   }
 
   @Test
-  public void mightContainCanProcessNonStandardCharacters() throws BloomFilterException {
+  public void mightContainCanProcessNonStandardCharacters() {
     // A non-empty BloomFilter object with 1 insertion : "ÀÒ∑"
     BloomFilter bloomFilter = new BloomFilter(new byte[] {(byte) 237, 5}, 5, 8);
     assertTrue(bloomFilter.mightContain("ÀÒ∑"));
     assertFalse(bloomFilter.mightContain("Ò∑À"));
   }
 
   @Test
-  public void mightContainOnEmptyBloomFilterShouldReturnFalse() throws BloomFilterException {
+  public void mightContainOnEmptyBloomFilterShouldReturnFalse() {
     BloomFilter bloomFilter = new BloomFilter(new byte[0], 0, 0);
     assertFalse(bloomFilter.mightContain(""));
     assertFalse(bloomFilter.mightContain("a"));
   }
 
   @Test
-  public void mightContainWithEmptyStringMightReturnFalsePositiveResult()
-      throws BloomFilterException {
+  public void mightContainWithEmptyStringMightReturnFalsePositiveResult() {
     {
       BloomFilter bloomFilter1 = new BloomFilter(new byte[] {1}, 1, 1);
       assertFalse(bloomFilter1.mightContain(""));
@@ -152,7 +151,7 @@ public void mightContainWithEmptyStringMightReturnFalsePositiveResult()
   }
 
   @Test
-  public void bloomFilterToString() throws BloomFilterException {
+  public void bloomFilterToString() {
     {
       BloomFilter emptyBloomFilter = new BloomFilter(new byte[0], 0, 0);
       assertEquals(emptyBloomFilter.toString(), "BloomFilter{hashCount=0, size=0, bitmap=\"\"}");

firebase-firestore/src/test/java/com/google/firebase/firestore/spec/SpecTestCase.java

@@ -143,18 +143,27 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
   // this tag and they'll all be run (but all others won't).
   private static final String EXCLUSIVE_TAG = "exclusive";
 
-  // The name of a Java system property ({@link System#getProperty(String)}) whose value is a filter
-  // that specifies which tests to execute. The value of this property is a regular expression that
-  // is matched against the name of each test. Using this property is an alternative to setting the
-  // {@link #EXCLUSIVE_TAG} tag, which requires modifying the JSON file. To use this property,
-  // specify -DspecTestFilter=<Regex> to the Java runtime, replacing <Regex> with a regular
-  // expression; a test will be executed if and only if its name matches this regular expression.
-  // In this context, a test's "name" is the result of appending its "itName" to its "describeName",
+  // The name of a Java system property ({@link System#getProperty(String)}) whose
+  // value is a filter
+  // that specifies which tests to execute. The value of this property is a
+  // regular expression that
+  // is matched against the name of each test. Using this property is an
+  // alternative to setting the
+  // {@link #EXCLUSIVE_TAG} tag, which requires modifying the JSON file. To use
+  // this property,
+  // specify -DspecTestFilter=<Regex> to the Java runtime, replacing <Regex> with
+  // a regular
+  // expression; a test will be executed if and only if its name matches this
+  // regular expression.
+  // In this context, a test's "name" is the result of appending its "itName" to
+  // its "describeName",
   // separated by a space character.
   private static final String TEST_FILTER_PROPERTY = "specTestFilter";
 
-  // Tags on tests that should be excluded from execution, useful to allow the platforms to
-  // temporarily diverge or for features that are designed to be platform specific (such as
+  // Tags on tests that should be excluded from execution, useful to allow the
+  // platforms to
+  // temporarily diverge or for features that are designed to be platform specific
+  // (such as
   // 'multi-client').
   private static final Set<String> DISABLED_TAGS =
       RUN_BENCHMARK_TESTS
@@ -233,7 +242,8 @@ public abstract class SpecTestCase implements RemoteStoreCallback {
 
   public static void info(String line) {
     if (DEBUG) {
-      // Print log information out directly to cut down on logger-related cruft like the extra
+      // Print log information out directly to cut down on logger-related cruft like
+      // the extra
       // line for the date and class method which are always SpecTestCase+info
       System.err.println(line);
     } else {
@@ -632,7 +642,8 @@ private void doWatchRemove(JSONObject watchRemoveSpec) throws Exception {
         new WatchTargetChange(
             WatchTargetChangeType.Removed, targetIds, WatchStream.EMPTY_RESUME_TOKEN, error);
     writeWatchChange(change, SnapshotVersion.NONE);
-    // Unlike web, the MockDatastore detects a watch removal with cause and will remove active
+    // Unlike web, the MockDatastore detects a watch removal with cause and will
+    // remove active
     // targets
   }
 
@@ -675,14 +686,12 @@ private void doWatchEntity(JSONObject watchEntity) throws Exception {
   }
 
   private void doWatchFilter(JSONObject watchFilter) throws Exception {
-    List<Integer> targets =
-        watchFilter.has("targetIds")
-            ? parseIntList(watchFilter.getJSONArray("targetIds"))
-            : Collections.emptyList();
+    List<Integer> targets = parseIntList(watchFilter.getJSONArray("targetIds"));
+
     Assert.hardAssert(
         targets.size() == 1, "ExistenceFilters currently support exactly one target only.");
 
-    int keyCount = watchFilter.has("keys") ? watchFilter.getJSONArray("keys").length() : 0;
+    int keyCount = watchFilter.getJSONArray("keys").length();
     BloomFilter bloomFilterProto =
         watchFilter.has("bloomFilter")
             ? parseBloomFilter(watchFilter.getJSONObject("bloomFilter"))
@@ -701,7 +710,8 @@ private void doWatchReset(JSONArray targetIds) throws Exception {
   }
 
   private void doWatchSnapshot(JSONObject watchSnapshot) throws Exception {
-    // The client will only respond to watchSnapshots if they are on a target change with an empty
+    // The client will only respond to watchSnapshots if they are on a target change
+    // with an empty
     // set of target IDs.
     List<Integer> targets =
         watchSnapshot.has("targetIds")
@@ -724,7 +734,8 @@ private void doWatchStreamClose(JSONObject spec) throws Exception {
     Status status =
         Status.fromCodeValue(error.getInt("code")).withDescription(error.getString("message"));
     queue.runSync(() -> datastore.failWatchStream(status));
-    // Unlike web, stream should re-open synchronously (if we have active listeners).
+    // Unlike web, stream should re-open synchronously (if we have active
+    // listeners).
     if (!this.queryListeners.isEmpty()) {
       assertTrue("Watch stream is open", datastore.isWatchStreamOpen());
     }
@@ -740,7 +751,7 @@ private void doWriteAck(JSONObject writeAckSpec) throws Exception {
     validateNextWriteSent(write.first);
 
     MutationResult mutationResult =
-        new MutationResult(version(version), /*transformResults=*/ Collections.emptyList());
+        new MutationResult(version(version), /* transformResults= */ Collections.emptyList());
     queue.runSync(() -> datastore.ackWrite(version(version), singletonList(mutationResult)));
   }
 
@@ -865,7 +876,8 @@ private void doStep(JSONObject step) throws Exception {
     } else if (step.has("watchStreamClose")) {
       doWatchStreamClose(step.getJSONObject("watchStreamClose"));
     } else if (step.has("watchProto")) {
-      // watchProto isn't yet used, and it's unclear how to create arbitrary protos from JSON.
+      // watchProto isn't yet used, and it's unclear how to create arbitrary protos
+      // from JSON.
       throw Assert.fail("watchProto is not yet supported.");
     } else if (step.has("writeAck")) {
       doWriteAck(step.getJSONObject("writeAck"));
@@ -882,9 +894,12 @@ private void doStep(JSONObject step) throws Exception {
         doDisableNetwork();
       }
     } else if (step.has("changeUser")) {
-      // NOTE: JSONObject.getString("foo") where "foo" is mapped to null will return "null".
-      // Explicitly testing for isNull here allows the null value to be preserved. This is important
-      // because the unauthenticated user is represented as having a null uid as a value for
+      // NOTE: JSONObject.getString("foo") where "foo" is mapped to null will return
+      // "null".
+      // Explicitly testing for isNull here allows the null value to be preserved.
+      // This is important
+      // because the unauthenticated user is represented as having a null uid as a
+      // value for
       // "changeUser".
       String uid = step.isNull("changeUser") ? null : step.getString("changeUser");
       doChangeUser(uid);
@@ -1027,8 +1042,10 @@ private void validateExpectedState(@Nullable JSONObject expectedState) throws JS
           expectedActiveTargets.put(targetId, new ArrayList<>());
           for (int i = 0; i < queryArrayJson.length(); i++) {
             Query query = parseQuery(queryArrayJson.getJSONObject(i));
-            // TODO: populate the purpose of the target once it's possible to encode that in the
-            // spec tests. For now, hard-code that it's a listen despite the fact that it's not
+            // TODO: populate the purpose of the target once it's possible to encode that in
+            // the
+            // spec tests. For now, hard-code that it's a listen despite the fact that it's
+            // not
             // always the right value.
             TargetData targetData =
                 new TargetData(
@@ -1058,7 +1075,8 @@ private void validateExpectedState(@Nullable JSONObject expectedState) throws JS
     // Always validate that the expected limbo docs match the actual limbo docs.
     validateActiveLimboDocs();
     validateEnqueuedLimboDocs();
-    // Always validate that the expected active targets match the actual active targets.
+    // Always validate that the expected active targets match the actual active
+    // targets.
     validateActiveTargets();
   }
 
@@ -1097,7 +1115,8 @@ private void validateUserCallbacks(@Nullable JSONObject expected) throws JSONExc
   }
 
   private void validateActiveLimboDocs() {
-    // Make a copy so it can modified while checking against the expected limbo docs.
+    // Make a copy so it can modified while checking against the expected limbo
+    // docs.
     @SuppressWarnings("VisibleForTests")
     Map<DocumentKey, Integer> actualLimboDocs =
         new HashMap<>(syncEngine.getActiveLimboDocumentResolutions());
@@ -1174,7 +1193,8 @@ private void validateActiveTargets() {
       TargetData expectedTarget = expectedQueries.get(0);
       TargetData actualTarget = actualTargets.get(expected.getKey());
 
-      // TODO: validate the purpose of the target once it's possible to encode that in the
+      // TODO: validate the purpose of the target once it's possible to encode that in
+      // the
       // spec tests. For now, only validate properties that can be validated.
       // assertEquals(expectedTarget, actualTarget);
       assertEquals(expectedTarget.getTarget(), actualTarget.getTarget());
@@ -1233,10 +1253,14 @@ private void runSteps(JSONArray steps, JSONObject config) throws Exception {
     } catch (Exception e) {
       throw Assert.fail("Spec test failed with %s", e);
     } finally {
-      // Ensure that Persistence is torn down even if the test is failing due to a thrown exception
-      // so that any open databases are closed. This is important when the LocalStore is backed by
-      // SQLite because SQLite opens databases in exclusive mode. If tearDownForSpec were not called
-      // after an exception then subsequent attempts to open the SQLite database will fail, making
+      // Ensure that Persistence is torn down even if the test is failing due to a
+      // thrown exception
+      // so that any open databases are closed. This is important when the LocalStore
+      // is backed by
+      // SQLite because SQLite opens databases in exclusive mode. If tearDownForSpec
+      // were not called
+      // after an exception then subsequent attempts to open the SQLite database will
+      // fail, making
       // it harder to zero in on the spec tests as a culprit.
       specTearDown();
     }
@@ -1286,7 +1310,8 @@ public void testSpecTests() throws Exception {
       String fileName = parsedSpecFile.first;
       JSONObject fileJSON = parsedSpecFile.second;
 
-      // Print the names of the files and tests regardless of whether verbose logging is enabled.
+      // Print the names of the files and tests regardless of whether verbose logging
+      // is enabled.
       info("Spec test file: " + fileName);
 
       // Iterate over the tests in the file and run them.