Firestore:Fix Observable in calls instead of wrapping in ApiFuture #6148
Conversation
|
|
||
| /** Fetches the document reference in the form of the stream inside apiStreamObserver */ | ||
| @Nonnull | ||
| public void get( |
There was a problem hiding this comment.
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().
| * @param responseObserver ApiStreamObserver of DocumentSnapshot | ||
| * @param fieldMask If set, specifies the subset of fields to return. | ||
| */ | ||
| void getAll( |
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * Retrieves multiple documents from Firestore, while optionally applying a field mask to reduce | ||
| * the amount of data transmitted. |
There was a problem hiding this comment.
Please mention that the documents will be returned out of order and that missing documents will not be returned at all.
| return futureList; | ||
| } | ||
|
|
||
| void getAll( |
There was a problem hiding this comment.
Please merge this with the internal method above. You can likely rewrite the existing getAll() to use your new method.
| public void onCompleted() {} | ||
| }, | ||
| documentReferences); | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
Please rewrite this tests to not sleep.
| public void onCompleted() {} | ||
| }; | ||
| ref.get(FieldMask.of("foo"), responseObserver, ref); | ||
| Thread.sleep(1000); |
There was a problem hiding this comment.
Please rewrite this tests to not sleep.
| }, | ||
| documentReferences); | ||
| Thread.sleep(1000); | ||
| assertEquals(map("foo", "bar"), documentSnapshots.get(0).getData()); |
There was a problem hiding this comment.
Please assert response size and fetch multiple documents in this test, including non-existing documents.
| @Override | ||
| public void onCompleted() {} | ||
| }; | ||
| ref.get(FieldMask.of("foo"), responseObserver, ref); |
There was a problem hiding this comment.
I think this test will likely go away as per my previous suggestion of removing this API.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
This is pretty close now. Thanks for the update!
| * will not be returned. | ||
| * | ||
| * @param documentReferences Array with Document References to fetch. | ||
| * @param fieldMask If set, specifies the subset of fields to return. |
| public ApiFuture<List<DocumentSnapshot>> getAll( | ||
| @Nonnull DocumentReference... documentReferences) { | ||
| return this.getAll(documentReferences, null, null); | ||
| return this.getAll(documentReferences, null, null, null); |
There was a problem hiding this comment.
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);
| @Nullable FieldMask fieldMask, | ||
| @Nullable ByteString transactionId) { | ||
| @Nullable ByteString transactionId, | ||
| @Nullable final ApiStreamObserver apiStreamObserver) { |
There was a problem hiding this comment.
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
}
|
|
||
| @Test | ||
| public void getAllWithObserver() throws Exception { | ||
| DocumentReference ref = randomColl.document("doc1"); |
| DocumentReference ref = randomColl.document("doc1"); | ||
| ref.set(ALL_SUPPORTED_TYPES_MAP).get(); | ||
|
|
||
| DocumentReference ref1 = randomColl.document("doc2"); |
|
|
||
| DocumentReference ref2 = randomColl.document("doc3"); | ||
| ref2.set(ALL_SUPPORTED_TYPES_MAP).get(); | ||
| ref2.delete(); |
There was a problem hiding this comment.
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.
| assertFalse(documentSnapshot.exists()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| if (null != documentSnapshot.getData()) { | ||
| assertEquals(map("foo", "bar"), documentSnapshot.getData()); | ||
| } else { | ||
| assertFalse(documentSnapshot.exists()); |
There was a problem hiding this comment.
This check (getData() == null && ! docSnapshot.exists()) is always true and doesn't quite verify that one of your documents is not returned.
| } | ||
| }); | ||
|
|
||
| future.get(); |
There was a problem hiding this comment.
Thank you for updating this!
| if (null != documentSnapshots && documentSnapshots.size() > 0) { | ||
| for (DocumentSnapshot documentSnapshot : documentSnapshots) { | ||
| if (null != documentSnapshot.getData()) { | ||
| assertEquals(map("foo", "bar"), documentSnapshot.getData()); |
There was a problem hiding this comment.
I'm surprised this work because ALL_SUPPORTED_TYPES_MAP has a lot more fields.
| ref2.set(ALL_SUPPORTED_TYPES_MAP).get(); | ||
| ref2.delete(); | ||
|
|
||
| final List<DocumentSnapshot> documentSnapshots = new ArrayList<>(); |
There was a problem hiding this comment.
This should be wrapped in a synchronized list since it is being written and read from different threads.
| final List<DocumentSnapshot> documentSnapshots = new ArrayList<>(); | |
| final List<DocumentSnapshot> documentSnapshots = Collections.synchronizedList(new ArrayList<DocumentSnapshot>()); |
|
|
||
| future.get(); | ||
|
|
||
| if (null != documentSnapshots && documentSnapshots.size() > 0) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks! This looks good. Can you try to remove the "transactionId" argument before merging?
| * @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 |
There was a problem hiding this comment.
Since this is the external Firestore API, do you mind coming up with a more descriptive way to describe responseObserver?
| * | ||
| * @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} |
There was a problem hiding this comment.
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.
| /** | ||
| * 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. |
There was a problem hiding this comment.
Hm, it looks like I was wrong and missing documents do get returned. Can you remove the last part of your API doc?
|
@BenWhitehead This is good to be merged. |
|
Thanks @schmidt-sebastian, I'll hold off until the builds have completed (there appears to be some slowness with the build queue). |
Fixes #5887