Skip to content

Commit

Permalink
feat: ability to serialize Query to Proto (#241)
Browse files Browse the repository at this point in the history
### 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>
  • Loading branch information
schmidt-sebastian and BenWhitehead committed Jun 16, 2020
1 parent bde9643 commit bae22e0
Show file tree
Hide file tree
Showing 12 changed files with 553 additions and 162 deletions.
13 changes: 13 additions & 0 deletions google-cloud-firestore/clirr-ignored-differences.xml
Expand Up @@ -23,4 +23,17 @@
<className>com/google/cloud/firestore/Firestore</className>
<method>com.google.api.core.ApiFuture runAsyncTransaction(*)</method>
</difference>

<difference>
<differenceType>7005</differenceType>
<className>com/google/cloud/firestore/*</className>
<method>*(com.google.cloud.firestore.FirestoreImpl, *)</method>
<to>*(com.google.cloud.firestore.FirestoreRpcContext, *)</to>
</difference>
<difference>
<differenceType>7009</differenceType>
<className>com/google/cloud/firestore/*</className>
<method>*(com.google.cloud.firestore.FirestoreImpl, *)</method>
</difference>

</differences>
Expand Up @@ -19,8 +19,11 @@
import com.google.api.core.ApiFunction;
import com.google.api.core.ApiFuture;
import com.google.api.core.ApiFutures;
import com.google.api.core.InternalExtensionOnly;
import com.google.api.gax.rpc.ApiException;
import com.google.api.gax.rpc.ApiExceptions;
import com.google.api.gax.rpc.UnaryCallable;
import com.google.cloud.firestore.spi.v1.FirestoreRpc;
import com.google.cloud.firestore.v1.FirestoreClient.ListDocumentsPagedResponse;
import com.google.common.base.Preconditions;
import com.google.common.util.concurrent.MoreExecutors;
Expand All @@ -40,16 +43,17 @@
* test mocks. Subclassing is not supported in production code and new SDK releases may break code
* that does so.
*/
@InternalExtensionOnly
public class CollectionReference extends Query {

/**
* Creates a CollectionReference from a complete collection path.
*
* @param firestore The Firestore client.
* @param rpcContext The Firestore client.
* @param collectionPath The Path of this collection.
*/
protected CollectionReference(FirestoreImpl firestore, ResourcePath collectionPath) {
super(firestore, collectionPath);
CollectionReference(FirestoreRpcContext<?> rpcContext, ResourcePath collectionPath) {
super(rpcContext, collectionPath);
}

/**
Expand All @@ -71,7 +75,7 @@ public String getId() {
@Nullable
public DocumentReference getParent() {
ResourcePath parent = options.getParentPath();
return parent.isDocument() ? new DocumentReference(firestore, parent) : null;
return parent.isDocument() ? new DocumentReference(rpcContext, parent) : null;
}

/**
Expand Down Expand Up @@ -109,7 +113,7 @@ public DocumentReference document(@Nonnull String childPath) {
Preconditions.checkArgument(
documentPath.isDocument(),
String.format("Path should point to a Document Reference: %s", getPath()));
return new DocumentReference(firestore, documentPath);
return new DocumentReference(rpcContext, documentPath);
}

/**
Expand All @@ -133,10 +137,12 @@ public Iterable<DocumentReference> listDocuments() {
final ListDocumentsPagedResponse response;

try {
response =
ApiExceptions.callAndTranslateApiException(
firestore.sendRequest(
request.build(), firestore.getClient().listDocumentsPagedCallable()));
FirestoreRpc client = rpcContext.getClient();
UnaryCallable<ListDocumentsRequest, ListDocumentsPagedResponse> callable =
client.listDocumentsPagedCallable();
ListDocumentsRequest build = request.build();
ApiFuture<ListDocumentsPagedResponse> future = rpcContext.sendRequest(build, callable);
response = ApiExceptions.callAndTranslateApiException(future);
} catch (ApiException exception) {
throw FirestoreException.apiException(exception);
}
Expand Down
Expand Up @@ -19,6 +19,7 @@
import com.google.api.core.ApiFunction;
import com.google.api.core.ApiFuture;
import com.google.api.core.ApiFutures;
import com.google.api.core.InternalExtensionOnly;
import com.google.api.gax.rpc.ApiException;
import com.google.api.gax.rpc.ApiExceptions;
import com.google.cloud.firestore.v1.FirestoreClient.ListCollectionIdsPagedResponse;
Expand All @@ -42,15 +43,16 @@
* test mocks. Subclassing is not supported in production code and new SDK releases may break code
* that does so.
*/
@InternalExtensionOnly
public class DocumentReference {

private final ResourcePath path;
private final FirestoreImpl firestore;
private final FirestoreRpcContext<?> rpcContext;

protected DocumentReference(
FirestoreImpl firestore, ResourcePath path) { // Elevated access level for mocking.
DocumentReference(
FirestoreRpcContext<?> rpcContext, ResourcePath path) { // Elevated access level for mocking.
this.path = path;
this.firestore = firestore;
this.rpcContext = rpcContext;
}

/*
Expand All @@ -60,7 +62,7 @@ protected DocumentReference(
*/
@Nonnull
public Firestore getFirestore() {
return firestore;
return rpcContext.getFirestore();
}

/**
Expand Down Expand Up @@ -102,7 +104,7 @@ String getName() {
*/
@Nonnull
public CollectionReference getParent() {
return new CollectionReference(firestore, path.getParent());
return new CollectionReference(rpcContext, path.getParent());
}

/**
Expand All @@ -114,7 +116,7 @@ public CollectionReference getParent() {
*/
@Nonnull
public CollectionReference collection(@Nonnull String collectionPath) {
return new CollectionReference(firestore, path.append(collectionPath));
return new CollectionReference(rpcContext, path.append(collectionPath));
}

/**
Expand Down Expand Up @@ -144,7 +146,7 @@ public T apply(List<T> results) {
*/
@Nonnull
public ApiFuture<WriteResult> create(@Nonnull Map<String, Object> fields) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.create(this, fields).commit());
}

Expand All @@ -157,7 +159,7 @@ public ApiFuture<WriteResult> create(@Nonnull Map<String, Object> fields) {
*/
@Nonnull
public ApiFuture<WriteResult> create(@Nonnull Object pojo) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.create(this, pojo).commit());
}

Expand All @@ -170,7 +172,7 @@ public ApiFuture<WriteResult> create(@Nonnull Object pojo) {
*/
@Nonnull
public ApiFuture<WriteResult> set(@Nonnull Map<String, Object> fields) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.set(this, fields).commit());
}

Expand All @@ -186,7 +188,7 @@ public ApiFuture<WriteResult> set(@Nonnull Map<String, Object> fields) {
@Nonnull
public ApiFuture<WriteResult> set(
@Nonnull Map<String, Object> fields, @Nonnull SetOptions options) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.set(this, fields, options).commit());
}

