From 54f0766ab20f236fef85d39de686770fb099b365 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 11:00:01 -0500 Subject: [PATCH 01/11] fix: Make ObjectValue thread safe. --- .../firebase/firestore/model/ObjectValue.java | 75 ++++++++++++------- 1 file changed, 49 insertions(+), 26 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index a7ea2997618..9233f1b5b1c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -24,6 +24,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; +import java.util.Objects; import java.util.Set; /** A structured object value stored in Firestore. */ @@ -41,6 +42,13 @@ public final class ObjectValue implements Cloneable { */ private final Map overlayMap = new HashMap<>(); + + /** + * Dirty bit for when `overlayMap` is modified. The memoized `partialValue` only needs to be + * updated when dirty bit is true. + */ + private volatile boolean dirty = false; + public static ObjectValue fromMap(Map value) { return new ObjectValue( Value.newBuilder().setMapValue(MapValue.newBuilder().putAllFields(value)).build()); @@ -103,20 +111,24 @@ private FieldMask extractFieldMask(MapValue value) { } @Nullable - private Value extractNestedValue(Value value, FieldPath fieldPath) { + private static Value extractNestedValue(Value value, FieldPath fieldPath) { if (fieldPath.isEmpty()) { return value; } else { for (int i = 0; i < fieldPath.length() - 1; ++i) { - value = value.getMapValue().getFieldsOrDefault(fieldPath.getSegment(i), null); - if (!Values.isMapValue(value)) { + value = extractSegmentValue(value, fieldPath.getSegment(i)); + if (value == null) { return null; } } - return value.getMapValue().getFieldsOrDefault(fieldPath.getLastSegment(), null); + return extractSegmentValue(value, fieldPath.getLastSegment()); } } + private static Value extractSegmentValue(Value value, String segment) { + return Values.isMapValue(value) ? value.getMapValue().getFieldsOrDefault(segment, null) : null; + } + /** * Returns the Protobuf that backs this ObjectValue. * @@ -124,11 +136,16 @@ private Value extractNestedValue(Value value, FieldPath fieldPath) { * invocations are based on this memoized result. */ private Value buildProto() { - synchronized (overlayMap) { - MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap); - if (mergedResult != null) { - partialValue = Value.newBuilder().setMapValue(mergedResult).build(); - overlayMap.clear(); + if (dirty) { + synchronized (overlayMap) { + if (dirty) { + MapValue mergedResult = applyOverlay(FieldPath.EMPTY_PATH, overlayMap, partialValue); + if (mergedResult != null) { + partialValue = Value.newBuilder().setMapValue(mergedResult).build(); + } + overlayMap.clear(); + dirty = false; + } } } return partialValue; @@ -142,7 +159,9 @@ private Value buildProto() { */ public void delete(FieldPath path) { hardAssert(!path.isEmpty(), "Cannot delete field for empty path on ObjectValue"); - setOverlay(path, null); + synchronized (overlayMap) { + setOverlay(path, null); + } } /** @@ -153,17 +172,15 @@ public void delete(FieldPath path) { */ public void set(FieldPath path, Value value) { hardAssert(!path.isEmpty(), "Cannot set field for empty path on ObjectValue"); - setOverlay(path, value); + synchronized (overlayMap) { + setOverlay(path, value); + } } public void setAll(Map data) { - for (Map.Entry entry : data.entrySet()) { - FieldPath path = entry.getKey(); - if (entry.getValue() == null) { - delete(path); - } else { - set(path, entry.getValue()); - } + synchronized (overlayMap) { + hardAssert(!data.containsKey(FieldPath.EMPTY_PATH), "Cannot setAll on ObjectValue because map contains empty path"); + data.forEach(this::setOverlay); } } @@ -195,7 +212,11 @@ private void setOverlay(FieldPath path, @Nullable Value value) { } } - currentLevel.put(path.getLastSegment(), value); + String lastSegment = path.getLastSegment(); + if (!Objects.equals(currentLevel.get(lastSegment), value)) { + currentLevel.put(lastSegment, value); + dirty = true; + } } /** @@ -206,18 +227,20 @@ private void setOverlay(FieldPath path, @Nullable Value value) { * FieldValue.EMPTY_PATH} to represent the root. * @param currentOverlays The overlays at the current nesting level in the same format as {@code * overlayMap}. + * @param currentValue The existing value at the current nesting level. * @return The merged data at `currentPath` or null if no modifications were applied. */ - private @Nullable MapValue applyOverlay( - FieldPath currentPath, Map currentOverlays) { + private static @Nullable MapValue applyOverlay( + FieldPath currentPath, Map currentOverlays, Value currentValue) { + if (currentOverlays.isEmpty()) { + return null; + } boolean modified = false; - - @Nullable Value existingValue = extractNestedValue(partialValue, currentPath); MapValue.Builder resultAtPath = - Values.isMapValue(existingValue) + Values.isMapValue(currentValue) // If there is already data at the current path, base our modifications on top // of the existing data. - ? existingValue.getMapValue().toBuilder() + ? currentValue.getMapValue().toBuilder() : MapValue.newBuilder(); for (Map.Entry entry : currentOverlays.entrySet()) { @@ -227,7 +250,7 @@ private void setOverlay(FieldPath path, @Nullable Value value) { if (value instanceof Map) { @Nullable MapValue nested = - applyOverlay(currentPath.append(pathSegment), (Map) value); + applyOverlay(currentPath.append(pathSegment), (Map) value, extractSegmentValue(currentValue, pathSegment)); if (nested != null) { resultAtPath.putFields(pathSegment, Value.newBuilder().setMapValue(nested).build()); modified = true; From 411cd4c575327e6d8f43e5353d85bf9289795f52 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 11:13:00 -0500 Subject: [PATCH 02/11] Pretty --- .../google/firebase/firestore/model/ObjectValue.java | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index 9233f1b5b1c..915e0089936 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -42,7 +42,6 @@ public final class ObjectValue implements Cloneable { */ private final Map overlayMap = new HashMap<>(); - /** * Dirty bit for when `overlayMap` is modified. The memoized `partialValue` only needs to be * updated when dirty bit is true. @@ -179,7 +178,9 @@ public void set(FieldPath path, Value value) { public void setAll(Map data) { synchronized (overlayMap) { - hardAssert(!data.containsKey(FieldPath.EMPTY_PATH), "Cannot setAll on ObjectValue because map contains empty path"); + hardAssert( + !data.containsKey(FieldPath.EMPTY_PATH), + "Cannot setAll on ObjectValue because map contains empty path"); data.forEach(this::setOverlay); } } @@ -231,7 +232,7 @@ private void setOverlay(FieldPath path, @Nullable Value value) { * @return The merged data at `currentPath` or null if no modifications were applied. */ private static @Nullable MapValue applyOverlay( - FieldPath currentPath, Map currentOverlays, Value currentValue) { + FieldPath currentPath, Map currentOverlays, Value currentValue) { if (currentOverlays.isEmpty()) { return null; } @@ -250,7 +251,10 @@ private void setOverlay(FieldPath path, @Nullable Value value) { if (value instanceof Map) { @Nullable MapValue nested = - applyOverlay(currentPath.append(pathSegment), (Map) value, extractSegmentValue(currentValue, pathSegment)); + applyOverlay( + currentPath.append(pathSegment), + (Map) value, + extractSegmentValue(currentValue, pathSegment)); if (nested != null) { resultAtPath.putFields(pathSegment, Value.newBuilder().setMapValue(nested).build()); modified = true; From 1d3cef266f07327eab71e0d00dbdd331efca50ba Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 11:34:54 -0500 Subject: [PATCH 03/11] Changelog --- firebase-firestore/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 8773057577c..5c2da1125ff 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -1,6 +1,7 @@ # Unreleased * [changed] Internal test improvements. * [fixed] Fixed the `@Exclude` annotation doesn't been propagated to Kotlin's corresponding bridge methods. [#5626](//github.com/firebase/firebase-android-sdk/pull/5626) +* [fixed] Remove possible thread safety issues for ObjectValue. [#5699](//github.com/firebase/firebase-android-sdk/pull/5699) # 24.10.1 * [fixed] Fixed an issue caused by calling mutation on immutable map object. [#5573](//github.com/firebase/firebase-android-sdk/pull/5573) From 56872f4ebfecddf363a8829aaabc23e7f3fa81a3 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 14:15:14 -0500 Subject: [PATCH 04/11] Fix --- .../firebase/firestore/model/ObjectValue.java | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index 915e0089936..de86542d74c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -214,9 +214,17 @@ private void setOverlay(FieldPath path, @Nullable Value value) { } String lastSegment = path.getLastSegment(); - if (!Objects.equals(currentLevel.get(lastSegment), value)) { - currentLevel.put(lastSegment, value); - dirty = true; + if (value == null) { // Delete + // Get will return null if entry does not exist, so we must also do containsKey. + if (currentLevel.get(lastSegment) != null || !currentLevel.containsKey(lastSegment)) { + currentLevel.put(lastSegment, null); + dirty = true; + } + } else { // Set + if (!value.equals(currentLevel.get(lastSegment))) { + currentLevel.put(lastSegment, value); + dirty = true; + } } } From a711b7d83ab02425934844f94dcbcc5120e53190 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 14:37:22 -0500 Subject: [PATCH 05/11] Pretty --- .../java/com/google/firebase/firestore/model/ObjectValue.java | 1 - 1 file changed, 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index de86542d74c..2235d9081d8 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -24,7 +24,6 @@ import java.util.HashMap; import java.util.HashSet; import java.util.Map; -import java.util.Objects; import java.util.Set; /** A structured object value stored in Firestore. */ From 0d64d287b74169638367cdd0c4177011c374b556 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Thu, 8 Feb 2024 15:47:22 -0500 Subject: [PATCH 06/11] fix --- .../java/com/google/firebase/firestore/model/ObjectValue.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index 2235d9081d8..bff5aa8daf6 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -180,7 +180,9 @@ public void setAll(Map data) { hardAssert( !data.containsKey(FieldPath.EMPTY_PATH), "Cannot setAll on ObjectValue because map contains empty path"); - data.forEach(this::setOverlay); + for (Map.Entry entry : data.entrySet()) { + setOverlay(entry.getKey(), entry.getValue()); + } } } From c18bd0cf548b80c04c788658256c0a8498b08737 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 9 Feb 2024 09:47:50 -0500 Subject: [PATCH 07/11] Doesn't need to be volatile --- .../java/com/google/firebase/firestore/model/ObjectValue.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index bff5aa8daf6..fb99dcdc8eb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -45,7 +45,7 @@ public final class ObjectValue implements Cloneable { * Dirty bit for when `overlayMap` is modified. The memoized `partialValue` only needs to be * updated when dirty bit is true. */ - private volatile boolean dirty = false; + private boolean dirty = false; public static ObjectValue fromMap(Map value) { return new ObjectValue( From 6841850e20d5b4bc2c29ce9231eb88bd007f6f96 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Fri, 9 Feb 2024 12:27:13 -0500 Subject: [PATCH 08/11] Re-write in hopes this will fix flaky test. --- .../firestore/ServerTimestampTest.java | 34 ++++++++----------- 1 file changed, 15 insertions(+), 19 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java index 4fcc6f4f6f4..55bed2449be 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java @@ -23,8 +23,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; @@ -357,29 +357,25 @@ public void testPOJOSupport() { @Test public void testPOJOWithWrongType() { DocumentReference ref = testDocument(); - try { - ref.set(new TimestampPOJOWithWrongType()); - fail("Expected exception."); - } catch (IllegalArgumentException e) { - assertEquals( - "Field timestamp is annotated with @ServerTimestamp but is class " - + "java.lang.String instead of Date or Timestamp.", - e.getMessage()); - } + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, () -> ref.set(new TimestampPOJOWithWrongType())); + assertEquals( + "Field timestamp is annotated with @ServerTimestamp but is class " + + "java.lang.String instead of Date or Timestamp.", + e.getMessage()); } @Test public void testPOJOWithAnnotatedSetter() { DocumentReference ref = testDocument(); - try { - ref.set(new TimestampPOJOWithAnnotatedSetter()); - fail("Expected exception."); - } catch (IllegalArgumentException e) { - assertEquals( - "Method setTimestamp is annotated with @ServerTimestamp but should not be. " - + "@ServerTimestamp can only be applied to fields and getters, not setters.", - e.getMessage()); - } + IllegalArgumentException e = + assertThrows( + IllegalArgumentException.class, () -> ref.set(new TimestampPOJOWithAnnotatedSetter())); + assertEquals( + "Method setTimestamp is annotated with @ServerTimestamp but should not be. " + + "@ServerTimestamp can only be applied to fields and getters, not setters.", + e.getMessage()); } private static class TimestampPOJO { From 255a059ddac8b92e571970ed7908256c85b33d40 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Mon, 12 Feb 2024 13:31:51 -0500 Subject: [PATCH 09/11] Merge root --- firebase-firestore/CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/CHANGELOG.md b/firebase-firestore/CHANGELOG.md index 643edeca593..b380f18ae01 100644 --- a/firebase-firestore/CHANGELOG.md +++ b/firebase-firestore/CHANGELOG.md @@ -3,7 +3,12 @@ # 24.10.2 * [changed] Internal test improvements. -* [fixed] Fixed the `@Exclude` annotation doesn't been propagated to Kotlin's corresponding bridge methods. [#5626](//github.com/firebase/firebase-android-sdk/pull/5626) + + +## Kotlin +The Kotlin extensions library transitively includes the updated +`firebase-firestore` library. The Kotlin extensions library has no additional +updates. # 24.10.1 * [fixed] Fixed an issue caused by calling mutation on immutable map object. [#5573](//github.com/firebase/firebase-android-sdk/pull/5573) From 9b284f60c0a5fb74f1cc44c8c2d2deeda30db10f Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 13 Feb 2024 12:52:21 -0500 Subject: [PATCH 10/11] Revert --- .../firestore/ServerTimestampTest.java | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java index 55bed2449be..4fcc6f4f6f4 100644 --- a/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java +++ b/firebase-firestore/src/androidTest/java/com/google/firebase/firestore/ServerTimestampTest.java @@ -23,8 +23,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.android.gms.tasks.Task; @@ -357,25 +357,29 @@ public void testPOJOSupport() { @Test public void testPOJOWithWrongType() { DocumentReference ref = testDocument(); - IllegalArgumentException e = - assertThrows( - IllegalArgumentException.class, () -> ref.set(new TimestampPOJOWithWrongType())); - assertEquals( - "Field timestamp is annotated with @ServerTimestamp but is class " - + "java.lang.String instead of Date or Timestamp.", - e.getMessage()); + try { + ref.set(new TimestampPOJOWithWrongType()); + fail("Expected exception."); + } catch (IllegalArgumentException e) { + assertEquals( + "Field timestamp is annotated with @ServerTimestamp but is class " + + "java.lang.String instead of Date or Timestamp.", + e.getMessage()); + } } @Test public void testPOJOWithAnnotatedSetter() { DocumentReference ref = testDocument(); - IllegalArgumentException e = - assertThrows( - IllegalArgumentException.class, () -> ref.set(new TimestampPOJOWithAnnotatedSetter())); - assertEquals( - "Method setTimestamp is annotated with @ServerTimestamp but should not be. " - + "@ServerTimestamp can only be applied to fields and getters, not setters.", - e.getMessage()); + try { + ref.set(new TimestampPOJOWithAnnotatedSetter()); + fail("Expected exception."); + } catch (IllegalArgumentException e) { + assertEquals( + "Method setTimestamp is annotated with @ServerTimestamp but should not be. " + + "@ServerTimestamp can only be applied to fields and getters, not setters.", + e.getMessage()); + } } private static class TimestampPOJO { From 78d1c1e6ef1264a37f89b8dc10f95d77d6207f87 Mon Sep 17 00:00:00 2001 From: Tom Andersen Date: Tue, 13 Feb 2024 16:03:36 -0500 Subject: [PATCH 11/11] Fix --- .../com/google/firebase/firestore/model/ObjectValue.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java index fb99dcdc8eb..5e9c644c544 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/model/ObjectValue.java @@ -176,10 +176,10 @@ public void set(FieldPath path, Value value) { } public void setAll(Map data) { + hardAssert( + !data.containsKey(FieldPath.EMPTY_PATH), + "Cannot setAll on ObjectValue because map contains empty path"); synchronized (overlayMap) { - hardAssert( - !data.containsKey(FieldPath.EMPTY_PATH), - "Cannot setAll on ObjectValue because map contains empty path"); for (Map.Entry entry : data.entrySet()) { setOverlay(entry.getKey(), entry.getValue()); }