From c7de2e57ecf0884e70802a9b6c3d4dbe27cfe70c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 6 Nov 2017 11:59:21 -0800 Subject: [PATCH 1/4] Reject conflicting updates --- .../com/google/cloud/firestore/BasePath.java | 3 ++- .../com/google/cloud/firestore/FieldPath.java | 7 ++++++- .../google/cloud/firestore/FirestoreImpl.java | 3 ++- .../google/cloud/firestore/UpdateBuilder.java | 21 +++++++++++++++---- .../firestore/DocumentReferenceTest.java | 21 +++++++++++++++++++ 5 files changed, 48 insertions(+), 7 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 962d3aceb945..5bb110ab7b88 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 @@ -53,7 +53,8 @@ B getParent() { * @param path A relative path */ B append(String path) { - Preconditions.checkArgument(path != null && !path.isEmpty(), "'path' must be a non-empty String" ); + Preconditions.checkArgument( + path != null && !path.isEmpty(), "'path' must be a non-empty String"); ImmutableList.Builder components = ImmutableList.builder(); components.addAll(this.getSegments()); components.add(splitChildPath(path)); 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 0c70da06fe8a..28a8696cbb87 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 -abstract class FieldPath extends BasePath { +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 canonicalString().compareTo(other.canonicalString()); + } } diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index cf6402debd5c..70fc46c85d60 100644 --- a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -149,7 +149,8 @@ static Value encodeValue(@Nullable Object sanitizedObject) { "Failed to detect Project ID. " + "Please explicitly set your Project ID in FirestoreOptions."); this.databasePath = - ResourcePath.create(DatabaseRootName.create(options.getProjectId(), options.getDatabaseId())); + ResourcePath.create( + DatabaseRootName.create(options.getProjectId(), options.getDatabaseId())); } @Nonnull 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 7db53d23f416..6beed5146fed 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 @@ -29,6 +29,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; @@ -53,9 +55,11 @@ 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()); + + for (FieldPath field : sortedFields) { + List segments = field.getSegments(); + Object value = data.get(field); Map currentMap = result; @@ -66,7 +70,16 @@ 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)); + + Object nestedMap = currentMap.get(segments.get(i)); + + if (!(nestedMap instanceof Map)) { + throw new IllegalArgumentException( + String.format( + "Detected ambiguous definition for field '%s'.", + FieldPath.of(segments.subList(0, i + 1).toArray(new String[] {})))); + } + currentMap = (Map) nestedMap; } } } 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 6c6dee465577..39311700b000 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 @@ -582,6 +582,27 @@ 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'."); + } + } + @Test public void deleteField() throws Exception { doReturn(SINGLE_WRITE_COMMIT_RESPONSE) From 071417d0eaf06c5480687d1545ae1500f6eab124 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 6 Nov 2017 12:35:54 -0800 Subject: [PATCH 2/4] Adding example with nested map --- .../com/google/cloud/firestore/BasePath.java | 9 ++++++++ .../google/cloud/firestore/UpdateBuilder.java | 21 +++++++++++-------- .../firestore/DocumentReferenceTest.java | 18 ++++++++++++++++ 3 files changed, 39 insertions(+), 9 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 5bb110ab7b88..5445786cabe7 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 @@ -76,4 +76,13 @@ B append(BasePath path) { abstract String[] splitChildPath(String path); abstract B createPathWithSegments(ImmutableList segments); + + boolean isPrefixOf(BasePath 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/UpdateBuilder.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/UpdateBuilder.java index 6beed5146fed..58fdae1b3c68 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 @@ -57,10 +57,19 @@ private static Map expandObject(Map data) { SortedSet sortedFields = new TreeSet<>(data.keySet()); + FieldPath lastField = null; + for (FieldPath field : sortedFields) { List segments = field.getSegments(); Object value = data.get(field); + if (lastField != null) { + if (lastField.isPrefixOf(field)) { + throw new IllegalArgumentException( + String.format("Detected ambiguous definition for field '%s'.", lastField)); + } + } + Map currentMap = result; for (int i = 0; i < segments.size(); ++i) { @@ -71,17 +80,11 @@ private static Map expandObject(Map data) { currentMap.put(segments.get(i), new HashMap<>()); } - Object nestedMap = currentMap.get(segments.get(i)); - - if (!(nestedMap instanceof Map)) { - throw new IllegalArgumentException( - String.format( - "Detected ambiguous definition for field '%s'.", - FieldPath.of(segments.subList(0, i + 1).toArray(new String[] {})))); - } - currentMap = (Map) nestedMap; + 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 39311700b000..ce4d81352f36 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 @@ -601,6 +601,24 @@ public void updateConflictingFields() throws Exception { } 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 From acf3be7e8cd7e7d4e82931620526fcff72ff6bd8 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 6 Nov 2017 14:34:17 -0800 Subject: [PATCH 3/4] Addressing nits --- .../com/google/cloud/firestore/UpdateBuilder.java | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) 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 58fdae1b3c68..94f9a3cc2393 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 @@ -60,16 +60,13 @@ private static Map expandObject(Map data) { FieldPath lastField = null; for (FieldPath field : sortedFields) { - List segments = field.getSegments(); - Object value = data.get(field); - - if (lastField != null) { - if (lastField.isPrefixOf(field)) { - throw new IllegalArgumentException( - String.format("Detected ambiguous definition for field '%s'.", lastField)); - } + 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) { From 87e0d9789ecf64950324b5a9434f8d397c3ebd80 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Mon, 6 Nov 2017 22:33:54 -0800 Subject: [PATCH 4/4] Using the cached version of 'canonicalString' --- .../src/main/java/com/google/cloud/firestore/FieldPath.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 28a8696cbb87..c2e98ec71d2a 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 @@ -155,6 +155,6 @@ public String toString() { @Override public int compareTo(@Nonnull FieldPath other) { - return canonicalString().compareTo(other.canonicalString()); + return getEncodedPath().compareTo(other.getEncodedPath()); } }