Expand All @@ -199,7 +201,7 @@ public ApiFuture<WriteResult> set(
*/
@Nonnull
public ApiFuture<WriteResult> set(@Nonnull Object pojo) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.set(this, pojo).commit());
}

Expand All @@ -214,7 +216,7 @@ public ApiFuture<WriteResult> set(@Nonnull Object pojo) {
*/
@Nonnull
public ApiFuture<WriteResult> set(@Nonnull Object pojo, @Nonnull SetOptions options) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.set(this, pojo, options).commit());
}

Expand All @@ -227,7 +229,7 @@ public ApiFuture<WriteResult> set(@Nonnull Object pojo, @Nonnull SetOptions opti
*/
@Nonnull
public ApiFuture<WriteResult> update(@Nonnull Map<String, Object> fields) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.update(this, fields).commit());
}

Expand All @@ -241,7 +243,7 @@ public ApiFuture<WriteResult> update(@Nonnull Map<String, Object> fields) {
*/
@Nonnull
public ApiFuture<WriteResult> update(@Nonnull Map<String, Object> fields, Precondition options) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.update(this, fields, options).commit());
}

Expand All @@ -257,7 +259,7 @@ public ApiFuture<WriteResult> update(@Nonnull Map<String, Object> fields, Precon
@Nonnull
public ApiFuture<WriteResult> update(
@Nonnull String field, @Nullable Object value, Object... moreFieldsAndValues) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.update(this, field, value, moreFieldsAndValues).commit());
}

