feat: Implementation of Firestore Bundle Builder by wu-hui · Pull Request #293 · googleapis/java-firestore

wu-hui · 2020-07-11T02:00:50Z

No description provided.

codecov · 2020-07-11T02:02:34Z

Codecov Report

Merging #293 into master will increase coverage by 0.91%.
The diff coverage is 91.50%.

@@ Coverage Diff @@## master #293 +/- ##
============================================
+ Coverage 72.37% 73.29% +0.91% + Complexity 1099 1065 -34 
============================================
Files 67 65 -2 Lines 5814 5737 -77 Branches 742 717 -25 ============================================
- Hits 4208 4205 -3 + Misses 1351 1315 -36 + Partials 255 217 -38

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/com/google/cloud/firestore/Query.java	`88.10% <85.36%> (+0.30%)`	`131.00 <6.00> (-4.00)`	⬆️
...va/com/google/cloud/firestore/FirestoreBundle.java	`93.33% <93.33%> (ø)`	`3.00 <3.00> (?)`
...a/com/google/cloud/firestore/DocumentSnapshot.java	`81.72% <100.00%> (+2.65%)`	`39.00 <1.00> (+2.00)`
...n/java/com/google/cloud/firestore/RateLimiter.java	`100.00% <0.00%> (ø)`	`15.00% <0.00%> (+2.00%)`
.../cloud/firestore/AutoValue_Query_QueryOptions.java
...google/cloud/firestore/AutoValue_ResourcePath.java
...om/google/cloud/firestore/AutoValue_FieldPath.java
...java/com/google/cloud/firestore/FirestoreImpl.java	`77.61% <0.00%> (+0.74%)`	`27.00% <0.00%> (+2.00%)`
...a/com/google/cloud/firestore/FirestoreOptions.java	`35.89% <0.00%> (+0.85%)`	`7.00% <0.00%> (ø%)`
... and 4 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 5444fed...10b7384. Read the comment docs.

# Conflicts: # .kokoro/dependencies.sh # .kokoro/nightly/samples.cfg # .kokoro/presubmit/samples.cfg # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1/FirestoreClient.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1/FirestoreSettings.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1/stub/FirestoreStub.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1/stub/FirestoreStubSettings.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1/stub/GrpcFirestoreStub.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1beta1/FirestoreClient.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1beta1/FirestoreSettings.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1beta1/package-info.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1beta1/stub/FirestoreStub.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1beta1/stub/FirestoreStubSettings.java # google-cloud-firestore/src/main/java/com/google/cloud/firestore/v1beta1/stub/GrpcFirestoreStub.java # google-cloud-firestore/src/test/java/com/google/cloud/firestore/v1/FirestoreClientTest.java # google-cloud-firestore/src/test/java/com/google/cloud/firestore/v1beta1/FirestoreClientTest.java # google-cloud-firestore/src/test/java/com/google/cloud/firestore/v1beta1/MockFirestoreImpl.java # proto-google-cloud-firestore-v1/src/main/java/com/google/firestore/v1/QueryProto.java # proto-google-cloud-firestore-v1/src/main/java/com/google/firestore/v1/StructuredQuery.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ArrayValue.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/BatchGetDocumentsRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/BatchGetDocumentsResponse.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/BeginTransactionRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/BeginTransactionResponse.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/CommitRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/CommitResponse.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/CommonProto.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/CreateDocumentRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/Cursor.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/DeleteDocumentRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/Document.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/DocumentChange.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/DocumentDelete.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/DocumentProto.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/DocumentRemove.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/DocumentTransform.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ExistenceFilter.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/FirestoreProto.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/GetDocumentRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ListCollectionIdsRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ListCollectionIdsResponse.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ListDocumentsRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ListDocumentsResponse.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ListenRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/ListenResponse.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/MapValue.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/Precondition.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/QueryProto.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/RollbackRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/RunQueryRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/RunQueryResponse.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/StructuredQuery.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/Target.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/TargetChange.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/TransactionOptions.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/UpdateDocumentRequest.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/Value.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/Write.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/WriteProto.java # proto-google-cloud-firestore-v1beta1/src/main/java/com/google/firestore/v1beta1/WriteResult.java # synth.metadata # synth.py

BenWhitehead · 2020-09-10T19:52:01Z

 xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
 <modelVersion>4.0.0</modelVersion>
-
+ <groupId>com.google.firestore</groupId>


