From 7c93395874187c4502e078679e04eb15402bde0e Mon Sep 17 00:00:00 2001
From: Sebastian Schmidt <mrschmidt@google.com>
Date: Thu, 9 Nov 2017 16:07:44 -0800
Subject: [PATCH] Reject conflicting updates (#2582)

---
 .../com/google/cloud/firestore/BasePath.java  |  9 +++++
 .../com/google/cloud/firestore/FieldPath.java |  7 +++-
 .../google/cloud/firestore/UpdateBuilder.java | 19 +++++++--
 .../firestore/DocumentReferenceTest.java      | 39 +++++++++++++++++++
 4 files changed, 70 insertions(+), 4 deletions(-)

diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/BasePath.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/BasePath.java
index 77292471140a..704a9f5efe28 100644
--- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/BasePath.java
+++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/BasePath.java
@@ -96,4 +96,13 @@ boolean isPrefixOf(B path) {
   abstract String[] splitChildPath(String path);
 
   abstract B createPathWithSegments(ImmutableList<String> segments);
+
+  boolean isPrefixOf(BasePath<B> field) {
+    if (field.getSegments().size() < this.getSegments().size()) {
+      return false;
+    }
+
+    return this.equals(
+        createPathWithSegments(field.getSegments().subList(0, this.getSegments().size())));
+  }
 }
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FieldPath.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FieldPath.java
index cc13f34ff1dc..73accb250603 100644
--- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FieldPath.java
+++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FieldPath.java
@@ -28,7 +28,7 @@
  * field in the document).
  */
 @AutoValue
-public abstract class FieldPath extends BasePath<FieldPath> {
+public abstract class FieldPath extends BasePath<FieldPath> implements Comparable<FieldPath> {
 
   /**
    * A special sentinel FieldPath to refer to the ID of a document. It can be used in queries to
@@ -152,4 +152,9 @@ FieldPath createPathWithSegments(ImmutableList<String> segments) {
   public String toString() {
     return getEncodedPath();
   }
+
+  @Override
+  public int compareTo(@Nonnull FieldPath other) {
+    return getEncodedPath().compareTo(other.getEncodedPath());
+  }
 }
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java
index d1ad61a5607c..361d0e98b824 100644
--- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java
+++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java
@@ -32,6 +32,8 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.SortedSet;
+import java.util.TreeSet;
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
@@ -56,10 +58,18 @@ abstract class UpdateBuilder<T extends UpdateBuilder> {
   private static Map<String, Object> expandObject(Map<FieldPath, Object> data) {
     Map<String, Object> result = new HashMap<>();
 
-    for (Map.Entry<FieldPath, Object> entrySet : data.entrySet()) {
-      List<String> segments = entrySet.getKey().getSegments();
-      Object value = entrySet.getValue();
+    SortedSet<FieldPath> sortedFields = new TreeSet<>(data.keySet());
 
+    FieldPath lastField = null;
+
+    for (FieldPath field : sortedFields) {
+      if (lastField != null && lastField.isPrefixOf(field)) {
+        throw new IllegalArgumentException(
+            String.format("Detected ambiguous definition for field '%s'.", lastField));
+      }
+
+      List<String> segments = field.getSegments();
+      Object value = data.get(field);
       Map<String, Object> currentMap = result;
 
       for (int i = 0; i < segments.size(); ++i) {
@@ -69,9 +79,12 @@ private static Map<String, Object> expandObject(Map<FieldPath, Object> data) {
           if (!currentMap.containsKey(segments.get(i))) {
             currentMap.put(segments.get(i), new HashMap<>());
           }
+
           currentMap = (Map<String, Object>) currentMap.get(segments.get(i));
         }
       }
+
+      lastField = field;
     }
 
     return result;
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java
index ab602fff7578..723e01102410 100644
--- a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java
+++ b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/DocumentReferenceTest.java
@@ -584,6 +584,45 @@ public void updateWithDotsInFieldName() throws Exception {
     assertCommitEquals(expectedCommit, commitCapture.getValue());
   }
 
+  @Test
+  public void updateConflictingFields() throws Exception {
+   try {
+     documentReference
+         .update("a.b", "foo", "a", "foo")
+         .get();
+     fail();
+   } catch (IllegalArgumentException e) {
+      assertEquals(e.getMessage(), "Detected ambiguous definition for field 'a'.");
+   }
+
+    try {
+      documentReference
+          .update("a.b", "foo", "a.b.c", "foo")
+          .get();
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertEquals(e.getMessage(), "Detected ambiguous definition for field 'a.b'.");
+    }
+
+    try {
+      documentReference
+          .update("a.b", SINGLE_FIELD_MAP, "a", SINGLE_FIELD_MAP)
+          .get();
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertEquals(e.getMessage(), "Detected ambiguous definition for field 'a'.");
+    }
+
+    try {
+      documentReference
+          .update("a.b", SINGLE_FIELD_MAP, "a.b.c", SINGLE_FIELD_MAP)
+          .get();
+      fail();
+    } catch (IllegalArgumentException e) {
+      assertEquals(e.getMessage(), "Detected ambiguous definition for field 'a.b'.");
+    }
+  }
+
   @Test
   public void deleteField() throws Exception {
     doReturn(SINGLE_WRITE_COMMIT_RESPONSE)