From a22b2279265c779471ba7738618cbf007afa4054 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 6 Mar 2019 10:23:11 -0800 Subject: [PATCH 1/4] Firestore: Adding Collection Group queries --- .../cloud/firestore/CollectionReference.java | 19 +- .../com/google/cloud/firestore/Firestore.java | 10 + .../google/cloud/firestore/FirestoreImpl.java | 20 +- .../com/google/cloud/firestore/Query.java | 173 ++++++++++-------- .../com/google/cloud/firestore/Watch.java | 2 +- .../cloud/firestore/LocalFirestoreHelper.java | 20 +- .../com/google/cloud/firestore/QueryTest.java | 67 +++++-- .../cloud/firestore/TransactionTest.java | 2 +- .../cloud/firestore/it/ITSystemTest.java | 132 +++++++++++++ 9 files changed, 338 insertions(+), 107 deletions(-) diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java index f6d472a2aeb2..7cb74a9662cc 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java @@ -59,7 +59,7 @@ public class CollectionReference extends Query { */ @Nonnull public String getId() { - return path.getId(); + return options.collectionId; } /** @@ -69,7 +69,7 @@ public String getId() { */ @Nullable public DocumentReference getParent() { - ResourcePath parent = path.getParent(); + ResourcePath parent = options.parentPath; return parent.isDocument() ? new DocumentReference(firestore, parent) : null; } @@ -81,7 +81,7 @@ public DocumentReference getParent() { */ @Nonnull public String getPath() { - return path.getPath(); + return getResourcePath().getPath(); } /** @@ -104,10 +104,10 @@ public DocumentReference document() { */ @Nonnull public DocumentReference document(@Nonnull String childPath) { - ResourcePath documentPath = path.append(childPath); + ResourcePath documentPath = getResourcePath().append(childPath); Preconditions.checkArgument( documentPath.isDocument(), - String.format("Path should point to a Document Reference: %s", path)); + String.format("Path should point to a Document Reference: %s", getPath())); return new DocumentReference(firestore, documentPath); } @@ -124,8 +124,8 @@ public DocumentReference document(@Nonnull String childPath) { @Nonnull public Iterable listDocuments() { ListDocumentsRequest.Builder request = ListDocumentsRequest.newBuilder(); - request.setParent(path.getParent().toString()); - request.setCollectionId(this.getId()); + request.setParent(options.parentPath.toString()); + request.setCollectionId(options.collectionId); request.setMask(DocumentMask.getDefaultInstance()); request.setShowMissing(true); @@ -206,4 +206,9 @@ public ApiFuture add(Object pojo) { } return add((Map) converted); } + + /** Returns a resource path pointing to this collection. */ + ResourcePath getResourcePath() { + return options.parentPath.append(options.collectionId); + } } diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java index 6476c91ba0cd..32d223f99a4d 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Firestore.java @@ -62,6 +62,16 @@ public interface Firestore extends Service, AutoCloseable { @Nonnull Iterable getCollections(); + /** + * Creates and returns a new @link{Query} that includes all documents in the database that are + * contained in a collection or subcollection with the given @code{collectionId}. + * + * @param collectionId Identifies the collections to query over. Every collection or subcollection + * with this ID as the last segment of its path will be included. Cannot contain a slash. + * @return The created Query. + */ + Query collectionGroup(@Nonnull String collectionId); + /** * Executes the given updateFunction and then attempts to commit the changes applied within the * transaction. If any document read within the transaction has changed, the updateFunction will diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java index 3a1c677aaccc..6e568748a870 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/FirestoreImpl.java @@ -106,7 +106,10 @@ public WriteBatch batch() { @Nonnull @Override public CollectionReference collection(@Nonnull String collectionPath) { - return new CollectionReference(this, databasePath.append(collectionPath)); + ResourcePath path = databasePath.append(collectionPath); + Preconditions.checkArgument( + path.isCollection(), "Invalid path specified. Path should point to a collection"); + return new CollectionReference(this, path); } @Nonnull @@ -244,6 +247,16 @@ public void onCompleted() { return futureList; } + @Nonnull + @Override + public Query collectionGroup(@Nonnull final String collectionId) { + Preconditions.checkArgument( + !collectionId.contains("/"), + String.format( + "Invalid collectionId '%s'. Collection IDs must not contain '/'.", collectionId)); + return new Query(this, collectionId); + } + @Nonnull @Override public ApiFuture runTransaction(@Nonnull final Transaction.Function updateFunction) { @@ -401,6 +414,11 @@ String getDatabaseName() { return databasePath.getDatabaseName().toString(); } + /** Returns a path to the Firestore project associated with this client. */ + ResourcePath getResourcePath() { + return databasePath; + } + /** Returns the underlying RPC client. */ FirestoreRpc getClient() { return firestoreClient; diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java index 739f57e14883..ee8141c047cc 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java @@ -34,6 +34,7 @@ import com.google.firestore.v1.RunQueryRequest; import com.google.firestore.v1.RunQueryResponse; import com.google.firestore.v1.StructuredQuery; +import com.google.firestore.v1.StructuredQuery.CollectionSelector; import com.google.firestore.v1.StructuredQuery.CompositeFilter; import com.google.firestore.v1.StructuredQuery.FieldReference; import com.google.firestore.v1.StructuredQuery.Filter; @@ -58,7 +59,6 @@ */ public class Query { - final ResourcePath path; final FirestoreImpl firestore; final QueryOptions options; @@ -178,25 +178,36 @@ Order toProto() { } /** Options that define a Firestore Query. */ - private static class QueryOptions { - - private int limit; - private int offset; - private Cursor startCursor; - private Cursor endCursor; - private List fieldFilters; - private List fieldOrders; - private List fieldProjections; - - QueryOptions() { - limit = -1; - offset = -1; - fieldFilters = new ArrayList<>(); - fieldOrders = new ArrayList<>(); - fieldProjections = new ArrayList<>(); + protected static class QueryOptions { + + ResourcePath parentPath; + String collectionId; + boolean allDescendants; + int limit; + int offset; + @Nullable Cursor startCursor; + @Nullable Cursor endCursor; + List fieldFilters; + List fieldOrders; + List fieldProjections; + + private QueryOptions(ResourcePath parentPath, String collectionId) { + this.parentPath = parentPath; + this.collectionId = collectionId; + this.allDescendants = false; + this.limit = -1; + this.offset = -1; + this.startCursor = null; + this.endCursor = null; + this.fieldFilters = new ArrayList<>(); + this.fieldOrders = new ArrayList<>(); + this.fieldProjections = new ArrayList<>(); } - QueryOptions(QueryOptions options) { + private QueryOptions(QueryOptions options) { + parentPath = options.parentPath; + allDescendants = options.allDescendants; + collectionId = options.collectionId; limit = options.limit; offset = options.offset; startCursor = options.startCursor; @@ -217,12 +228,21 @@ public boolean equals(Object o) { QueryOptions that = (QueryOptions) o; + if (allDescendants != that.allDescendants) { + return false; + } if (limit != that.limit) { return false; } if (offset != that.offset) { return false; } + if (!parentPath.equals(that.parentPath)) { + return false; + } + if (!collectionId.equals(that.collectionId)) { + return false; + } if (startCursor != null ? !startCursor.equals(that.startCursor) : that.startCursor != null) { return false; } @@ -240,7 +260,10 @@ public boolean equals(Object o) { @Override public int hashCode() { - int result = limit; + int result = parentPath.hashCode(); + result = 31 * result + collectionId.hashCode(); + result = 31 * result + (allDescendants ? 1 : 0); + result = 31 * result + limit; result = 31 * result + offset; result = 31 * result + (startCursor != null ? startCursor.hashCode() : 0); result = 31 * result + (endCursor != null ? endCursor.hashCode() : 0); @@ -252,18 +275,20 @@ public int hashCode() { } Query(FirestoreImpl firestore, ResourcePath path) { - this(firestore, path, new QueryOptions()); + this.firestore = firestore; + this.options = new QueryOptions(path.getParent(), path.getId()); } - protected Query( - FirestoreImpl firestore, - ResourcePath path, - QueryOptions queryOptions) { // Elevated access level for mocking. - Preconditions.checkArgument( - path.isCollection(), "Invalid path specified. Path should point to a collection"); + Query(FirestoreImpl firestore, String collectionId) { + QueryOptions queryOptions = new QueryOptions(firestore.getResourcePath(), collectionId); + queryOptions.allDescendants = true; this.firestore = firestore; - this.path = path; + this.options = queryOptions; + } + + private Query(FirestoreImpl firestore, QueryOptions queryOptions) { + this.firestore = firestore; this.options = queryOptions; } @@ -378,9 +403,14 @@ private Cursor createCursor(List order, Object[] fieldValues, boolea * a DocumentReference that can directly be used in the Query. */ private Object convertReference(Object fieldValue) { + ResourcePath basePath = + options.allDescendants + ? options.parentPath + : options.parentPath.append(options.collectionId); + DocumentReference reference; if (fieldValue instanceof String) { - reference = new DocumentReference(firestore, path.append((String) fieldValue)); + reference = new DocumentReference(firestore, basePath.append((String) fieldValue)); } else if (fieldValue instanceof DocumentReference) { reference = (DocumentReference) fieldValue; } else { @@ -389,13 +419,14 @@ private Object convertReference(Object fieldValue) { + "DocumentReference."); } - if (!this.path.isPrefixOf(reference.getResourcePath())) { + if (!basePath.isPrefixOf(reference.getResourcePath())) { throw new IllegalArgumentException( String.format( "'%s' is not part of the query result set and cannot be used as a query boundary.", reference.getPath())); } - if (!reference.getParent().getResourcePath().equals(this.path)) { + + if (!options.allDescendants && !reference.getParent().getResourcePath().equals(basePath)) { throw new IllegalArgumentException( String.format( "Only a direct child can be used as a query boundary. Found: '%s'", @@ -432,18 +463,14 @@ public Query whereEqualTo(@Nonnull FieldPath fieldPath, @Nullable Object value) options.startCursor == null && options.endCursor == null, "Cannot call whereEqualTo() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); - QueryOptions newOptions = new QueryOptions(options); if (isUnaryComparison(value)) { + QueryOptions newOptions = new QueryOptions(options); newOptions.fieldFilters.add(new UnaryFilter(fieldPath, value)); + return new Query(firestore, newOptions); } else { - if (fieldPath.equals(FieldPath.DOCUMENT_ID)) { - value = this.convertReference(value); - } - newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, EQUAL, value)); + return whereHelper(fieldPath, EQUAL, value); } - - return new Query(firestore, path, newOptions); } /** @@ -473,9 +500,7 @@ public Query whereLessThan(@Nonnull FieldPath fieldPath, @Nonnull Object value) options.startCursor == null && options.endCursor == null, "Cannot call whereLessThan() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, LESS_THAN, value)); - return new Query(firestore, path, newOptions); + return whereHelper(fieldPath, LESS_THAN, value); } /** @@ -505,9 +530,7 @@ public Query whereLessThanOrEqualTo(@Nonnull FieldPath fieldPath, @Nonnull Objec options.startCursor == null && options.endCursor == null, "Cannot call whereLessThanOrEqualTo() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, LESS_THAN_OR_EQUAL, value)); - return new Query(firestore, path, newOptions); + return whereHelper(fieldPath, LESS_THAN_OR_EQUAL, value); } /** @@ -537,9 +560,7 @@ public Query whereGreaterThan(@Nonnull FieldPath fieldPath, @Nonnull Object valu options.startCursor == null && options.endCursor == null, "Cannot call whereGreaterThan() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, GREATER_THAN, value)); - return new Query(firestore, path, newOptions); + return whereHelper(fieldPath, GREATER_THAN, value); } /** @@ -569,9 +590,7 @@ public Query whereGreaterThanOrEqualTo(@Nonnull FieldPath fieldPath, @Nonnull Ob options.startCursor == null && options.endCursor == null, "Cannot call whereGreaterThanOrEqualTo() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, GREATER_THAN_OR_EQUAL, value)); - return new Query(firestore, path, newOptions); + return whereHelper(fieldPath, GREATER_THAN_OR_EQUAL, value); } /** @@ -607,9 +626,19 @@ public Query whereArrayContains(@Nonnull FieldPath fieldPath, @Nonnull Object va options.startCursor == null && options.endCursor == null, "Cannot call whereArrayContains() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); + return whereHelper(fieldPath, ARRAY_CONTAINS, value); + } + + private Query whereHelper( + FieldPath fieldPath, StructuredQuery.FieldFilter.Operator operator, Object value) { QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, ARRAY_CONTAINS, value)); - return new Query(firestore, path, newOptions); + + if (fieldPath.equals(FieldPath.DOCUMENT_ID)) { + value = this.convertReference(value); + } + + newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, operator, value)); + return new Query(firestore, newOptions); } /** @@ -665,7 +694,7 @@ public Query orderBy(@Nonnull FieldPath fieldPath, @Nonnull Direction direction) QueryOptions newOptions = new QueryOptions(options); newOptions.fieldOrders.add(new FieldOrder(fieldPath, direction)); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -679,7 +708,7 @@ public Query orderBy(@Nonnull FieldPath fieldPath, @Nonnull Direction direction) public Query limit(int limit) { QueryOptions newOptions = new QueryOptions(options); newOptions.limit = limit; - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -692,7 +721,7 @@ public Query limit(int limit) { public Query offset(int offset) { QueryOptions newOptions = new QueryOptions(options); newOptions.offset = offset; - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -708,7 +737,7 @@ public Query startAt(@Nonnull DocumentSnapshot snapshot) { QueryOptions newOptions = new QueryOptions(options); newOptions.fieldOrders = createImplicitOrderBy(); newOptions.startCursor = createCursor(newOptions.fieldOrders, snapshot, true); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -722,7 +751,7 @@ public Query startAt(@Nonnull DocumentSnapshot snapshot) { public Query startAt(Object... fieldValues) { QueryOptions newOptions = new QueryOptions(options); newOptions.startCursor = createCursor(newOptions.fieldOrders, fieldValues, true); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -767,7 +796,7 @@ public Query select(FieldPath... fieldPaths) { newOptions.fieldProjections.add(fieldReference); } - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -783,7 +812,7 @@ public Query startAfter(@Nonnull DocumentSnapshot snapshot) { QueryOptions newOptions = new QueryOptions(options); newOptions.fieldOrders = createImplicitOrderBy(); newOptions.startCursor = createCursor(newOptions.fieldOrders, snapshot, false); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -798,7 +827,7 @@ public Query startAfter(@Nonnull DocumentSnapshot snapshot) { public Query startAfter(Object... fieldValues) { QueryOptions newOptions = new QueryOptions(options); newOptions.startCursor = createCursor(newOptions.fieldOrders, fieldValues, false); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -814,7 +843,7 @@ public Query endBefore(@Nonnull DocumentSnapshot snapshot) { QueryOptions newOptions = new QueryOptions(options); newOptions.fieldOrders = createImplicitOrderBy(); newOptions.endCursor = createCursor(newOptions.fieldOrders, snapshot, true); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -829,7 +858,7 @@ public Query endBefore(@Nonnull DocumentSnapshot snapshot) { public Query endBefore(Object... fieldValues) { QueryOptions newOptions = new QueryOptions(options); newOptions.endCursor = createCursor(newOptions.fieldOrders, fieldValues, true); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -843,7 +872,7 @@ public Query endBefore(Object... fieldValues) { public Query endAt(Object... fieldValues) { QueryOptions newOptions = new QueryOptions(options); newOptions.endCursor = createCursor(newOptions.fieldOrders, fieldValues, false); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** @@ -859,14 +888,16 @@ public Query endAt(@Nonnull DocumentSnapshot snapshot) { QueryOptions newOptions = new QueryOptions(options); newOptions.fieldOrders = createImplicitOrderBy(); newOptions.endCursor = createCursor(newOptions.fieldOrders, snapshot, false); - return new Query(firestore, path, newOptions); + return new Query(firestore, newOptions); } /** Build the final Firestore query. */ StructuredQuery.Builder buildQuery() { StructuredQuery.Builder structuredQuery = StructuredQuery.newBuilder(); - structuredQuery.addFrom( - StructuredQuery.CollectionSelector.newBuilder().setCollectionId(path.getId())); + CollectionSelector.Builder collectionSelector = CollectionSelector.newBuilder(); + collectionSelector.setCollectionId(options.collectionId); + collectionSelector.setAllDescendants(options.allDescendants); + structuredQuery.addFrom(collectionSelector); if (options.fieldFilters.size() == 1) { Filter filter = options.fieldFilters.get(0).toProto(); @@ -963,7 +994,7 @@ Timestamp getReadTime() { private void stream( final QuerySnapshotObserver documentObserver, @Nullable ByteString transactionId) { RunQueryRequest.Builder request = RunQueryRequest.newBuilder(); - request.setStructuredQuery(buildQuery()).setParent(path.getParent().toString()); + request.setStructuredQuery(buildQuery()).setParent(options.parentPath.toString()); if (transactionId != null) { request.setTransaction(transactionId); @@ -1135,10 +1166,6 @@ public int compare(QueryDocumentSnapshot doc1, QueryDocumentSnapshot doc2) { }; } - ResourcePath getResourcePath() { - return path; - } - /** * Returns true if this Query is equal to the provided object. * @@ -1154,13 +1181,11 @@ public boolean equals(Object obj) { return false; } Query query = (Query) obj; - return Objects.equals(path, query.path) - && Objects.equals(firestore, query.firestore) - && Objects.equals(options, query.options); + return Objects.equals(firestore, query.firestore) && Objects.equals(options, query.options); } @Override public int hashCode() { - return Objects.hash(path, firestore, options); + return Objects.hash(firestore, options); } } diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java index 6d937aa2306e..b2e1686627ad 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java @@ -172,7 +172,7 @@ static Watch forQuery(Query query) { target.setQuery( QueryTarget.newBuilder() .setStructuredQuery(query.buildQuery()) - .setParent(query.getResourcePath().getParent().getName()) + .setParent(query.options.parentPath.getName()) .build()); target.setTargetId(WATCH_TARGET_ID); diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java index 718064bc5feb..3379e4a6bda8 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java +++ b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/LocalFirestoreHelper.java @@ -43,6 +43,7 @@ import com.google.firestore.v1.RunQueryRequest; import com.google.firestore.v1.RunQueryResponse; import com.google.firestore.v1.StructuredQuery; +import com.google.firestore.v1.StructuredQuery.CollectionSelector; import com.google.firestore.v1.StructuredQuery.CompositeFilter; import com.google.firestore.v1.StructuredQuery.FieldFilter; import com.google.firestore.v1.StructuredQuery.UnaryFilter; @@ -72,6 +73,7 @@ public final class LocalFirestoreHelper { public static final String DATABASE_NAME; + public static final String COLLECTION_ID; public static final String DOCUMENT_PATH; public static final String DOCUMENT_NAME; public static final ByteString TRANSACTION_ID; @@ -429,16 +431,21 @@ public static StructuredQuery unaryFilter(StructuredQuery.UnaryFilter.Operator o } public static RunQueryRequest query(StructuredQuery... query) { - return query(null, query); + return query(null, false, query); } public static RunQueryRequest query( - @Nullable ByteString transactionId, StructuredQuery... query) { + @Nullable ByteString transactionId, boolean allDescendants, StructuredQuery... query) { RunQueryRequest.Builder request = RunQueryRequest.newBuilder(); request.setParent(LocalFirestoreHelper.DATABASE_NAME + "/documents"); StructuredQuery.Builder structuredQuery = request.getStructuredQueryBuilder(); - structuredQuery.addFrom( - StructuredQuery.CollectionSelector.newBuilder().setCollectionId("coll")); + + CollectionSelector collectionSelector = + CollectionSelector.newBuilder() + .setCollectionId("coll") + .setAllDescendants(allDescendants) + .build(); + structuredQuery.addFrom(collectionSelector); for (StructuredQuery option : query) { structuredQuery.mergeFrom(option); @@ -550,6 +557,10 @@ public static Value object(String key, Value value) { return result.build(); } + public static Value reference(String value) { + return Value.newBuilder().setReferenceValue(value).build(); + } + public static StructuredQuery.FieldReference.Builder field(String fieldPath) { return StructuredQuery.FieldReference.newBuilder().setFieldPath(fieldPath); } @@ -648,6 +659,7 @@ public boolean equals(Object o) { BLOB = Blob.fromBytes(new byte[] {1, 2, 3}); DATABASE_NAME = "projects/test-project/databases/(default)"; + COLLECTION_ID = "coll"; DOCUMENT_PATH = "coll/doc"; DOCUMENT_NAME = DATABASE_NAME + "/documents/" + DOCUMENT_PATH; diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java index 770864f6ee31..173bbbff6ba8 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java +++ b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/QueryTest.java @@ -16,6 +16,7 @@ package com.google.cloud.firestore; +import static com.google.cloud.firestore.LocalFirestoreHelper.COLLECTION_ID; import static com.google.cloud.firestore.LocalFirestoreHelper.DOCUMENT_NAME; import static com.google.cloud.firestore.LocalFirestoreHelper.SINGLE_FIELD_SNAPSHOT; import static com.google.cloud.firestore.LocalFirestoreHelper.endAt; @@ -25,6 +26,7 @@ import static com.google.cloud.firestore.LocalFirestoreHelper.order; import static com.google.cloud.firestore.LocalFirestoreHelper.query; import static com.google.cloud.firestore.LocalFirestoreHelper.queryResponse; +import static com.google.cloud.firestore.LocalFirestoreHelper.reference; import static com.google.cloud.firestore.LocalFirestoreHelper.select; import static com.google.cloud.firestore.LocalFirestoreHelper.startAt; import static com.google.cloud.firestore.LocalFirestoreHelper.unaryFilter; @@ -72,7 +74,7 @@ public class QueryTest { @Before public void before() { - query = firestoreMock.collection("coll"); + query = firestoreMock.collection(COLLECTION_ID); } @Test @@ -183,11 +185,7 @@ public void withDocumentIdFilter() throws Exception { query.whereEqualTo(FieldPath.documentId(), "doc").get().get(); RunQueryRequest expectedRequest = - query( - filter( - Operator.EQUAL, - "__name__", - Value.newBuilder().setReferenceValue(DOCUMENT_NAME).build())); + query(filter(Operator.EQUAL, "__name__", reference(DOCUMENT_NAME))); assertEquals(expectedRequest, runQuery.getValue()); } @@ -287,8 +285,7 @@ public void withDocumentSnapshotCursor() throws Exception { query.startAt(SINGLE_FIELD_SNAPSHOT).get(); - Value documentBoundary = - Value.newBuilder().setReferenceValue(query.getResourcePath().toString() + "/doc").build(); + Value documentBoundary = reference(DOCUMENT_NAME); RunQueryRequest queryRequest = query( @@ -309,8 +306,7 @@ public void withDocumentIdAndDocumentSnapshotCursor() throws Exception { query.orderBy(FieldPath.documentId()).startAt(SINGLE_FIELD_SNAPSHOT).get(); - Value documentBoundary = - Value.newBuilder().setReferenceValue(query.getResourcePath().toString() + "/doc").build(); + Value documentBoundary = reference(DOCUMENT_NAME); RunQueryRequest queryRequest = query( @@ -331,8 +327,7 @@ public void withExtractedDirectionForDocumentSnapshotCursor() throws Exception { query.orderBy("foo", Query.Direction.DESCENDING).startAt(SINGLE_FIELD_SNAPSHOT).get(); - Value documentBoundary = - Value.newBuilder().setReferenceValue(query.getResourcePath().toString() + "/doc").build(); + Value documentBoundary = reference(DOCUMENT_NAME); RunQueryRequest queryRequest = query( @@ -360,8 +355,7 @@ public void withInequalityFilterForDocumentSnapshotCursor() throws Exception { .startAt(SINGLE_FIELD_SNAPSHOT) .get(); - Value documentBoundary = - Value.newBuilder().setReferenceValue(query.getResourcePath().toString() + "/doc").build(); + Value documentBoundary = reference(DOCUMENT_NAME); RunQueryRequest queryRequest = query( @@ -387,8 +381,7 @@ public void withEqualityFilterForDocumentSnapshotCursor() throws Exception { query.whereEqualTo("foo", "bar").startAt(SINGLE_FIELD_SNAPSHOT).get(); - Value documentBoundary = - Value.newBuilder().setReferenceValue(query.getResourcePath().toString() + "/doc").build(); + Value documentBoundary = reference(DOCUMENT_NAME); RunQueryRequest queryRequest = query( @@ -408,10 +401,9 @@ public void withStartAt() throws Exception { streamObserverCapture.capture(), Matchers.any()); - query.orderBy("foo").orderBy(FieldPath.documentId()).startAt("bar", "foo").get().get(); + query.orderBy("foo").orderBy(FieldPath.documentId()).startAt("bar", "doc").get().get(); - Value documentBoundary = - Value.newBuilder().setReferenceValue(query.getResourcePath().toString() + "/foo").build(); + Value documentBoundary = reference(DOCUMENT_NAME); RunQueryRequest queryRequest = query( @@ -505,6 +497,43 @@ public void withEndAt() throws Exception { assertEquals(queryRequest, runQuery.getValue()); } + @Test + public void withCollectionGroup() throws Exception { + doAnswer(queryResponse()) + .when(firestoreMock) + .streamRequest( + runQuery.capture(), + streamObserverCapture.capture(), + Matchers.any()); + + Query query = firestoreMock.collectionGroup(COLLECTION_ID); + query = query.whereGreaterThan(FieldPath.documentId(), "coll/doc"); + query = query.orderBy(FieldPath.documentId()); + query = query.endAt("coll/doc"); + query.get(); + + RunQueryRequest queryRequest = + query( + /* transactionId= */ null, + /* allDescendants= */ true, + filter(Operator.GREATER_THAN, "__name__", reference(DOCUMENT_NAME)), + order("__name__", StructuredQuery.Direction.ASCENDING), + endAt(reference(DOCUMENT_NAME), false)); + + assertEquals(queryRequest, runQuery.getValue()); + } + + @Test + public void collectionGroupCannotContainSlashes() { + try { + Query query = firestoreMock.collectionGroup("foo/bar"); + fail(); + } catch (IllegalArgumentException e) { + assertEquals( + "Invalid collectionId 'foo/bar'. Collection IDs must not contain '/'.", e.getMessage()); + } + } + @Test(expected = IllegalStateException.class) public void overspecifiedCursor() throws Exception { query.orderBy("foo").startAt("foo", "bar", "bar", "foo"); diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java index 0a227af0bbbf..11cda230907d 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java +++ b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java @@ -459,7 +459,7 @@ public QuerySnapshot updateCallback(Transaction transaction) assertEquals(3, requests.size()); assertEquals(begin(), requests.get(0)); - assertEquals(query(TRANSACTION_ID), requests.get(1)); + assertEquals(query(TRANSACTION_ID, false), requests.get(1)); assertEquals(commit(TRANSACTION_ID), requests.get(2)); } diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java index c4940f6ff38b..9115f3841d25 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java +++ b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITSystemTest.java @@ -17,6 +17,7 @@ package com.google.cloud.firestore.it; import static com.google.cloud.firestore.LocalFirestoreHelper.map; +import static java.util.Arrays.asList; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; @@ -33,6 +34,7 @@ import com.google.cloud.firestore.DocumentSnapshot; import com.google.cloud.firestore.EventListener; import com.google.cloud.firestore.FieldMask; +import com.google.cloud.firestore.FieldPath; import com.google.cloud.firestore.FieldValue; import com.google.cloud.firestore.Firestore; import com.google.cloud.firestore.FirestoreException; @@ -106,6 +108,14 @@ private DocumentReference addDocument(String key, Object value, Object... fields return documentReference; } + private List querySnapshotToIds(QuerySnapshot querySnapshot) { + List documentIds = new ArrayList<>(); + for (QueryDocumentSnapshot snapshot : querySnapshot.getDocuments()) { + documentIds.add(snapshot.getId()); + } + return documentIds; + } + @Test public void getAll() throws Exception { DocumentReference ref1 = randomColl.document("doc1"); @@ -977,4 +987,126 @@ public void arrayOperators() throws ExecutionException, InterruptedException { assertTrue(containsQuery.get().get().isEmpty()); } + + @Test + public void testCollectionGroupQueries() throws ExecutionException, InterruptedException { + // Use `randomColl` to get a random collection group name to use but ensure it starts with 'b' + // for predictable ordering. + String collectionGroup = "b" + randomColl.getId(); + + String[] docPaths = + new String[] { + "abc/123/${collectionGroup}/cg-doc1", + "abc/123/${collectionGroup}/cg-doc2", + "${collectionGroup}/cg-doc3", + "${collectionGroup}/cg-doc4", + "def/456/${collectionGroup}/cg-doc5", + "${collectionGroup}/virtual-doc/nested-coll/not-cg-doc", + "x${collectionGroup}/not-cg-doc", + "${collectionGroup}x/not-cg-doc", + "abc/123/${collectionGroup}x/not-cg-doc", + "abc/123/x${collectionGroup}/not-cg-doc", + "abc/${collectionGroup}" + }; + WriteBatch batch = firestore.batch(); + for (String path : docPaths) { + batch.set( + firestore.document(path.replace("${collectionGroup}", collectionGroup)), map("x", 1)); + } + batch.commit().get(); + + QuerySnapshot querySnapshot = firestore.collectionGroup(collectionGroup).get().get(); + assertEquals( + asList("cg-doc1", "cg-doc2", "cg-doc3", "cg-doc4", "cg-doc5"), + querySnapshotToIds(querySnapshot)); + } + + @Test + public void testCollectionGroupQueriesWithStartAtEndAtWithArbitraryDocumentIds() + throws ExecutionException, InterruptedException { + // Use `randomColl` to get a random collection group name to use but ensure it starts with 'b' + // for predictable ordering. + String collectionGroup = "b" + randomColl.getId(); + + String[] docPaths = + new String[] { + "a/a/${collectionGroup}/cg-doc1", + "a/b/a/b/${collectionGroup}/cg-doc2", + "a/b/${collectionGroup}/cg-doc3", + "a/b/c/d/${collectionGroup}/cg-doc4", + "a/c/${collectionGroup}/cg-doc5", + "${collectionGroup}/cg-doc6", + "a/b/nope/nope" + }; + WriteBatch batch = firestore.batch(); + for (String path : docPaths) { + batch.set( + firestore.document(path.replace("${collectionGroup}", collectionGroup)), map("x", 1)); + } + batch.commit().get(); + + QuerySnapshot querySnapshot = + firestore + .collectionGroup(collectionGroup) + .orderBy(FieldPath.documentId()) + .startAt("a/b") + .endAt("a/b0") + .get() + .get(); + assertEquals(asList("cg-doc2", "cg-doc3", "cg-doc4"), querySnapshotToIds(querySnapshot)); + + querySnapshot = + firestore + .collectionGroup(collectionGroup) + .orderBy(FieldPath.documentId()) + .startAfter("a/b") + .endBefore("a/b/" + collectionGroup + "/cg-doc3") + .get() + .get(); + assertEquals(asList("cg-doc2"), querySnapshotToIds(querySnapshot)); + } + + @Test + public void testCollectionGroupQueriesWithWhereFiltersOnArbitraryDocumentIds() + throws ExecutionException, InterruptedException { + // Use `randomColl` to get a random collection group name to use but ensure it starts with 'b' + // for predictable ordering. + String collectionGroup = "b" + randomColl.getId(); + + String[] docPaths = + new String[] { + "a/a/${collectionGroup}/cg-doc1", + "a/b/a/b/${collectionGroup}/cg-doc2", + "a/b/${collectionGroup}/cg-doc3", + "a/b/c/d/${collectionGroup}/cg-doc4", + "a/c/${collectionGroup}/cg-doc5", + "${collectionGroup}/cg-doc6", + "a/b/nope/nope" + }; + + WriteBatch batch = firestore.batch(); + for (String path : docPaths) { + batch.set( + firestore.document(path.replace("${collectionGroup}", collectionGroup)), map("x", 1)); + } + batch.commit().get(); + + QuerySnapshot querySnapshot = + firestore + .collectionGroup(collectionGroup) + .whereGreaterThanOrEqualTo(FieldPath.documentId(), "a/b") + .whereLessThanOrEqualTo(FieldPath.documentId(), "a/b0") + .get() + .get(); + assertEquals(asList("cg-doc2", "cg-doc3", "cg-doc4"), querySnapshotToIds(querySnapshot)); + + querySnapshot = + firestore + .collectionGroup(collectionGroup) + .whereGreaterThan(FieldPath.documentId(), "a/b") + .whereLessThan(FieldPath.documentId(), "a/b/" + collectionGroup + "/cg-doc3") + .get() + .get(); + assertEquals(asList("cg-doc2"), querySnapshotToIds(querySnapshot)); + } } From 10d379c58585a97feb6f0421de4f32455b32a52c Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 6 Mar 2019 16:28:15 -0800 Subject: [PATCH 2/4] Using AutoValue --- .../cloud/firestore/CollectionReference.java | 10 +- .../com/google/cloud/firestore/Query.java | 379 +++++++++--------- .../com/google/cloud/firestore/Watch.java | 2 +- 3 files changed, 201 insertions(+), 190 deletions(-) diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java index 7cb74a9662cc..a1cf52ba10de 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/CollectionReference.java @@ -59,7 +59,7 @@ public class CollectionReference extends Query { */ @Nonnull public String getId() { - return options.collectionId; + return options.getCollectionId(); } /** @@ -69,7 +69,7 @@ public String getId() { */ @Nullable public DocumentReference getParent() { - ResourcePath parent = options.parentPath; + ResourcePath parent = options.getParentPath(); return parent.isDocument() ? new DocumentReference(firestore, parent) : null; } @@ -124,8 +124,8 @@ public DocumentReference document(@Nonnull String childPath) { @Nonnull public Iterable listDocuments() { ListDocumentsRequest.Builder request = ListDocumentsRequest.newBuilder(); - request.setParent(options.parentPath.toString()); - request.setCollectionId(options.collectionId); + request.setParent(options.getParentPath().toString()); + request.setCollectionId(options.getCollectionId()); request.setMask(DocumentMask.getDefaultInstance()); request.setShowMissing(true); @@ -209,6 +209,6 @@ public ApiFuture add(Object pojo) { /** Returns a resource path pointing to this collection. */ ResourcePath getResourcePath() { - return options.parentPath.append(options.collectionId); + return options.getParentPath().append(options.getCollectionId()); } } diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java index ee8141c047cc..c61638e955a1 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java @@ -26,8 +26,11 @@ import com.google.api.core.ApiFuture; import com.google.api.core.SettableApiFuture; import com.google.api.gax.rpc.ApiStreamObserver; +import com.google.auto.value.AutoValue; import com.google.cloud.Timestamp; +import com.google.cloud.firestore.Query.QueryOptions.Builder; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.firestore.v1.Cursor; import com.google.firestore.v1.Document; @@ -78,7 +81,7 @@ StructuredQuery.Direction getDirection() { } } - private abstract static class FieldFilter { + abstract static class FieldFilter { final FieldPath fieldPath; final Object value; @@ -160,7 +163,7 @@ Filter toProto() { } } - private static class FieldOrder { + static class FieldOrder { final FieldPath fieldPath; final Direction direction; @@ -178,113 +181,87 @@ Order toProto() { } /** Options that define a Firestore Query. */ - protected static class QueryOptions { - - ResourcePath parentPath; - String collectionId; - boolean allDescendants; - int limit; - int offset; - @Nullable Cursor startCursor; - @Nullable Cursor endCursor; - List fieldFilters; - List fieldOrders; - List fieldProjections; - - private QueryOptions(ResourcePath parentPath, String collectionId) { - this.parentPath = parentPath; - this.collectionId = collectionId; - this.allDescendants = false; - this.limit = -1; - this.offset = -1; - this.startCursor = null; - this.endCursor = null; - this.fieldFilters = new ArrayList<>(); - this.fieldOrders = new ArrayList<>(); - this.fieldProjections = new ArrayList<>(); - } + @AutoValue + protected abstract static class QueryOptions { - private QueryOptions(QueryOptions options) { - parentPath = options.parentPath; - allDescendants = options.allDescendants; - collectionId = options.collectionId; - limit = options.limit; - offset = options.offset; - startCursor = options.startCursor; - endCursor = options.endCursor; - fieldFilters = new ArrayList<>(options.fieldFilters); - fieldOrders = new ArrayList<>(options.fieldOrders); - fieldProjections = options.fieldProjections; - } + abstract ResourcePath getParentPath(); - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (o == null || getClass() != o.getClass()) { - return false; - } + abstract String getCollectionId(); - QueryOptions that = (QueryOptions) o; + abstract boolean getAllDescendants(); - if (allDescendants != that.allDescendants) { - return false; - } - if (limit != that.limit) { - return false; - } - if (offset != that.offset) { - return false; - } - if (!parentPath.equals(that.parentPath)) { - return false; - } - if (!collectionId.equals(that.collectionId)) { - return false; - } - if (startCursor != null ? !startCursor.equals(that.startCursor) : that.startCursor != null) { - return false; - } - if (endCursor != null ? !endCursor.equals(that.endCursor) : that.endCursor != null) { - return false; - } - if (!fieldFilters.equals(that.fieldFilters)) { - return false; - } - if (!fieldOrders.equals(that.fieldOrders)) { - return false; - } - return fieldProjections.equals(that.fieldProjections); + abstract @Nullable Integer getLimit(); + + abstract @Nullable Integer getOffset(); + + abstract @Nullable Cursor getStartCursor(); + + abstract @Nullable Cursor getEndCursor(); + + abstract ImmutableList getFieldFilters(); + + abstract ImmutableList getFieldOrders(); + + abstract ImmutableList getFieldProjections(); + + static Builder builder() { + return new AutoValue_Query_QueryOptions.Builder() + .setAllDescendants(false) + .setFieldOrders(ImmutableList.of()) + .setFieldFilters(ImmutableList.of()) + .setFieldProjections(ImmutableList.of()); } - @Override - public int hashCode() { - int result = parentPath.hashCode(); - result = 31 * result + collectionId.hashCode(); - result = 31 * result + (allDescendants ? 1 : 0); - result = 31 * result + limit; - result = 31 * result + offset; - result = 31 * result + (startCursor != null ? startCursor.hashCode() : 0); - result = 31 * result + (endCursor != null ? endCursor.hashCode() : 0); - result = 31 * result + fieldFilters.hashCode(); - result = 31 * result + fieldOrders.hashCode(); - result = 31 * result + fieldProjections.hashCode(); - return result; + abstract Builder toBuilder(); + + @AutoValue.Builder + abstract static class Builder { + abstract Builder setParentPath(ResourcePath value); + + abstract Builder setCollectionId(String value); + + abstract Builder setAllDescendants(boolean value); + + abstract Builder setLimit(Integer value); + + abstract Builder setOffset(Integer value); + + abstract Builder setStartCursor(@Nullable Cursor value); + + abstract Builder setEndCursor(@Nullable Cursor value); + + abstract Builder setFieldFilters(ImmutableList value); + + abstract Builder setFieldOrders(ImmutableList value); + + abstract Builder setFieldProjections(ImmutableList value); + + abstract QueryOptions build(); } } + /** Creates a query for documents in a single collection */ Query(FirestoreImpl firestore, ResourcePath path) { - this.firestore = firestore; - this.options = new QueryOptions(path.getParent(), path.getId()); + this( + firestore, + QueryOptions.builder() + .setParentPath(path.getParent()) + .setCollectionId(path.getId()) + .build()); } + /** + * Creates a Collection Group query that matches all documents directly nested under a + * specifically named collection + */ Query(FirestoreImpl firestore, String collectionId) { - QueryOptions queryOptions = new QueryOptions(firestore.getResourcePath(), collectionId); - queryOptions.allDescendants = true; - - this.firestore = firestore; - this.options = queryOptions; + this( + firestore, + QueryOptions.builder() + .setParentPath(firestore.getResourcePath()) + .setCollectionId(collectionId) + .setAllDescendants(true) + .build()); } private Query(FirestoreImpl firestore, QueryOptions queryOptions) { @@ -308,20 +285,20 @@ private static boolean isUnaryComparison(@Nullable Object value) { } /** Computes the backend ordering semantics for DocumentSnapshot cursors. */ - private List createImplicitOrderBy() { - List implicitOrders = new ArrayList<>(options.fieldOrders); + private ImmutableList createImplicitOrderBy() { + List implicitOrders = new ArrayList<>(options.getFieldOrders()); boolean hasDocumentId = false; if (implicitOrders.isEmpty()) { // If no explicit ordering is specified, use the first inequality to define an implicit order. - for (FieldFilter fieldFilter : options.fieldFilters) { + for (FieldFilter fieldFilter : options.getFieldFilters()) { if (!fieldFilter.isEqualsFilter()) { implicitOrders.add(new FieldOrder(fieldFilter.fieldPath, Direction.ASCENDING)); break; } } } else { - for (FieldOrder fieldOrder : options.fieldOrders) { + for (FieldOrder fieldOrder : options.getFieldOrders()) { if (fieldOrder.fieldPath.equals(FieldPath.DOCUMENT_ID)) { hasDocumentId = true; } @@ -337,11 +314,12 @@ private List createImplicitOrderBy() { implicitOrders.add(new FieldOrder(FieldPath.documentId(), lastDirection)); } - return implicitOrders; + + return ImmutableList.builder().addAll(implicitOrders).build(); } private Cursor createCursor( - List order, DocumentSnapshot documentSnapshot, boolean before) { + ImmutableList order, DocumentSnapshot documentSnapshot, boolean before) { List fieldValues = new ArrayList<>(); for (FieldOrder fieldOrder : order) { @@ -404,9 +382,9 @@ private Cursor createCursor(List order, Object[] fieldValues, boolea */ private Object convertReference(Object fieldValue) { ResourcePath basePath = - options.allDescendants - ? options.parentPath - : options.parentPath.append(options.collectionId); + options.getAllDescendants() + ? options.getParentPath() + : options.getParentPath().append(options.getCollectionId()); DocumentReference reference; if (fieldValue instanceof String) { @@ -426,7 +404,7 @@ private Object convertReference(Object fieldValue) { reference.getPath())); } - if (!options.allDescendants && !reference.getParent().getResourcePath().equals(basePath)) { + if (!options.getAllDescendants() && !reference.getParent().getResourcePath().equals(basePath)) { throw new IllegalArgumentException( String.format( "Only a direct child can be used as a query boundary. Found: '%s'", @@ -460,14 +438,20 @@ public Query whereEqualTo(@Nonnull String field, @Nullable Object value) { @Nonnull public Query whereEqualTo(@Nonnull FieldPath fieldPath, @Nullable Object value) { Preconditions.checkState( - options.startCursor == null && options.endCursor == null, + options.getStartCursor() == null && options.getEndCursor() == null, "Cannot call whereEqualTo() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); if (isUnaryComparison(value)) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldFilters.add(new UnaryFilter(fieldPath, value)); - return new Query(firestore, newOptions); + Builder newOptions = options.toBuilder(); + UnaryFilter newFieldFilter = new UnaryFilter(fieldPath, value); + ImmutableList newFieldFilters = + ImmutableList.builder() + .addAll(options.getFieldFilters()) + .add(newFieldFilter) + .build(); + newOptions.setFieldFilters(newFieldFilters); + return new Query(firestore, newOptions.build()); } else { return whereHelper(fieldPath, EQUAL, value); } @@ -497,7 +481,7 @@ public Query whereLessThan(@Nonnull String field, @Nonnull Object value) { @Nonnull public Query whereLessThan(@Nonnull FieldPath fieldPath, @Nonnull Object value) { Preconditions.checkState( - options.startCursor == null && options.endCursor == null, + options.getStartCursor() == null && options.getEndCursor() == null, "Cannot call whereLessThan() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); return whereHelper(fieldPath, LESS_THAN, value); @@ -527,7 +511,7 @@ public Query whereLessThanOrEqualTo(@Nonnull String field, @Nonnull Object value @Nonnull public Query whereLessThanOrEqualTo(@Nonnull FieldPath fieldPath, @Nonnull Object value) { Preconditions.checkState( - options.startCursor == null && options.endCursor == null, + options.getStartCursor() == null && options.getEndCursor() == null, "Cannot call whereLessThanOrEqualTo() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); return whereHelper(fieldPath, LESS_THAN_OR_EQUAL, value); @@ -557,7 +541,7 @@ public Query whereGreaterThan(@Nonnull String field, @Nonnull Object value) { @Nonnull public Query whereGreaterThan(@Nonnull FieldPath fieldPath, @Nonnull Object value) { Preconditions.checkState( - options.startCursor == null && options.endCursor == null, + options.getStartCursor() == null && options.getEndCursor() == null, "Cannot call whereGreaterThan() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); return whereHelper(fieldPath, GREATER_THAN, value); @@ -587,7 +571,7 @@ public Query whereGreaterThanOrEqualTo(@Nonnull String field, @Nonnull Object va @Nonnull public Query whereGreaterThanOrEqualTo(@Nonnull FieldPath fieldPath, @Nonnull Object value) { Preconditions.checkState( - options.startCursor == null && options.endCursor == null, + options.getStartCursor() == null && options.getEndCursor() == null, "Cannot call whereGreaterThanOrEqualTo() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); return whereHelper(fieldPath, GREATER_THAN_OR_EQUAL, value); @@ -623,7 +607,7 @@ public Query whereArrayContains(@Nonnull String field, @Nonnull Object value) { @Nonnull public Query whereArrayContains(@Nonnull FieldPath fieldPath, @Nonnull Object value) { Preconditions.checkState( - options.startCursor == null && options.endCursor == null, + options.getStartCursor() == null && options.getEndCursor() == null, "Cannot call whereArrayContains() after defining a boundary with startAt(), " + "startAfter(), endBefore() or endAt()."); return whereHelper(fieldPath, ARRAY_CONTAINS, value); @@ -631,14 +615,19 @@ public Query whereArrayContains(@Nonnull FieldPath fieldPath, @Nonnull Object va private Query whereHelper( FieldPath fieldPath, StructuredQuery.FieldFilter.Operator operator, Object value) { - QueryOptions newOptions = new QueryOptions(options); - if (fieldPath.equals(FieldPath.DOCUMENT_ID)) { value = this.convertReference(value); } - newOptions.fieldFilters.add(new ComparisonFilter(fieldPath, operator, value)); - return new Query(firestore, newOptions); + Builder newOptions = options.toBuilder(); + ComparisonFilter newFieldFilter = new ComparisonFilter(fieldPath, operator, value); + ImmutableList newFieldFilters = + ImmutableList.builder() + .addAll(options.getFieldFilters()) + .add(newFieldFilter) + .build(); + newOptions.setFieldFilters(newFieldFilters); + return new Query(firestore, newOptions.build()); } /** @@ -687,14 +676,20 @@ public Query orderBy(@Nonnull String field, @Nonnull Direction direction) { @Nonnull public Query orderBy(@Nonnull FieldPath fieldPath, @Nonnull Direction direction) { Preconditions.checkState( - options.startCursor == null && options.endCursor == null, + options.getStartCursor() == null && options.getEndCursor() == null, "Cannot specify an orderBy() constraint after calling startAt(), " + "startAfter(), endBefore() or endAt()."); - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldOrders.add(new FieldOrder(fieldPath, direction)); + Builder newOptions = options.toBuilder(); + FieldOrder newFieldOrder = new FieldOrder(fieldPath, direction); + ImmutableList newFieldOrders = + ImmutableList.builder() + .addAll(options.getFieldOrders()) + .add(newFieldOrder) + .build(); + newOptions.setFieldOrders(newFieldOrders); - return new Query(firestore, newOptions); + return new Query(firestore, newOptions.build()); } /** @@ -706,9 +701,7 @@ public Query orderBy(@Nonnull FieldPath fieldPath, @Nonnull Direction direction) */ @Nonnull public Query limit(int limit) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.limit = limit; - return new Query(firestore, newOptions); + return new Query(firestore, options.toBuilder().setLimit(limit).build()); } /** @@ -719,9 +712,7 @@ public Query limit(int limit) { */ @Nonnull public Query offset(int offset) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.offset = offset; - return new Query(firestore, newOptions); + return new Query(firestore, options.toBuilder().setOffset(offset).build()); } /** @@ -734,10 +725,13 @@ public Query offset(int offset) { */ @Nonnull public Query startAt(@Nonnull DocumentSnapshot snapshot) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldOrders = createImplicitOrderBy(); - newOptions.startCursor = createCursor(newOptions.fieldOrders, snapshot, true); - return new Query(firestore, newOptions); + ImmutableList fieldOrders = createImplicitOrderBy(); + Cursor cursor = createCursor(fieldOrders, snapshot, true); + + Builder newOptions = options.toBuilder(); + newOptions.setFieldOrders(fieldOrders); + newOptions.setStartCursor(cursor); + return new Query(firestore, newOptions.build()); } /** @@ -749,9 +743,11 @@ public Query startAt(@Nonnull DocumentSnapshot snapshot) { */ @Nonnull public Query startAt(Object... fieldValues) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.startCursor = createCursor(newOptions.fieldOrders, fieldValues, true); - return new Query(firestore, newOptions); + Cursor cursor = createCursor(options.getFieldOrders(), fieldValues, true); + + Builder newOptions = options.toBuilder(); + newOptions.setStartCursor(cursor); + return new Query(firestore, newOptions.build()); } /** @@ -783,8 +779,7 @@ public Query select(String... fields) { */ @Nonnull public Query select(FieldPath... fieldPaths) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldProjections = new ArrayList<>(); + ImmutableList.Builder fieldProjections = ImmutableList.builder(); if (fieldPaths.length == 0) { fieldPaths = new FieldPath[] {FieldPath.DOCUMENT_ID}; @@ -793,10 +788,11 @@ public Query select(FieldPath... fieldPaths) { for (FieldPath path : fieldPaths) { FieldReference fieldReference = FieldReference.newBuilder().setFieldPath(path.getEncodedPath()).build(); - newOptions.fieldProjections.add(fieldReference); + fieldProjections.add(fieldReference); } - return new Query(firestore, newOptions); + Builder newOptions = options.toBuilder().setFieldProjections(fieldProjections.build()); + return new Query(firestore, newOptions.build()); } /** @@ -809,10 +805,13 @@ public Query select(FieldPath... fieldPaths) { */ @Nonnull public Query startAfter(@Nonnull DocumentSnapshot snapshot) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldOrders = createImplicitOrderBy(); - newOptions.startCursor = createCursor(newOptions.fieldOrders, snapshot, false); - return new Query(firestore, newOptions); + ImmutableList fieldOrders = createImplicitOrderBy(); + Cursor cursor = createCursor(fieldOrders, snapshot, false); + + Builder newOptions = options.toBuilder(); + newOptions.setFieldOrders(fieldOrders); + newOptions.setStartCursor(cursor); + return new Query(firestore, newOptions.build()); } /** @@ -825,9 +824,11 @@ public Query startAfter(@Nonnull DocumentSnapshot snapshot) { * @return The created Query. */ public Query startAfter(Object... fieldValues) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.startCursor = createCursor(newOptions.fieldOrders, fieldValues, false); - return new Query(firestore, newOptions); + Cursor cursor = createCursor(options.getFieldOrders(), fieldValues, false); + + Builder newOptions = options.toBuilder(); + newOptions.setStartCursor(cursor); + return new Query(firestore, newOptions.build()); } /** @@ -840,10 +841,13 @@ public Query startAfter(Object... fieldValues) { */ @Nonnull public Query endBefore(@Nonnull DocumentSnapshot snapshot) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldOrders = createImplicitOrderBy(); - newOptions.endCursor = createCursor(newOptions.fieldOrders, snapshot, true); - return new Query(firestore, newOptions); + ImmutableList fieldOrders = createImplicitOrderBy(); + Cursor cursor = createCursor(fieldOrders, snapshot, true); + + Builder newOptions = options.toBuilder(); + newOptions.setFieldOrders(fieldOrders); + newOptions.setEndCursor(cursor); + return new Query(firestore, newOptions.build()); } /** @@ -856,9 +860,11 @@ public Query endBefore(@Nonnull DocumentSnapshot snapshot) { */ @Nonnull public Query endBefore(Object... fieldValues) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.endCursor = createCursor(newOptions.fieldOrders, fieldValues, true); - return new Query(firestore, newOptions); + Cursor cursor = createCursor(options.getFieldOrders(), fieldValues, true); + + Builder newOptions = options.toBuilder(); + newOptions.setEndCursor(cursor); + return new Query(firestore, newOptions.build()); } /** @@ -870,9 +876,11 @@ public Query endBefore(Object... fieldValues) { */ @Nonnull public Query endAt(Object... fieldValues) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.endCursor = createCursor(newOptions.fieldOrders, fieldValues, false); - return new Query(firestore, newOptions); + Cursor cursor = createCursor(options.getFieldOrders(), fieldValues, false); + + Builder newOptions = options.toBuilder(); + newOptions.setEndCursor(cursor); + return new Query(firestore, newOptions.build()); } /** @@ -885,22 +893,25 @@ public Query endAt(Object... fieldValues) { */ @Nonnull public Query endAt(@Nonnull DocumentSnapshot snapshot) { - QueryOptions newOptions = new QueryOptions(options); - newOptions.fieldOrders = createImplicitOrderBy(); - newOptions.endCursor = createCursor(newOptions.fieldOrders, snapshot, false); - return new Query(firestore, newOptions); + ImmutableList fieldOrders = createImplicitOrderBy(); + Cursor cursor = createCursor(fieldOrders, snapshot, false); + + Builder newOptions = options.toBuilder(); + newOptions.setFieldOrders(fieldOrders); + newOptions.setEndCursor(cursor); + return new Query(firestore, newOptions.build()); } /** Build the final Firestore query. */ StructuredQuery.Builder buildQuery() { StructuredQuery.Builder structuredQuery = StructuredQuery.newBuilder(); CollectionSelector.Builder collectionSelector = CollectionSelector.newBuilder(); - collectionSelector.setCollectionId(options.collectionId); - collectionSelector.setAllDescendants(options.allDescendants); + collectionSelector.setCollectionId(options.getCollectionId()); + collectionSelector.setAllDescendants(options.getAllDescendants()); structuredQuery.addFrom(collectionSelector); - if (options.fieldFilters.size() == 1) { - Filter filter = options.fieldFilters.get(0).toProto(); + if (options.getFieldFilters().size() == 1) { + Filter filter = options.getFieldFilters().get(0).toProto(); if (filter.hasFieldFilter()) { structuredQuery.getWhereBuilder().setFieldFilter(filter.getFieldFilter()); } else { @@ -908,42 +919,42 @@ StructuredQuery.Builder buildQuery() { filter.hasUnaryFilter(), "Expected a UnaryFilter or a FieldFilter."); structuredQuery.getWhereBuilder().setUnaryFilter(filter.getUnaryFilter()); } - } else if (options.fieldFilters.size() > 1) { + } else if (options.getFieldFilters().size() > 1) { Filter.Builder filter = Filter.newBuilder(); StructuredQuery.CompositeFilter.Builder compositeFilter = StructuredQuery.CompositeFilter.newBuilder(); compositeFilter.setOp(CompositeFilter.Operator.AND); - for (FieldFilter fieldFilter : options.fieldFilters) { + for (FieldFilter fieldFilter : options.getFieldFilters()) { compositeFilter.addFilters(fieldFilter.toProto()); } filter.setCompositeFilter(compositeFilter.build()); structuredQuery.setWhere(filter.build()); } - if (!options.fieldOrders.isEmpty()) { - for (FieldOrder order : options.fieldOrders) { + if (!options.getFieldOrders().isEmpty()) { + for (FieldOrder order : options.getFieldOrders()) { structuredQuery.addOrderBy(order.toProto()); } } - if (!options.fieldProjections.isEmpty()) { - structuredQuery.getSelectBuilder().addAllFields(options.fieldProjections); + if (!options.getFieldProjections().isEmpty()) { + structuredQuery.getSelectBuilder().addAllFields(options.getFieldProjections()); } - if (options.limit != -1) { - structuredQuery.setLimit(Int32Value.newBuilder().setValue(options.limit)); + if (options.getLimit() != null) { + structuredQuery.setLimit(Int32Value.newBuilder().setValue(options.getLimit())); } - if (options.offset != -1) { - structuredQuery.setOffset(options.offset); + if (options.getOffset() != null) { + structuredQuery.setOffset(options.getOffset()); } - if (options.startCursor != null) { - structuredQuery.setStartAt(options.startCursor); + if (options.getStartCursor() != null) { + structuredQuery.setStartAt(options.getStartCursor()); } - if (options.endCursor != null) { - structuredQuery.setEndAt(options.endCursor); + if (options.getEndCursor() != null) { + structuredQuery.setEndAt(options.getEndCursor()); } return structuredQuery; @@ -994,7 +1005,7 @@ Timestamp getReadTime() { private void stream( final QuerySnapshotObserver documentObserver, @Nullable ByteString transactionId) { RunQueryRequest.Builder request = RunQueryRequest.newBuilder(); - request.setStructuredQuery(buildQuery()).setParent(options.parentPath.toString()); + request.setStructuredQuery(buildQuery()).setParent(options.getParentPath().toString()); if (transactionId != null) { request.setTransaction(transactionId); @@ -1127,13 +1138,13 @@ Comparator comparator() { @Override public int compare(QueryDocumentSnapshot doc1, QueryDocumentSnapshot doc2) { // Add implicit sorting by name, using the last specified direction. + ImmutableList fieldOrders = options.getFieldOrders(); Direction lastDirection = - options.fieldOrders.isEmpty() + fieldOrders.isEmpty() ? Direction.ASCENDING - : options.fieldOrders.get(options.fieldOrders.size() - 1).direction; + : fieldOrders.get(fieldOrders.size() - 1).direction; - List orderBys = new ArrayList<>(); - orderBys.addAll(options.fieldOrders); + List orderBys = new ArrayList<>(fieldOrders); orderBys.add(new FieldOrder(FieldPath.DOCUMENT_ID, lastDirection)); for (FieldOrder orderBy : orderBys) { diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java index b2e1686627ad..f0783924e127 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Watch.java @@ -172,7 +172,7 @@ static Watch forQuery(Query query) { target.setQuery( QueryTarget.newBuilder() .setStructuredQuery(query.buildQuery()) - .setParent(query.options.parentPath.getName()) + .setParent(query.options.getParentPath().getName()) .build()); target.setTargetId(WATCH_TARGET_ID); From 98aabe02b68ec34e5737498b52b6cc4c75d96060 Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 6 Mar 2019 16:36:26 -0800 Subject: [PATCH 3/4] Adding comment --- .../test/java/com/google/cloud/firestore/TransactionTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java index 11cda230907d..8895bca4f4a7 100644 --- a/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java +++ b/google-cloud-clients/google-cloud-firestore/src/test/java/com/google/cloud/firestore/TransactionTest.java @@ -459,7 +459,7 @@ public QuerySnapshot updateCallback(Transaction transaction) assertEquals(3, requests.size()); assertEquals(begin(), requests.get(0)); - assertEquals(query(TRANSACTION_ID, false), requests.get(1)); + assertEquals(query(TRANSACTION_ID, /* allDescendants= */ false), requests.get(1)); assertEquals(commit(TRANSACTION_ID), requests.get(2)); } From a6d948bf3731a7e1ce1fcd3db8ab733a3c9b17de Mon Sep 17 00:00:00 2001 From: Sebastian Schmidt Date: Wed, 6 Mar 2019 16:45:01 -0800 Subject: [PATCH 4/4] Add append() helper --- .../com/google/cloud/firestore/Query.java | 36 +++++++++---------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java index c61638e955a1..20912a52d3e3 100644 --- a/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java +++ b/google-cloud-clients/google-cloud-firestore/src/main/java/com/google/cloud/firestore/Query.java @@ -163,7 +163,7 @@ Filter toProto() { } } - static class FieldOrder { + static final class FieldOrder { final FieldPath fieldPath; final Direction direction; @@ -182,7 +182,7 @@ Order toProto() { /** Options that define a Firestore Query. */ @AutoValue - protected abstract static class QueryOptions { + abstract static class QueryOptions { abstract ResourcePath getParentPath(); @@ -445,12 +445,7 @@ public Query whereEqualTo(@Nonnull FieldPath fieldPath, @Nullable Object value) if (isUnaryComparison(value)) { Builder newOptions = options.toBuilder(); UnaryFilter newFieldFilter = new UnaryFilter(fieldPath, value); - ImmutableList newFieldFilters = - ImmutableList.builder() - .addAll(options.getFieldFilters()) - .add(newFieldFilter) - .build(); - newOptions.setFieldFilters(newFieldFilters); + newOptions.setFieldFilters(append(options.getFieldFilters(), newFieldFilter)); return new Query(firestore, newOptions.build()); } else { return whereHelper(fieldPath, EQUAL, value); @@ -621,12 +616,7 @@ private Query whereHelper( Builder newOptions = options.toBuilder(); ComparisonFilter newFieldFilter = new ComparisonFilter(fieldPath, operator, value); - ImmutableList newFieldFilters = - ImmutableList.builder() - .addAll(options.getFieldFilters()) - .add(newFieldFilter) - .build(); - newOptions.setFieldFilters(newFieldFilters); + newOptions.setFieldFilters(append(options.getFieldFilters(), newFieldFilter)); return new Query(firestore, newOptions.build()); } @@ -682,12 +672,7 @@ public Query orderBy(@Nonnull FieldPath fieldPath, @Nonnull Direction direction) Builder newOptions = options.toBuilder(); FieldOrder newFieldOrder = new FieldOrder(fieldPath, direction); - ImmutableList newFieldOrders = - ImmutableList.builder() - .addAll(options.getFieldOrders()) - .add(newFieldOrder) - .build(); - newOptions.setFieldOrders(newFieldOrders); + newOptions.setFieldOrders(append(options.getFieldOrders(), newFieldOrder)); return new Query(firestore, newOptions.build()); } @@ -1177,6 +1162,17 @@ public int compare(QueryDocumentSnapshot doc1, QueryDocumentSnapshot doc2) { }; } + /** + * Helper method to append an element to an existing ImmutableList. Returns the newly created + * list. + */ + private ImmutableList append(ImmutableList existingList, T newElement) { + ImmutableList.Builder builder = ImmutableList.builder(); + builder.addAll(existingList); + builder.add(newElement); + return builder.build(); + } + /** * Returns true if this Query is equal to the provided object. *