From 0f468378354a9fdf12fe130a0d43993d7c85dfc7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Mar 2022 11:10:18 -0500 Subject: [PATCH 01/10] Change DocumentOverlayCache.SaveOverlays() to require non-null values in the given Map --- .../google/firebase/firestore/local/LocalDocumentsView.java | 5 ++++- .../firestore/local/MemoryDocumentOverlayCache.java | 6 +----- .../firestore/local/SQLiteDocumentOverlayCache.java | 6 ++---- .../google/firebase/firestore/model/mutation/Mutation.java | 1 + .../firebase/firestore/model/mutation/MutationBatch.java | 4 +++- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java index 4d0d7552e09..205cd91512f 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalDocumentsView.java @@ -189,7 +189,10 @@ private void recalculateAndSaveOverlays(Map docs) Map overlays = new HashMap<>(); for (DocumentKey key : entry.getValue()) { if (!processed.contains(key)) { - overlays.put(key, Mutation.calculateOverlayMutation(docs.get(key), masks.get(key))); + Mutation mutation = Mutation.calculateOverlayMutation(docs.get(key), masks.get(key)); + if (mutation != null) { + overlays.put(key, mutation); + } processed.add(key); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java index 52471f49d6e..a569a64b6e6 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java @@ -50,11 +50,7 @@ public Map getOverlays(SortedSet keys) { return result; } - private void saveOverlay(int largestBatchId, @Nullable Mutation mutation) { - if (mutation == null) { - return; - } - + private void saveOverlay(int largestBatchId, Mutation mutation) { // Remove the association of the overlay to its batch id. Overlay existing = this.overlays.get(mutation.getKey()); if (existing != null) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java index 03a8f726a23..301087e8307 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java @@ -106,7 +106,7 @@ private void processSingleCollection( } } - private void saveOverlay(int largestBatchId, DocumentKey key, @Nullable Mutation mutation) { + private void saveOverlay(int largestBatchId, DocumentKey key, Mutation mutation) { String group = key.getCollectionGroup(); String collectionPath = EncodedPath.encode(key.getPath().popLast()); String documentId = key.getPath().getLastSegment(); @@ -125,9 +125,7 @@ private void saveOverlay(int largestBatchId, DocumentKey key, @Nullable Mutation @Override public void saveOverlays(int largestBatchId, Map overlays) { for (Map.Entry entry : overlays.entrySet()) { - if (entry.getValue() != null) { - saveOverlay(largestBatchId, entry.getKey(), entry.getValue()); - } + saveOverlay(largestBatchId, entry.getKey(), entry.getValue()); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java index bf5e337d040..26f2ccea4f2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/Mutation.java @@ -87,6 +87,7 @@ public abstract class Mutation { * of the document, and a {@link FieldMask} representing the fields that are mutated by the local * mutations. */ + @Nullable public static Mutation calculateOverlayMutation(MutableDocument doc, @Nullable FieldMask mask) { if ((!doc.hasLocalMutations()) || (mask != null && mask.getMask().isEmpty())) { return null; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java index e234b3a7fc5..6e1fe042d58 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/mutation/MutationBatch.java @@ -147,7 +147,9 @@ public Map applyToLocalDocumentSet( MutableDocument document = (MutableDocument) documentMap.get(key); FieldMask mutatedFields = applyToLocalView(document); Mutation overlay = Mutation.calculateOverlayMutation(document, mutatedFields); - overlays.put(key, overlay); + if (overlay != null) { + overlays.put(key, overlay); + } if (!document.isValidDocument()) { document.convertToNoDocument(SnapshotVersion.NONE); } From dcc135764fa4a2f1b3b0d58213bc0b3890541053 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Mar 2022 11:35:47 -0500 Subject: [PATCH 02/10] empty commit to re-trigger GitHub actions From 12bbef39a15788e5c50319f9469baf7abfff6d71 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Mar 2022 12:54:24 -0500 Subject: [PATCH 03/10] Assert that the overlays in the map are not null, instead of just assuming that they are not null --- .../firebase/firestore/local/MemoryDocumentOverlayCache.java | 5 ++++- .../firebase/firestore/local/SQLiteDocumentOverlayCache.java | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java index a569a64b6e6..0db2b997377 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.local; +import static com.google.firebase.firestore.util.Preconditions.checkNotNull; + import androidx.annotation.Nullable; import com.google.firebase.firestore.model.DocumentKey; import com.google.firebase.firestore.model.ResourcePath; @@ -69,7 +71,8 @@ private void saveOverlay(int largestBatchId, Mutation mutation) { @Override public void saveOverlays(int largestBatchId, Map overlays) { for (Map.Entry entry : overlays.entrySet()) { - saveOverlay(largestBatchId, entry.getValue()); + Mutation overlay = checkNotNull(entry.getValue(), "null value for key: " + entry.getKey()); + saveOverlay(largestBatchId, overlay); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java index 301087e8307..6614f790f0e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java @@ -16,6 +16,7 @@ import static com.google.firebase.firestore.util.Assert.fail; import static com.google.firebase.firestore.util.Assert.hardAssert; +import static com.google.firebase.firestore.util.Preconditions.checkNotNull; import android.database.Cursor; import androidx.annotation.Nullable; @@ -125,7 +126,9 @@ private void saveOverlay(int largestBatchId, DocumentKey key, Mutation mutation) @Override public void saveOverlays(int largestBatchId, Map overlays) { for (Map.Entry entry : overlays.entrySet()) { - saveOverlay(largestBatchId, entry.getKey(), entry.getValue()); + DocumentKey key = entry.getKey(); + Mutation overlay = checkNotNull(entry.getValue(), "null value for key: " + key); + saveOverlay(largestBatchId, key, overlay); } } From 0eae6aab1c4ef45cd36413c215dadef527cdc3b4 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Mar 2022 13:15:13 -0500 Subject: [PATCH 04/10] Use a new version of Preconditions.checkNotNull() to avoid String computations on the happy path --- .../local/MemoryDocumentOverlayCache.java | 2 +- .../local/SQLiteDocumentOverlayCache.java | 2 +- .../firestore/util/Preconditions.java | 19 +++++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java index 0db2b997377..0731591c5d2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/MemoryDocumentOverlayCache.java @@ -71,7 +71,7 @@ private void saveOverlay(int largestBatchId, Mutation mutation) { @Override public void saveOverlays(int largestBatchId, Map overlays) { for (Map.Entry entry : overlays.entrySet()) { - Mutation overlay = checkNotNull(entry.getValue(), "null value for key: " + entry.getKey()); + Mutation overlay = checkNotNull(entry.getValue(), "null value for key: %s", entry.getKey()); saveOverlay(largestBatchId, overlay); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java index 6614f790f0e..84628edd4a2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteDocumentOverlayCache.java @@ -127,7 +127,7 @@ private void saveOverlay(int largestBatchId, DocumentKey key, Mutation mutation) public void saveOverlays(int largestBatchId, Map overlays) { for (Map.Entry entry : overlays.entrySet()) { DocumentKey key = entry.getKey(); - Mutation overlay = checkNotNull(entry.getValue(), "null value for key: " + key); + Mutation overlay = checkNotNull(entry.getValue(), "null value for key: %s", key); saveOverlay(largestBatchId, key, overlay); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Preconditions.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Preconditions.java index 5270808795e..15dd0e6fee2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Preconditions.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/util/Preconditions.java @@ -14,6 +14,7 @@ package com.google.firebase.firestore.util; +import java.util.Locale; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -149,6 +150,24 @@ public static T checkNotNull( return reference; } + /** + * Ensures that an object reference passed as a parameter to the calling method is not null. + * + * @param reference an object reference + * @param errorFormatString the exception message to use if the check fails; will be formatted + * using {@link String#format}. + * @param formatArgs the args to specify when formatting the given {@code errorFormatString}. + * @return the non-null reference that was validated + * @throws NullPointerException if {@code reference} is null + */ + public static T checkNotNull( + @Nonnull T reference, String errorFormatString, @Nullable Object... formatArgs) { + if (reference == null) { + throw new NullPointerException(String.format(Locale.US, errorFormatString, formatArgs)); + } + return reference; + } + /** * Ensures the truth of an expression involving the state of the calling instance, but not * involving any parameters to the calling method. From 2267bcadf56f068b6db898c46ba63cf29038841d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 10 Mar 2022 13:16:23 -0500 Subject: [PATCH 05/10] Add a unit test for invoking saveOverlays() with a null value in the Map --- .../local/DocumentOverlayCacheTestCase.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/DocumentOverlayCacheTestCase.java b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/DocumentOverlayCacheTestCase.java index ced5e67c8e3..19da1de177f 100644 --- a/firebase-firestore/src/test/java/com/google/firebase/firestore/local/DocumentOverlayCacheTestCase.java +++ b/firebase-firestore/src/test/java/com/google/firebase/firestore/local/DocumentOverlayCacheTestCase.java @@ -23,6 +23,7 @@ import static com.google.firebase.firestore.testutil.TestUtil.setMutation; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import com.google.firebase.firestore.auth.User; @@ -41,6 +42,7 @@ import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.junit.function.ThrowingRunnable; /** * These are tests for any implementation of the DocumentOverlayCache interface. @@ -252,6 +254,24 @@ public void testUpdateDocumentOverlay() { assertNull(cache.getOverlay(DocumentKey.fromPathString("coll/doc"))); } + @Test + public void testSaveOverlaysThrowsNullPointerExceptionOnNullMapValue() { + Map data = new HashMap<>(); + data.put(key("coll/doc"), null); + + NullPointerException e = + assertThrows( + NullPointerException.class, + new ThrowingRunnable() { + @Override + public void run() { + cache.saveOverlays(1, data); + } + }); + + assertThat(e.getMessage()).contains("coll/doc"); + } + void verifyOverlayContains(Map overlays, String... keys) { Set expected = Arrays.stream(keys).map(TestUtil::key).collect(Collectors.toSet()); assertThat(overlays.keySet()).containsExactlyElementsIn(expected); From 4e6ee432e86e744e7db5cf11b1dafd3384c45cb9 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 11 Mar 2022 13:24:26 -0500 Subject: [PATCH 06/10] empty commit to re-trigger GitHub actions From c0afc9ee938c43110d61359b7c56a81e185cb1cf Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 14 Mar 2022 13:01:05 -0400 Subject: [PATCH 07/10] empty commit to re-trigger GitHub actions From f14104166a08307322b5768035ab31c19985f891 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 14 Mar 2022 21:53:07 -0400 Subject: [PATCH 08/10] Cherry-pick #3531 to fix the `More than one file was found with OS independent path META-INF/DEPENDENCIES` build error --- smoke-tests/build.gradle | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/smoke-tests/build.gradle b/smoke-tests/build.gradle index 5a9a8601ff7..4add09d72bb 100644 --- a/smoke-tests/build.gradle +++ b/smoke-tests/build.gradle @@ -51,6 +51,10 @@ android { } } + packagingOptions { + exclude 'META-INF/DEPENDENCIES' + } + compileOptions { sourceCompatibility 1.8 targetCompatibility 1.8 From 27f648f1f890f48c35d179a2fa101e31f511968f Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 14 Mar 2022 22:35:38 -0400 Subject: [PATCH 09/10] Revert "Cherry-pick #3531 to fix the `More than one file was found with OS independent path META-INF/DEPENDENCIES` build error" This reverts commit f14104166a08307322b5768035ab31c19985f891. --- smoke-tests/build.gradle | 4 ---- 1 file changed, 4 deletions(-) diff --git a/smoke-tests/build.gradle b/smoke-tests/build.gradle index 4add09d72bb..5a9a8601ff7 100644 --- a/smoke-tests/build.gradle +++ b/smoke-tests/build.gradle @@ -51,10 +51,6 @@ android { } } - packagingOptions { - exclude 'META-INF/DEPENDENCIES' - } - compileOptions { sourceCompatibility 1.8 targetCompatibility 1.8 From 28570e1e681ffdfc2dec9e527d27aa992d3b6e9d Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Mon, 14 Mar 2022 23:41:57 -0400 Subject: [PATCH 10/10] empty commit to re-trigger GitHub actions