feat: add API to serialize Queries to Proto #241
Merged
+553
−162
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
googlebot
added
the
cla: yes
schmidt-sebastian
force-pushed
the
mrschmidt/serializequery
branch
from
1660d20
to
296928e
Compare
Codecov Report
@@ Coverage Diff @@
## master #241 +/- ##
============================================
- Coverage 73.01% 72.99% -0.03%
- Complexity 1036 1044 +8
============================================
Files 64 64
Lines 5444 5525 +81
Branches 618 645 +27
============================================
+ Hits 3975 4033 +58
- Misses 1271 1281 +10
- Partials 198 211 +13
Continue to review full report at Codecov.
|
schmidt-sebastian
force-pushed
the
mrschmidt/serializequery
branch
from
296928e
to
3c6e051
Compare
|
@BenWhitehead This is now ready for review. |
wilhuff
reviewed
Jun 7, 2020
| * | ||
| * @return the serialized RunQueryRequest | ||
| */ | ||
| public RunQueryRequest serializeToProto() { |
There was a problem hiding this comment.
The internal convention seems to be toProto and fromProto, which seems perfectly fine. Why the extra long name here?
There was a problem hiding this comment.
No strong opinion here. Renamed.
schmidt-sebastian
changed the title
schmidt-sebastian
changed the title
|
@BenWhitehead Do we have a way forward regarding the |
|
I've just pushed a new commit that refactors things so we don't have to depend on FirestoreImpl as the input and allows for |
google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/DocumentReference.java
Show resolved
Hide resolved
google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java
Show resolved
Hide resolved
|
|
||
| @InternalApi | ||
| @InternalExtensionOnly | ||
| interface FirestoreRpcContext<FS extends Firestore> { |
There was a problem hiding this comment.
My takeaway from our discussion last week that that this was intended to be public. Let me know if I misunderstood you :)
There was a problem hiding this comment.
I think that was my original intent, but after poking at things and seeing the amount of interconnectedness between the data and the operations I think we need to scope this to the level FirestoreImpl currently is. If this interface were as simple as getDatabseName() and getResourcePath() I think it'd be easier for us to have it public, but there is a lot of coupling against these methods that we don't want on the public api surface.
BenWhitehead
force-pushed
the
mrschmidt/serializequery
branch
from
e46bd2b
to
c4ce2a3
Compare
### Feature Add new methods `toProto` and `fromProto` to Query, allowing serialization to and deserialization from RunQueryRequest. ### Refactoring FieldFilter in Query has had a `fromProto` defined on it. Properties in FieldFilter and corresponding subclasses have been refactored to use proto types internally rather than previously using high level api types. Add a new interface FirestoreRpcContext which defines all the methods needed by model classes that were only provided by FirestoreImpl. FirestoreImpl now implements FirestoreRpcContext, and the new interface methods are all the same implementation as before. All dependency on FirestoreImpl from the following classes has been replaced with FirestoreRpcContext: * CollectionReference * DocumentReference * DocumentSnapshot * Query * QueryDocumentSnapshot Motivation for this change is due to the fact that we want to expose a new method on Query `fromProto` to allow a Query to be loaded from a proto. However, Query requires additional context & access to be able execute queries and thus has a dependency on a Firestore instance as well as those methods now defined in FirestoreRpcContext. Due to the constraints of Java's type system, the constraint on the instance passed to `fromProto` is runtime checked versus compile time checked. The method signature accepts any `Firestore` instance, but we assert that the instance is also a `FirestoreRpcContext`. This isn't ideal as it is a runtime error when not satisfied. However, Firestore is annotated @InternalExtensionOnly so we have policy control that the instance returned from FirestoreOptions will always also implement FirestoreRpcContext. A test has been added to verify that a proxy instance of FirestoreImpl could satisfy the compile and runtime checks present such that use should allow the code to work with java ee container based approaches using dynamic class proxies for things like instrumentation and dependency injection. As part of the change, the constructors for the above mentioned model classes have had their scope narrowed to package private from protected. The classes themselves are now also annotated with @InternalExtensionOnly along with the note in their Javadoc stating the policy. clirr rules have been added to ignore the constructor changes in the model classes. Co-authored-by: Sebastian Schmidt <mrschmidt@google.com> Co-authored-by: BenWhitehead <BenWhitehead@users.noreply.github.com>
BenWhitehead
force-pushed
the
mrschmidt/serializequery
branch
from
c4ce2a3
to
62f2866
Compare
gcf-merge-on-green bot
pushed a commit
that referenced
this pull request
Jun 17, 2020
🤖 I have created a release \*beep\* \*boop\* --- ## [1.35.0](https://www.github.com/googleapis/java-firestore/compare/v1.34.0...v1.35.0) (2020-06-17) ### Features * ability to serialize Query to Proto ([#241](https://www.github.com/googleapis/java-firestore/issues/241)) ([bae22e0](https://www.github.com/googleapis/java-firestore/commit/bae22e0839de55e11dda604c3034feaedbbc172a)) * add support for fieldmask to document reference ([#245](https://www.github.com/googleapis/java-firestore/issues/245)) ([4a846b1](https://www.github.com/googleapis/java-firestore/commit/4a846b1f067ad8e462df673ada38589da224fcef)) ### Dependencies * update core dependencies ([#254](https://www.github.com/googleapis/java-firestore/issues/254)) ([9b275ca](https://www.github.com/googleapis/java-firestore/commit/9b275ca5b3f2adbe18be77ea8c86d8446a5833d6)) * update dependency com.google.api:api-common to v1.9.2 ([#238](https://www.github.com/googleapis/java-firestore/issues/238)) ([c47d327](https://www.github.com/googleapis/java-firestore/commit/c47d32705645a76d8f9598aa954dbc3b1c067c73)) * update dependency io.grpc:grpc-bom to v1.30.0 ([#244](https://www.github.com/googleapis/java-firestore/issues/244)) ([b5749d4](https://www.github.com/googleapis/java-firestore/commit/b5749d4e9bac3628da66451fa070c1bf6f852614)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds an API to serialize and deserialize Queries to and from RunQueryRequests. The new API endpoints are
Query.toProtoandQuery.fromProto.