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..7b66fed04143 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 @@ -79,7 +79,7 @@ B append(BasePath path) { * @param path the path to check against * @return true if current path is a prefix of the other path. */ - boolean isPrefixOf(B path) { + boolean isPrefixOf(BasePath path) { ImmutableList prefixSegments = getSegments(); ImmutableList childSegments = path.getSegments(); if (prefixSegments.size() > path.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 { +public abstract class FieldPath extends BasePath implements Comparable { /** * 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 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 { private static Map expandObject(Map data) { Map result = new HashMap<>(); - for (Map.Entry entrySet : data.entrySet()) { - List segments = entrySet.getKey().getSegments(); - Object value = entrySet.getValue(); + SortedSet 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 segments = field.getSegments(); + Object value = data.get(field); Map currentMap = result; for (int i = 0; i < segments.size(); ++i) { @@ -69,9 +79,12 @@ private static Map expandObject(Map data) { if (!currentMap.containsKey(segments.get(i))) { currentMap.put(segments.get(i), new HashMap<>()); } + currentMap = (Map) 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)