Expand All @@ -273,7 +275,7 @@ public ApiFuture<WriteResult> update(
@Nonnull
public ApiFuture<WriteResult> update(
@Nonnull FieldPath fieldPath, @Nullable Object value, Object... moreFieldsAndValues) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.update(this, fieldPath, value, moreFieldsAndValues).commit());
}

Expand All @@ -293,7 +295,7 @@ public ApiFuture<WriteResult> update(
@Nonnull String field,
@Nullable Object value,
Object... moreFieldsAndValues) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(
writeBatch.update(this, options, field, value, moreFieldsAndValues).commit());
}
Expand All @@ -314,7 +316,7 @@ public ApiFuture<WriteResult> update(
@Nonnull FieldPath fieldPath,
@Nullable Object value,
Object... moreFieldsAndValues) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(
writeBatch.update(this, options, fieldPath, value, moreFieldsAndValues).commit());
}
Expand All @@ -327,7 +329,7 @@ public ApiFuture<WriteResult> update(
*/
@Nonnull
public ApiFuture<WriteResult> delete(@Nonnull Precondition options) {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.delete(this, options).commit());
}

Expand All @@ -338,7 +340,7 @@ public ApiFuture<WriteResult> delete(@Nonnull Precondition options) {
*/
@Nonnull
public ApiFuture<WriteResult> delete() {
WriteBatch writeBatch = firestore.batch();
WriteBatch writeBatch = rpcContext.getFirestore().batch();
return extractFirst(writeBatch.delete(this).commit());
}

Expand All @@ -351,7 +353,7 @@ public ApiFuture<WriteResult> delete() {
*/
@Nonnull
public ApiFuture<DocumentSnapshot> get() {
return extractFirst(firestore.getAll(this));
return extractFirst(rpcContext.getFirestore().getAll(this));
}

/**
Expand All @@ -364,7 +366,8 @@ public ApiFuture<DocumentSnapshot> get() {
*/
@Nonnull
public ApiFuture<DocumentSnapshot> get(FieldMask fieldMask) {
return extractFirst(firestore.getAll(new DocumentReference[] {this}, fieldMask));
return extractFirst(
rpcContext.getFirestore().getAll(new DocumentReference[] {this}, fieldMask));
}

/**
Expand All @@ -382,8 +385,8 @@ public Iterable<CollectionReference> listCollections() {
try {
response =
ApiExceptions.callAndTranslateApiException(
firestore.sendRequest(
request.build(), firestore.getClient().listCollectionIdsPagedCallable()));
rpcContext.sendRequest(
request.build(), rpcContext.getClient().listCollectionIdsPagedCallable()));
} catch (ApiException exception) {
throw FirestoreException.apiException(exception);
}
Expand Down Expand Up @@ -456,7 +459,7 @@ public void onEvent(
}
listener.onEvent(
DocumentSnapshot.fromMissing(
firestore, DocumentReference.this, value.getReadTime()),
rpcContext, DocumentReference.this, value.getReadTime()),
null);
}
});
Expand All @@ -471,7 +474,7 @@ public void onEvent(
@Nonnull
public ListenerRegistration addSnapshotListener(
@Nonnull EventListener<DocumentSnapshot> listener) {
return addSnapshotListener(firestore.getClient().getExecutor(), listener);
return addSnapshotListener(rpcContext.getClient().getExecutor(), listener);
}

ResourcePath getResourcePath() {
Expand All @@ -498,11 +501,11 @@ public boolean equals(Object obj) {
return false;
}
DocumentReference that = (DocumentReference) obj;
return Objects.equals(path, that.path) && Objects.equals(firestore, that.firestore);
return Objects.equals(path, that.path) && Objects.equals(rpcContext, that.rpcContext);
}

@Override
public int hashCode() {
return Objects.hash(path, firestore);
return Objects.hash(path, rpcContext);
}
}

0 comments on commit bae22e0

Please sign in to comment.