Firestore:Fix Observable in calls instead of wrapping in ApiFuture by abhinav-qlogic · Pull Request #6148 · googleapis/google-cloud-java

abhinav-qlogic · 2019-08-23T11:19:10Z

schmidt-sebastian · 2019-08-23T16:08:35Z

...ients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentReference.java


+  /** Fetches the document reference in the form of the stream inside apiStreamObserver */
+  @Nonnull
+  public void get(


I'm still lukewarm on adding additional API surface. I think we can live without DocumentReference.get() since you should be able to call getAll().

schmidt-sebastian · 2019-08-23T16:10:49Z

...cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * @param responseObserver ApiStreamObserver of DocumentSnapshot
+   * @param fieldMask If set, specifies the subset of fields to return.
+   */
+  void getAll(


Please match the signature above and change the order to:

getAll(DocumentReference[] documentReferences, @Nullable FieldMask fieldMask, final ApiStreamObserver<DocumentSnapshot> responseObserver)

Putting the observer last also makes for much more readable code when a Lambda is used.

schmidt-sebastian · 2019-08-23T16:11:50Z

...cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java


+  /**
+   * Retrieves multiple documents from Firestore, while optionally applying a field mask to reduce
+   * the amount of data transmitted.


Please mention that the documents will be returned out of order and that missing documents will not be returned at all.

schmidt-sebastian · 2019-08-23T16:13:07Z

...d-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

    return futureList;
  }

+  void getAll(


Please merge this with the internal method above. You can likely rewrite the existing getAll() to use your new method.

schmidt-sebastian · 2019-08-23T16:13:46Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+          public void onCompleted() {}
+        },
+        documentReferences);
+    Thread.sleep(1000);


Please rewrite this tests to not sleep.

schmidt-sebastian · 2019-08-23T16:13:56Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+          public void onCompleted() {}
+        };
+    ref.get(FieldMask.of("foo"), responseObserver, ref);
+    Thread.sleep(1000);


Please rewrite this tests to not sleep.

schmidt-sebastian · 2019-08-23T16:14:26Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+        },
+        documentReferences);
+    Thread.sleep(1000);
+    assertEquals(map("foo", "bar"), documentSnapshots.get(0).getData());


Please assert response size and fetch multiple documents in this test, including non-existing documents.

schmidt-sebastian · 2019-08-23T16:15:03Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+          @Override
+          public void onCompleted() {}
+        };
+    ref.get(FieldMask.of("foo"), responseObserver, ref);


I think this test will likely go away as per my previous suggestion of removing this API.

codecov · 2019-08-28T11:31:40Z

Codecov Report

Merging #6148 into master will increase coverage by 0.11%.
The diff coverage is 82.35%.

@@             Coverage Diff              @@
##             master    #6148      +/-   ##
============================================
+ Coverage     47.38%    47.5%   +0.11%     
- Complexity    27198    27462     +264     
============================================
  Files          2523     2527       +4     
  Lines        274600   274768     +168     
  Branches      31378    31415      +37     
============================================
+ Hits         130127   130528     +401     
+ Misses       134861   134609     -252     
- Partials       9612     9631      +19

Impacted Files	Coverage Δ	Complexity Δ
...java/com/google/cloud/firestore/FirestoreImpl.java	`82.96% <82.35%> (-0.47%)`	`23 <1> (+1)`
...ud/bigtable/data/v2/stub/EnhancedBigtableStub.java	`94.85% <0%> (-3.63%)`	`22% <0%> (ø)`
.../google/cloud/spanner/jdbc/CredentialsService.java	`91.11% <0%> (-2.23%)`	`11% <0%> (-1%)`
.../com/google/cloud/spanner/jdbc/ConnectionImpl.java	`82.58% <0%> (-1.12%)`	`155% <0%> (ø)`
...a/com/google/cloud/firestore/DocumentSnapshot.java	`84.95% <0%> (-0.76%)`	`55% <0%> (ø)`
...m/google/cloud/spanner/jdbc/ConnectionOptions.java	`82.42% <0%> (-0.4%)`	`46% <0%> (+2%)`
...ain/java/com/google/cloud/pubsub/v1/Publisher.java	`89.11% <0%> (-0.35%)`	`40% <0%> (ø)`
...oogle/cloud/spanner/jdbc/SingleUseTransaction.java	`87.5% <0%> (-0.23%)`	`37% <0%> (-3%)`
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.64% <0%> (-0.01%)`	`1% <0%> (ø)`
...rc/main/java/com/google/cloud/storage/Storage.java	`79.72% <0%> (ø)`	`0% <0%> (ø)`	⬇️
... and 102 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 002f217...74a4aec. Read the comment docs.

schmidt-sebastian

This is pretty close now. Thanks for the update!

schmidt-sebastian · 2019-08-30T02:33:38Z

...cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * will not be returned.
+   *
+   * @param documentReferences Array with Document References to fetch.
+   * @param fieldMask If set, specifies the subset of fields to return.


s/set/not null/

schmidt-sebastian · 2019-08-30T02:35:35Z

...d-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

  public ApiFuture<List<DocumentSnapshot>> getAll(
      @Nonnull DocumentReference... documentReferences) {
-    return this.getAll(documentReferences, null, null);
+    return this.getAll(documentReferences, null, null, null);


For cases like this, it might make sense to add inline comments to describe the arguments:

return this.getAll(documentReferences, /* foo= */ null, /* bar= */ null, /* baz= */ null);

schmidt-sebastian · 2019-08-30T02:39:09Z

...d-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java

      @Nullable FieldMask fieldMask,
-      @Nullable ByteString transactionId) {
+      @Nullable ByteString transactionId,
+      @Nullable final ApiStreamObserver apiStreamObserver) {


Do you think we could have one method that adds the ApiStreamObserver, and one that turns it into an ApiFuture?

void getAll( final DocumentReference[] documentReferences, @Nullable FieldMask fieldMask, @Nullable ByteString transactionId, @Nullable final ApiStreamObserver apiStreamObserver) { ... } ApiFuture<List<DocumentSnapshot>> getAll( final DocumentReference[] documentReferences, @Nullable FieldMask fieldMask, @Nullable ByteString transactionId) { List results.... getAll(..., new ApiStreamObserver() -> { result.add(); }); // Sort and return task }

schmidt-sebastian · 2019-08-30T02:40:03Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+
+  @Test
+  public void getAllWithObserver() throws Exception {
+    DocumentReference ref = randomColl.document("doc1");


s/ref/ref1/

schmidt-sebastian · 2019-08-30T02:40:12Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+    DocumentReference ref = randomColl.document("doc1");
+    ref.set(ALL_SUPPORTED_TYPES_MAP).get();
+
+    DocumentReference ref1 = randomColl.document("doc2");


s/ref1/ref2

schmidt-sebastian · 2019-08-30T02:41:01Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+
+    DocumentReference ref2 = randomColl.document("doc3");
+    ref2.set(ALL_SUPPORTED_TYPES_MAP).get();
+    ref2.delete();


You don't need to set and delete a document in order for it to show up as "missing". You can just query a non-existing document that you never created.

schmidt-sebastian · 2019-08-30T02:44:10Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+          assertFalse(documentSnapshot.exists());
+        }
+      }
+    }


