getPath() should be relative to the database root (#2599)

schmidt-sebastian · web-flow · commit a53f7a8420f6 · 2017-11-08T19:14:23.000-08:00
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java
@@ -70,7 +70,7 @@ public DocumentReference getParent() {
    */
   @Nonnull
   public String getPath() {
-    return path.toString();
+    return path.getPath();
   }
 
   /**
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentReference.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentReference.java
@@ -68,24 +68,34 @@ public String getId() {
   }
 
   /**
-   * A reference to the Collection to which this DocumentReference belongs to.
+   * A string representing the path of the referenced document (relative to the root of the
+   * database).
    *
-   * @return The parent Collection.
+   * @return The slash-separated path relative to the root of the database.
    */
   @Nonnull
-  public CollectionReference getParent() {
-    return new CollectionReference(firestore, path.getParent());
+  public String getPath() {
+    return path.getPath();
   }
 
   /**
-   * A string representing the path of the referenced document (relative to the root of the
-   * database).
+   * A string representing the fully-qualified path of the referenced document.
    *
-   * @return The slash-separated path relative to the root of the database.
+   * @return The fully-qualified path of the document.
    */
   @Nonnull
-  public String getPath() {
-    return path.toString();
+  String getName() {
+    return path.getName();
+  }
+
+  /**
+   * A reference to the Collection to which this DocumentReference belongs to.
+   *
+   * @return The parent Collection.
+   */
+  @Nonnull
+  public CollectionReference getParent() {
+    return new CollectionReference(firestore, path.getParent());
   }
 
   /**
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentSnapshot.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentSnapshot.java
@@ -388,7 +388,7 @@ public DocumentReference getReference() {
   Document.Builder toPb() {
     Preconditions.checkState(exists(), "Can't call toDocument() on a document that doesn't exist");
     Document.Builder document = Document.newBuilder();
-    document.setName(docRef.getPath());
+    document.setName(docRef.getName());
     document.putAllFields(fields);
     return document;
   }
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentTransform.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentTransform.java
@@ -116,7 +116,7 @@ Write toPb() {
     Write.Builder write = Write.newBuilder();
     com.google.firestore.v1beta1.DocumentTransform.Builder transform = write.getTransformBuilder();
     transform.addAllFieldTransforms(transforms);
-    transform.setDocument(documentReference.getPath());
+    transform.setDocument(documentReference.getName());
     return write.build();
   }
 }
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
@@ -118,7 +118,7 @@ static Value encodeValue(@Nullable Object sanitizedObject) {
       return Value.newBuilder().setBytesValue(blob.toByteString()).build();
     } else if (sanitizedObject instanceof DocumentReference) {
       DocumentReference docRef = (DocumentReference) sanitizedObject;
-      return Value.newBuilder().setReferenceValue(docRef.getPath()).build();
+      return Value.newBuilder().setReferenceValue(docRef.getName()).build();
     } else if (sanitizedObject instanceof Map) {
       MapValue.Builder res = MapValue.newBuilder();
       for (Map.Entry<String, Object> entry : ((Map<String, Object>) sanitizedObject).entrySet()) {
@@ -256,7 +256,7 @@ public void onCompleted() {
     }
 
     for (DocumentReference docRef : documentReferences) {
-      request.addDocuments(docRef.getPath());
+      request.addDocuments(docRef.getName());
     }
 
     streamRequest(request.build(), responseObserver, firestoreClient.batchGetDocumentsCallable());
diff --git a/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ResourcePath.java b/google-cloud-firestore/src/main/java/com/google/cloud/firestore/ResourcePath.java
@@ -84,7 +84,7 @@ boolean isCollection() {
   /**
    * The Path's id (last component).
    *
-   * @return The last component of the Path or null if the path points to the root..
+   * @return The last component of the Path or null if the path points to the root.
    */
   @Nullable
   String getId() {
@@ -98,26 +98,46 @@ String getId() {
   }
 
   /**
-   * String representation as expected by the Firestore API.
+   * The Path's name (the location relative to the root of the database).
    *
-   * @return The formatted name of the resource.
+   * @return The resource path relative to the root of the database.
    */
-  @Override
-  public String toString() {
+  String getPath() {
     StringBuilder result = new StringBuilder();
-    result.append(getDatabaseName());
 
-    ImmutableList<String> parts = getSegments();
-    if (parts.size() > 0) {
-      result.append("/documents");
-      for (String part : parts) {
+    boolean first = true;
+    for (String part : getSegments()) {
+      if (first) {
+        result.append(part);
+        first = false;
+      } else {
         result.append("/").append(part);
       }
     }
 
     return result.toString();
   }
 
+  /**
+   * String representation as expected by the Firestore API.
+   *
+   * @return The formatted name of the resource.
+   */
+  String getName() {
+    String path = getPath();
+
+    if (path.isEmpty()) {
+      return getDatabaseName().toString();
+    } else {
+      return getDatabaseName() + "/documents/" + getPath();
+    }
+  }
+
+  @Override
+  public String toString() {
+    return getName();
+  }
+
   @Override
   String[] splitChildPath(String name) {
     return name.split("/");
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
@@ -451,7 +451,7 @@ public T delete(@Nonnull DocumentReference documentReference) {
   private T performDelete(
       @Nonnull DocumentReference documentReference, @Nonnull Precondition precondition) {
     Write.Builder writeBuilder = Write.newBuilder();
-    writeBuilder.setDelete(documentReference.getPath());
+    writeBuilder.setDelete(documentReference.getName());
     writeBuilder.setCurrentDocument(precondition.toPb());
     writes.add(writeBuilder.build());
     return (T) this;
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/CollectionReferenceTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/CollectionReferenceTest.java
@@ -84,9 +84,7 @@ public void getParent() {
     assertNull(documentReference);
     CollectionReference subcollection = collectionReference.document("doc").collection("subcoll");
     assertEquals("subcoll", subcollection.getId());
-    assertEquals(
-        "projects/test-project/databases/(default)/documents/coll/doc/subcoll",
-        subcollection.getPath());
+    assertEquals("coll/doc/subcoll", subcollection.getPath());
     documentReference = subcollection.getParent();
     assertEquals("doc", documentReference.getId());
   }
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
@@ -22,6 +22,7 @@
 import static com.google.cloud.firestore.LocalFirestoreHelper.BLOB;
 import static com.google.cloud.firestore.LocalFirestoreHelper.DATE;
 import static com.google.cloud.firestore.LocalFirestoreHelper.DOCUMENT_NAME;
+import static com.google.cloud.firestore.LocalFirestoreHelper.DOCUMENT_PATH;
 import static com.google.cloud.firestore.LocalFirestoreHelper.EMPTY_MAP_PROTO;
 import static com.google.cloud.firestore.LocalFirestoreHelper.GEO_POINT;
 import static com.google.cloud.firestore.LocalFirestoreHelper.NESTED_CLASS_OBJECT;
@@ -116,7 +117,7 @@ public void getCollection() {
 
   @Test
   public void getPath() {
-    assertEquals(DOCUMENT_NAME, documentReference.getPath());
+    assertEquals(DOCUMENT_PATH, documentReference.getPath());
   }
 
   @Test
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java
@@ -69,6 +69,7 @@
 public final class LocalFirestoreHelper {
 
   public static final String DATABASE_NAME;
+  public static final String DOCUMENT_PATH;
   public static final String DOCUMENT_NAME;
   public static final ByteString TRANSACTION_ID;
 
@@ -547,7 +548,8 @@ public boolean equals(Object o) {
     BLOB = Blob.fromBytes(new byte[] {1, 2, 3});
 
     DATABASE_NAME = "projects/test-project/databases/(default)";
-    DOCUMENT_NAME = DATABASE_NAME + "/documents/coll/doc";
+    DOCUMENT_PATH = "coll/doc";
+    DOCUMENT_NAME = DATABASE_NAME + "/documents/" + DOCUMENT_PATH;
 
     EMPTY_MAP_PROTO =
         ImmutableMap.of(
diff --git a/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java b/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java
@@ -288,8 +288,7 @@ public void withInvalidStartAt() throws Exception {
       fail();
     } catch (IllegalArgumentException e) {
       assertEquals(
-          "Only a direct child can be used as a query boundary. Found: "
-              + "'projects/test-project/databases/(default)/documents/coll/coll/doc/coll'",
+          "Only a direct child can be used as a query boundary. Found: 'coll/coll/doc/coll'",
           e.getMessage());
     }
 
@@ -298,8 +297,7 @@ public void withInvalidStartAt() throws Exception {
       fail();
     } catch (IllegalArgumentException e) {
       assertEquals(
-          "'projects/test-project/databases/(default)/documents/foo/bar' is not part of "
-              + "the query result set and cannot be used as a query boundary.",
+          "'foo/bar' is not part of the query result set and cannot be used as a query boundary.",
           e.getMessage());
     }
   }

Original file line number	Diff line number	Diff line change
`@@ -70,7 +70,7 @@ public DocumentReference getParent() {`
`70`	`70`	`*/`
`71`	`71`	`@Nonnull`
`72`	`72`	`public String getPath() {`
`73`		`- return path.toString();`
	`73`	`+ return path.getPath();`
`74`	`74`	`}`
`75`	`75`
`76`	`76`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -388,7 +388,7 @@ public DocumentReference getReference() {`
`388`	`388`	`Document.Builder toPb() {`
`389`	`389`	`Preconditions.checkState(exists(), "Can't call toDocument() on a document that doesn't exist");`
`390`	`390`	`Document.Builder document = Document.newBuilder();`
`391`		`- document.setName(docRef.getPath());`
	`391`	`+ document.setName(docRef.getName());`
`392`	`392`	`document.putAllFields(fields);`
`393`	`393`	`return document;`
`394`	`394`	`}`
Original file line number	Diff line number	Diff line change
`@@ -116,7 +116,7 @@ Write toPb() {`
`116`	`116`	`Write.Builder write = Write.newBuilder();`
`117`	`117`	`com.google.firestore.v1beta1.DocumentTransform.Builder transform = write.getTransformBuilder();`
`118`	`118`	`transform.addAllFieldTransforms(transforms);`
`119`		`- transform.setDocument(documentReference.getPath());`
	`119`	`+ transform.setDocument(documentReference.getName());`
`120`	`120`	`return write.build();`
`121`	`121`	`}`
`122`	`122`	`}`