[Firestore] Snippets for document snapshots and paginated queries. #1243
Conversation
| @Test | ||
| public void testPaginateCursor() throws Exception { | ||
| // Snippet executes it's own query. Failures result in thrown Exceptions | ||
| queryDataSnippets.paginateCursor(); |
There was a problem hiding this comment.
This test doesn't verify that the API is returning what's expected, just that it doesn't through any exceptions.
This is my concern with methods that don't return anything, except this one doesn't even log anything to stdout either, so there's no way to catch any bugs or changes from one version to another.
Maybe that's not technically a goal or responsibility with our samples, but if a bug gets through the API and doesn't get caught in our samples, that's going to mean people taking our code and using it in their own projects, and it not working for them either.
There was a problem hiding this comment.
That's a fair concern, and one I agree with. The reason I went with this over a more involved test was because:
- I followed the golang example
- there are only 4 results in the first page, so the second page is technically empty.
Do you have a better way in mind to test it? I could check for an empty list - would that be preferred? I decided against it at the time, mostly because there are similar issues with some snippets in this folder and figured it would be best just to wait until they are modified to fit the new sample format.
There was a problem hiding this comment.
figured it would be best just to wait until they are modified to fit the new sample format.
That might be the answer for now, depending on how soon all these samples are going to be updated. Are there any queries that are long enough to be paginated? Or can we just reduce the page element count to force the pagination even if it's not practical to get a page of 2 results?
In any case, I'd like it to at least write its findings to stdout, if not return them to be verified by the test.
There was a problem hiding this comment.
I've updated the snippet slightly to return the queries and rerun for the tests.
| .get(); | ||
|
|
||
| // Wait for results. | ||
| List<QueryDocumentSnapshot> docs = firstPage.get(30, TimeUnit.SECONDS).getDocuments(); |
There was a problem hiding this comment.
This is performing another get? I'm a little confused by snippet given it calls get: one here and another on the previous line.
There was a problem hiding this comment.
The first .get() makes an async API call and returns a future that represents the results. The second .get represents blocking until the results are ready, and returns the result of the first API call as a value.
| // [START fs_document_snapshot_cursor] | ||
| // Fetch the Snapshot | ||
| ApiFuture<DocumentSnapshot> future = db.collection("cities").document("SF").get(); | ||
| DocumentSnapshot snapshot = future.get(30, TimeUnit.SECONDS); |
There was a problem hiding this comment.
What does the magic number represent other than 30 seconds?
There was a problem hiding this comment.
This is the amount of time to block for results -- if this limit is exceeded, an exception is thrown.
There was a problem hiding this comment.
Could you use a variable name instead of a magic number to state the same information?
There was a problem hiding this comment.
The number isn't magical, it is just a safe amount of time. I don't think a variable would make sense in the context, since the value depends on the users situation and is only used once. I have updated with a comment to better explain what the future is doing.
f10c2e2 to
d810015
Compare
| // [START fs_document_snapshot_cursor] | ||
| // Fetch the Snapshot | ||
| ApiFuture<DocumentSnapshot> future = db.collection("cities").document("SF").get(); | ||
| DocumentSnapshot snapshot = future.get(30, TimeUnit.SECONDS); |
Add Java snippets for the Paginating Data with Query Cursors query.