I'd slightly prefer if you checked the document references. Do note that they are returned alphabetically sorted by the backend and you could just verify get them by index.

schmidt-sebastian · 2019-08-30T02:45:04Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+        if (null != documentSnapshot.getData()) {
+          assertEquals(map("foo", "bar"), documentSnapshot.getData());
+        } else {
+          assertFalse(documentSnapshot.exists());


This check (getData() == null && ! docSnapshot.exists()) is always true and doesn't quite verify that one of your documents is not returned.

schmidt-sebastian · 2019-08-30T02:45:14Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+          }
+        });
+
+    future.get();


Thank you for updating this!

schmidt-sebastian · 2019-08-30T02:46:02Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+    if (null != documentSnapshots && documentSnapshots.size() > 0) {
+      for (DocumentSnapshot documentSnapshot : documentSnapshots) {
+        if (null != documentSnapshot.getData()) {
+          assertEquals(map("foo", "bar"), documentSnapshot.getData());


I'm surprised this work because ALL_SUPPORTED_TYPES_MAP has a lot more fields.

BenWhitehead · 2019-08-30T15:31:22Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+    ref2.set(ALL_SUPPORTED_TYPES_MAP).get();
+    ref2.delete();
+
+    final List<DocumentSnapshot> documentSnapshots = new ArrayList<>();


This should be wrapped in a synchronized list since it is being written and read from different threads.

Suggested change

final List<DocumentSnapshot> documentSnapshots = new ArrayList<>();

final List<DocumentSnapshot> documentSnapshots = Collections.synchronizedList(new ArrayList<DocumentSnapshot>());

BenWhitehead · 2019-08-30T15:37:58Z

...clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java

+
+    future.get();
+
+    if (null != documentSnapshots && documentSnapshots.size() > 0) {


This if statement doesn't actually do anything. documentSnapshots is never null due to the final initializer above, and the size check before iterating is not needed due to the for-each not executing if the list is empty.

schmidt-sebastian

Thanks! This looks good. Can you try to remove the "transactionId" argument before merging?

schmidt-sebastian · 2019-09-03T16:22:49Z

...cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   * @param documentReferences Array with Document References to fetch.
+   * @param fieldMask If not null, specifies the subset of fields to return.
+   * @param transactionId Id of {@link Transaction}
+   * @param responseObserver ApiStreamObserver of DocumentSnapshot


Since this is the external Firestore API, do you mind coming up with a more descriptive way to describe responseObserver?

schmidt-sebastian · 2019-09-03T16:23:48Z

...cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+   *
+   * @param documentReferences Array with Document References to fetch.
+   * @param fieldMask If not null, specifies the subset of fields to return.
+   * @param transactionId Id of {@link Transaction}


Ideally, the transaction ID handling would not be exposed in the public API. We currently don't use the transaction ID in any of our customer facing APIs, but instead have helper methods that use them internally.

schmidt-sebastian · 2019-09-03T16:26:11Z

...cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java

+  /**
+   * Retrieves multiple documents from Firestore while optionally applying a field mask to reduce
+   * the amount of data transmitted. Returned documents will be out of order and missing documents
+   * will not be returned.


Hm, it looks like I was wrong and missing documents do get returned. Can you remove the last part of your API doc?

schmidt-sebastian · 2019-09-04T15:56:17Z

@BenWhitehead This is good to be merged.

BenWhitehead · 2019-09-04T16:04:10Z

Thanks @schmidt-sebastian, I'll hold off until the builds have completed (there appears to be some slowness with the build queue).

abhinav-qlogic added 4 commits August 23, 2019 16:39

Fix getAll and get methods With observer

817f455

Fix getAll and get method changes

e28a7ba

getAll and get method changes

c998ec4

get method javadoc changes

0cdd424

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 23, 2019

abhinav-qlogic requested review from BenWhitehead and schmidt-sebastian August 23, 2019 11:22

schmidt-sebastian suggested changes Aug 23, 2019

View reviewed changes

Fix review changes

a553d5a

rahulKQL added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 29, 2019

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 29, 2019

schmidt-sebastian reviewed Aug 30, 2019

View reviewed changes

schmidt-sebastian assigned abhinav-qlogic Aug 30, 2019

BenWhitehead suggested changes Aug 30, 2019

View reviewed changes

review changes

70b827f

BenWhitehead approved these changes Sep 3, 2019

View reviewed changes

schmidt-sebastian approved these changes Sep 3, 2019

View reviewed changes

review changes

74a4aec

chingor13 merged commit 61a759a into googleapis:master Sep 11, 2019

kolea2 mentioned this pull request Sep 11, 2019

Release google-cloud-java v0.109.0 #6252

Merged

	final List<DocumentSnapshot> documentSnapshots = new ArrayList<>();
	final List<DocumentSnapshot> documentSnapshots = Collections.synchronizedList(new ArrayList<DocumentSnapshot>());


		future.get();

		if (null != documentSnapshots && documentSnapshots.size() > 0) {

Conversation

abhinav-qlogic commented Aug 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov bot commented Aug 28, 2019 •

edited

Loading