I'm not sure release automation will be able to write to the group com.google.firestore is it possible for the groupId of this artifact to stay com.google.cloud?

BenWhitehead · 2020-09-10T20:21:21Z

+ }
+
+ @Test
+ public void testBuildingBundleWithLimitAndLimitToLastQueries() throws Exception {


Can we split this test into two, one for the limit First, and one for limit last? If either case ever fails it'd be nice to tell independently which is failing.

My thought was this provides a good contrast and helps reader understand two cases.
I don't feel strong, I can split if you insist.

If the code overhead is minimal I'd prefer the two separate tests for clarity of individual scenarios.

BenWhitehead · 2020-09-10T20:37:54Z

+ elements.get(1).getDocumentMetadata(),
+ elements.get(2).getDocument(),
+ DOCUMENT_NAME,
+ Lists.newArrayList(),


Same as above
Suggested change
Lists.newArrayList(),
Lists.<String>newArrayList(),

BenWhitehead · 2020-09-10T20:40:32Z

+ if (originalDocument == null
+ || documentSnapshot
+ .getReadTime()
+ .compareTo(Timestamp.fromProto(originalDocument.getMetadata().getReadTime()))


This call to compare to should guard against getReadTime() returning null, since there doesn't appear to be a previous check ensuring it's non-null.

I gave it a try, and it looks awkward. I am inclining to not check it here, because the only place setting originalDocument is below, and the need to defend against that is not very strong IMHO.

The compareTo method on Timestamp expects a non-null argument (and doesn't assert it unfortunately) and there is the possibility that getReadTime() returns a null value, so we do in fact need to guard this check so that it doesn't result in an exception.
A similar scenario is present below on line 88 (in this commit) where it's possible a null value is returned before calling toProto().

# Conflicts: # google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java # proto-google-cloud-firestore-bundle-v1/pom.xml # synth.metadata # synth.py

wu-hui

Thanks for the review!

It is now updated with bundle.proto from googleapis/third_party as well.

wu-hui · 2020-10-09T02:01:52Z

+ if (originalDocument == null
+ || documentSnapshot
+ .getReadTime()
+ .compareTo(Timestamp.fromProto(originalDocument.getMetadata().getReadTime()))


I gave it a try, and it looks awkward. I am inclining to not check it here, because the only place setting originalDocument is below, and the need to defend against that is not very strong IMHO.

wu-hui · 2020-10-09T02:08:45Z

+ elements.get(1).getDocumentMetadata(),
+ elements.get(2).getDocument(),
+ DOCUMENT_NAME,
+ Lists.newArrayList(),


wu-hui · 2020-10-09T02:11:32Z

+ }
+
+ @Test
+ public void testBuildingBundleWithLimitAndLimitToLastQueries() throws Exception {


My thought was this provides a good contrast and helps reader understand two cases.
I don't feel strong, I can split if you insist.

BenWhitehead · 2020-10-14T17:37:41Z

+ <artifactId>proto-google-cloud-firestore-bundle-v1</artifactId>
+ <version>2.1.1-SNAPSHOT</version>
+ <scope>compile</scope>
+ </dependency>


Due to some overarching complexities in how our poms get built, bundled and released with the broader GCP libraries this declaration needs to change a little bit. I'll fix up all the pom files and push a commit to this branch with all the necessary changes, we can then review if you're interested in the details.

…le-cloud-firestore-bundle-v1 artifact

BenWhitehead

A couple small things, and the remaining comment about getReadTime possibly causing an exception.

BenWhitehead · 2020-10-14T17:39:04Z

+/** Represents a Firestore data bundle with results from the given document and query snapshots. */
+public final class FirestoreBundle {
+
+ static int BUNDLE_SCHEMA_VERSION = 1;


Suggested change
staticintBUNDLE_SCHEMA_VERSION = 1;
staticfinalintBUNDLE_SCHEMA_VERSION = 1;

BenWhitehead · 2020-10-14T17:39:56Z

+
+ static int BUNDLE_SCHEMA_VERSION = 1;
+ // Printer to encode protobuf objects into JSON string.
+ private static JsonFormat.Printer PRINTER = JsonFormat.printer();


Suggested change
privatestaticJsonFormat.PrinterPRINTER = JsonFormat.printer();
privatestaticfinalJsonFormat.PrinterPRINTER = JsonFormat.printer();
Constants should always be final so they can't be modified.

BenWhitehead · 2020-10-14T17:48:14Z

+ if (originalDocument == null
+ || documentSnapshot
+ .getReadTime()
+ .compareTo(Timestamp.fromProto(originalDocument.getMetadata().getReadTime()))


The compareTo method on Timestamp expects a non-null argument (and doesn't assert it unfortunately) and there is the possibility that getReadTime() returns a null value, so we do in fact need to guard this check so that it doesn't result in an exception.
A similar scenario is present below on line 88 (in this commit) where it's possible a null value is returned before calling toProto().

…y/BundleBuilder

wu-hui added 3 commits June 22, 2020 13:33

Add bundle.proto.
9d69eb5

Generated from proto.
096d267

Reformatted.
5d12efa

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 11, 2020

wu-hui requested a review from BenWhitehead July 11, 2020 02:01

wu-hui assigned BenWhitehead Jul 11, 2020

BenWhitehead added this to the v2 milestone Jul 13, 2020

BenWhitehead added the status: blocked Resolving the issue is dependent on other work. label Jul 13, 2020

BenWhitehead removed the status: blocked Resolving the issue is dependent on other work. label Jul 21, 2020

BenWhitehead removed this from the v2 milestone Aug 11, 2020

product-auto-labelBot added the api: firestore Issues related to the googleapis/java-firestore API. label Aug 21, 2020

wu-hui added 8 commits August 24, 2020 09:53

Manual changes to add proto, build rule and dependencies.
b35c1df

Auto generated contents
5151105

Remove unused dependencies.
9032020

Temp commit.
90a2d51

Implementation with unit tests.
48fac25

Add system tests and comments.
91ea0ca

Builds and tests pass after rebasing.
4124116

wu-hui force-pushed the wuandy/BundleBuilder branch from 76e5416 to 4124116Compare August 24, 2020 17:43

wu-hui unassigned BenWhitehead Aug 24, 2020

Build with query document mapping and always newest doc
1ac8a88

wu-hui assigned BenWhitehead Aug 24, 2020

BenWhitehead suggested changes Sep 10, 2020
View reviewed changes

Base automatically changed from wuandy/BundleProto to masterSeptember 18, 2020 14:10

wu-hui added 2 commits October 8, 2020 22:13

Address comments
fd7003d

Merge branch 'master' into wuandy/BundleBuilder
4597efd
# Conflicts: # google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java # proto-google-cloud-firestore-bundle-v1/pom.xml # synth.metadata # synth.py

wu-hui requested review from a teamOctober 9, 2020 02:22

wu-hui requested a review from a teamOctober 9, 2020 02:22

wu-hui commented Oct 9, 2020
View reviewed changes

Delete update.sh
def3a98

wu-hui assigned BenWhitehead and unassigned BenWhitehead Oct 9, 2020

Delete obselete dependency
376763c

BenWhitehead reviewed Oct 14, 2020
View reviewed changes

build: fixup poms for google-cloud-firestore accessing the proto-goog…
f56fe62
…le-cloud-firestore-bundle-v1 artifact

BenWhitehead suggested changes Oct 14, 2020
View reviewed changes

wu-huiand others added 3 commits October 14, 2020 15:10

Address comments
f966428

Merge remote-tracking branch 'origin/wuandy/BundleBuilder' into wuand…
1ccb5f1
…y/BundleBuilder

chore: fix dependency:analyze build
10b7384

BenWhitehead approved these changes Oct 16, 2020
View reviewed changes

BenWhitehead added the automerge Merge the pull request once unit tests and other checks pass. label Oct 16, 2020

gcf-merge-on-greenBot merged commit fd5ef90 into masterOct 16, 2020

gcf-merge-on-greenBot deleted the wuandy/BundleBuilder branch October 16, 2020 20:16

gcf-merge-on-greenBot removed the automerge Merge the pull request once unit tests and other checks pass. label Oct 16, 2020

	staticintBUNDLE_SCHEMA_VERSION = 1;
	staticfinalintBUNDLE_SCHEMA_VERSION = 1;

	privatestaticJsonFormat.PrinterPRINTER = JsonFormat.printer();
	privatestaticfinalJsonFormat.PrinterPRINTER = JsonFormat.printer();

Uh oh!

Conversation

wu-hui commented Jul 11, 2020

Uh oh!

codecovBot commented Jul 11, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

BenWhitehead 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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecovBot commented Jul 11, 2020•
edited
Loading