From dc01726ec1aae6dd8a129d4eb2d87c78633240a4 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 15 Dec 2021 09:20:58 -0600 Subject: [PATCH 1/8] Add composite filters. --- .../com/google/firebase/firestore/Filter.java | 111 ++++++++---- .../com/google/firebase/firestore/Query.java | 164 +++++++++++++----- .../firestore/core/CompositeFilter.java | 127 ++++++++++++++ .../firebase/firestore/core/FieldFilter.java | 7 + .../firebase/firestore/core/Filter.java | 4 + .../google/firebase/firestore/core/Query.java | 37 ++-- .../firebase/firestore/local/QueryEngine.java | 3 +- 7 files changed, 354 insertions(+), 99 deletions(-) create mode 100644 firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java index 70e6c77c00d..8b7b512e190 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java @@ -18,38 +18,13 @@ import androidx.annotation.Nullable; import androidx.annotation.RestrictTo; import com.google.firebase.firestore.core.FieldFilter; +import com.google.firestore.v1.StructuredQuery; +import java.util.Arrays; +import java.util.List; /** @hide */ @RestrictTo(RestrictTo.Scope.LIBRARY) public class Filter { - private final FieldPath field; - private final FieldFilter.Operator operator; - private final Object value; - - private Filter(@NonNull FieldPath field, FieldFilter.Operator operator, Object value) { - this.field = field; - this.operator = operator; - this.value = value; - } - - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) - public FieldPath getField() { - return field; - } - - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) - public FieldFilter.Operator getOperator() { - return operator; - } - - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) - public Object getValue() { - return value; - } - @NonNull public static Filter equalTo(@NonNull String field, @Nullable Object value) { return equalTo(FieldPath.fromDotSeparatedPath(field), value); @@ -57,7 +32,7 @@ public static Filter equalTo(@NonNull String field, @Nullable Object value) { @NonNull public static Filter equalTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.EQUAL, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.EQUAL, value); } @NonNull @@ -67,7 +42,7 @@ public static Filter notEqualTo(@NonNull String field, @Nullable Object value) { @NonNull public static Filter notEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.NOT_EQUAL, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.NOT_EQUAL, value); } @NonNull @@ -77,7 +52,7 @@ public static Filter greaterThan(@NonNull String field, @Nullable Object value) @NonNull public static Filter greaterThan(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.GREATER_THAN, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.GREATER_THAN, value); } @NonNull @@ -87,7 +62,7 @@ public static Filter greaterThanOrEqualTo(@NonNull String field, @Nullable Objec @NonNull public static Filter greaterThanOrEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.GREATER_THAN_OR_EQUAL, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.GREATER_THAN_OR_EQUAL, value); } @NonNull @@ -97,7 +72,7 @@ public static Filter lessThan(@NonNull String field, @Nullable Object value) { @NonNull public static Filter lessThan(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.LESS_THAN, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.LESS_THAN, value); } @NonNull @@ -107,7 +82,7 @@ public static Filter lessThanOrEqualTo(@NonNull String field, @Nullable Object v @NonNull public static Filter lessThanOrEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.LESS_THAN_OR_EQUAL, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.LESS_THAN_OR_EQUAL, value); } @NonNull @@ -117,7 +92,7 @@ public static Filter arrayContains(@NonNull String field, @Nullable Object value @NonNull public static Filter arrayContains(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS, value); } @NonNull @@ -127,7 +102,7 @@ public static Filter arrayContainsAny(@NonNull String field, @Nullable Object va @NonNull public static Filter arrayContainsAny(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS_ANY, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS_ANY, value); } @NonNull @@ -137,7 +112,7 @@ public static Filter inArray(@NonNull String field, @Nullable Object value) { @NonNull public static Filter inArray(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.IN, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.IN, value); } @NonNull @@ -147,6 +122,66 @@ public static Filter notInArray(@NonNull String field, @Nullable Object value) { @NonNull public static Filter notInArray(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new Filter(fieldPath, FieldFilter.Operator.NOT_IN, value); + return new FieldFilterData(fieldPath, FieldFilter.Operator.NOT_IN, value); + } + + @NonNull + public static Filter or(Filter... filters) { + // TODO(orquery): Change this to Operator.OR once it is available. + return new CompositeFilterData( + Arrays.asList(filters), StructuredQuery.CompositeFilter.Operator.OPERATOR_UNSPECIFIED); + } + + @NonNull + public static Filter and(Filter... filters) { + return new CompositeFilterData( + Arrays.asList(filters), StructuredQuery.CompositeFilter.Operator.AND); + } +} + +/** @hide */ +@RestrictTo(RestrictTo.Scope.LIBRARY) +class FieldFilterData extends Filter { + private final FieldPath field; + private final FieldFilter.Operator operator; + private final Object value; + + public FieldFilterData(@NonNull FieldPath field, FieldFilter.Operator operator, Object value) { + this.field = field; + this.operator = operator; + this.value = value; + } + + public FieldPath getField() { + return field; + } + + public FieldFilter.Operator getOperator() { + return operator; + } + + public Object getValue() { + return value; + } +} + +/** @hide */ +@RestrictTo(RestrictTo.Scope.LIBRARY) +class CompositeFilterData extends Filter { + private final List filters; + private final StructuredQuery.CompositeFilter.Operator operator; + + public CompositeFilterData( + @NonNull List filters, StructuredQuery.CompositeFilter.Operator operator) { + this.filters = filters; + this.operator = operator; + } + + public List getFilters() { + return filters; + } + + public StructuredQuery.CompositeFilter.Operator getOperator() { + return operator; } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 306a923f9f1..b44e3b19755 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -28,6 +28,7 @@ import com.google.firebase.firestore.core.ActivityScope; import com.google.firebase.firestore.core.AsyncEventListener; import com.google.firebase.firestore.core.Bound; +import com.google.firebase.firestore.core.CompositeFilter; import com.google.firebase.firestore.core.EventManager.ListenOptions; import com.google.firebase.firestore.core.FieldFilter; import com.google.firebase.firestore.core.FieldFilter.Operator; @@ -387,15 +388,17 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List parsedFilters = new ArrayList<>(); + for (Filter filter : compositeFilterData.getFilters()) { + com.google.firebase.firestore.core.Filter parsedFilter = parseFilterData(filter); + if (parsedFilter != null) { + parsedFilters.add(parsedFilter); + } + } + // For empty composite filters, return null. + // For example: and(or(), and()). + if (parsedFilters.isEmpty()) { + return null; + } + // For composite filters containing 1 filter, return the only filter. + // For example: AND(FieldFilter1) == FieldFilter1 + if (parsedFilters.size() == 1) { + return parsedFilters.get(0); + } + return new CompositeFilter(parsedFilters, compositeFilterData.getOperator()); + } + + /** + * Takes a filter whose value has not been parsed, parses the value object and returns a + * FieldFilter or CompositeFilter with parsed values. + */ + @Nullable + private com.google.firebase.firestore.core.Filter parseFilterData(Filter filter) { + hardAssert( + filter instanceof FieldFilterData || filter instanceof CompositeFilterData, + "Parsing is only supported for FieldFilterData and CompositeFilterData."); + if (filter instanceof FieldFilterData) { + return parseFieldFilterData((FieldFilterData) filter); + } + return parseCompositeFilterData((CompositeFilterData) filter); + } + // TODO(orquery): This method will become public API. Change visibility and add documentation. private Query where(Filter filter) { - return new Query( - query.filter(parseFieldFilter(filter.getField(), filter.getOperator(), filter.getValue())), - firestore); + com.google.firebase.firestore.core.Filter parsedFilter = parseFilterData(filter); + if (parsedFilter == null) { + // Return the existing query if not adding any more filters (e.g. an empty composite filter). + return this; + } + validateAddingNewFilter(parsedFilter); + return new Query(query.filter(parsedFilter), firestore); } private void validateOrderByField(com.google.firebase.firestore.model.FieldPath field) { @@ -553,44 +604,71 @@ private List conflictingOps(Operator op) { } } - private void validateNewFilter(com.google.firebase.firestore.core.Filter filter) { - if (filter instanceof FieldFilter) { - FieldFilter fieldFilter = (FieldFilter) filter; - Operator filterOp = fieldFilter.getOperator(); - if (fieldFilter.isInequality()) { - com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField(); - com.google.firebase.firestore.model.FieldPath newInequality = fieldFilter.getField(); + /** Checks that adding the given field filter to the given query is valid */ + private void validateAddingNewFieldFilter( + com.google.firebase.firestore.core.Query query, + com.google.firebase.firestore.core.FieldFilter fieldFilter) { + Operator filterOp = fieldFilter.getOperator(); + if (fieldFilter.isInequality()) { + com.google.firebase.firestore.model.FieldPath existingInequality = query.inequalityField(); + com.google.firebase.firestore.model.FieldPath newInequality = fieldFilter.getField(); - if (existingInequality != null && !existingInequality.equals(newInequality)) { - throw new IllegalArgumentException( - String.format( - "All where filters with an inequality (notEqualTo, notIn, lessThan, " - + "lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the " - + "same field. But you have filters on '%s' and '%s'", - existingInequality.canonicalString(), newInequality.canonicalString())); - } - com.google.firebase.firestore.model.FieldPath firstOrderByField = - query.getFirstOrderByField(); - if (firstOrderByField != null) { - validateOrderByFieldMatchesInequality(firstOrderByField, newInequality); - } + if (existingInequality != null && !existingInequality.equals(newInequality)) { + throw new IllegalArgumentException( + String.format( + "All where filters with an inequality (notEqualTo, notIn, lessThan, " + + "lessThanOrEqualTo, greaterThan, or greaterThanOrEqualTo) must be on the " + + "same field. But you have filters on '%s' and '%s'", + existingInequality.canonicalString(), newInequality.canonicalString())); } - Operator conflictingOp = query.findFilterOperator(conflictingOps(filterOp)); - if (conflictingOp != null) { - // We special case when it's a duplicate op to give a slightly clearer error message. - if (conflictingOp == filterOp) { - throw new IllegalArgumentException( - "Invalid Query. You cannot use more than one '" + filterOp.toString() + "' filter."); - } else { - throw new IllegalArgumentException( - "Invalid Query. You cannot use '" - + filterOp.toString() - + "' filters with '" - + conflictingOp.toString() - + "' filters."); + com.google.firebase.firestore.model.FieldPath firstOrderByField = + query.getFirstOrderByField(); + if (firstOrderByField != null) { + validateOrderByFieldMatchesInequality(firstOrderByField, newInequality); + } + } + Operator conflictingOp = findFilterOperator(query.getFilters(), conflictingOps(filterOp)); + if (conflictingOp != null) { + // We special case when it's a duplicate op to give a slightly clearer error message. + if (conflictingOp == filterOp) { + throw new IllegalArgumentException( + "Invalid Query. You cannot use more than one '" + filterOp.toString() + "' filter."); + } else { + throw new IllegalArgumentException( + "Invalid Query. You cannot use '" + + filterOp.toString() + + "' filters with '" + + conflictingOp.toString() + + "' filters."); + } + } + } + + /** Checks that adding the given filter to the current query is valid */ + private void validateAddingNewFilter(com.google.firebase.firestore.core.Filter filter) { + com.google.firebase.firestore.core.Query testQuery = query; + for (FieldFilter subfilter : filter.getAllFieldFilters()) { + validateAddingNewFieldFilter(testQuery, subfilter); + testQuery = query.filter(subfilter); + } + } + + /** + * Checks if any of the provided filter operators are included in the given list of filters and + * returns the first one that is, or null if none are. + */ + @Nullable + public Operator findFilterOperator( + List filters, List operators) { + for (com.google.firebase.firestore.core.Filter filter : filters) { + if (filter instanceof FieldFilter) { + Operator filterOp = ((FieldFilter) filter).getOperator(); + if (operators.contains(filterOp)) { + return filterOp; } } } + return null; } /** diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java new file mode 100644 index 00000000000..e91a4afb2dc --- /dev/null +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java @@ -0,0 +1,127 @@ +// Copyright 2021 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.firebase.firestore.core; + +import com.google.firebase.firestore.model.Document; +import com.google.firebase.firestore.util.Function; +import com.google.firestore.v1.StructuredQuery.CompositeFilter.Operator; +import java.util.ArrayList; +import java.util.List; + +/** Represents a filter that is the conjunction or disjunction of other filters. */ +public class CompositeFilter extends Filter { + private final List filters; + private final Operator operator; + + public CompositeFilter(List filters, Operator operator) { + this.filters = filters; + this.operator = operator; + } + + public List getFilters() { + return filters; + } + + public Operator getOperator() { + return operator; + } + + @Override + public List getAllFieldFilters() { + List result = new ArrayList<>(); + for (Filter subfilter : filters) { + result.addAll(subfilter.getAllFieldFilters()); + } + return result; + } + + public boolean isConjunction() { + return operator == Operator.AND; + } + + public boolean isDisjunction() { + // TODO(orquery): Replace with Operator.OR. + return operator == Operator.OPERATOR_UNSPECIFIED; + } + + /** + * Performs a depth-first search to find and return the first FieldFilter in the composite filter + * that satisfies the condition. Returns null if none of the FieldFilters satisfy the condition. + */ + public FieldFilter firstFieldFilterWhere(Function condition) { + for (Filter filter : filters) { + if (filter instanceof FieldFilter && condition.apply(((FieldFilter) filter))) { + return (FieldFilter) filter; + } else if (filter instanceof CompositeFilter) { + FieldFilter found = ((CompositeFilter) filter).firstFieldFilterWhere(condition); + if (found != null) { + return found; + } + } + } + return null; + } + + /** + * Returns the first inequality filter contained within this composite filter. Returns null if it + * does not contain any inequalities. + */ + public FieldFilter getInequalityFilter() { + return firstFieldFilterWhere(f -> f.isInequality()); + } + + @Override + public boolean matches(Document doc) { + if (isConjunction()) { + // For conjunctions, all filters must match, so return false if any filter doesn't match. + for (Filter filter : filters) { + if (!filter.matches(doc)) { + return false; + } + } + return true; + } else { + // For disjunctions, at least one filter should match. + for (Filter filter : filters) { + if (filter.matches(doc)) { + return true; + } + } + return false; + } + } + + @Override + public String getCanonicalId() { + // TODO(orquery): Add special case for flat AND filters. + + StringBuilder builder = new StringBuilder(); + for (Filter filter : filters) { + if (builder.length() == 0) { + builder.append(isConjunction() ? "and(" : "or("); + } else { + builder.append(","); + } + builder.append(filter.getCanonicalId()); + } + builder.append(")"); + return builder.toString(); + } + + @Override + public String toString() { + return getCanonicalId(); + } +} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java index ce528bb9788..484434862ee 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java @@ -22,6 +22,7 @@ import com.google.firebase.firestore.util.Assert; import com.google.firestore.v1.Value; import java.util.Arrays; +import java.util.List; /** Represents a filter to be applied to query. */ public class FieldFilter extends Filter { @@ -158,6 +159,12 @@ public String getCanonicalId() { return getField().canonicalString() + getOperator().toString() + Values.canonicalId(getValue()); } + @Override + public List getAllFieldFilters() { + // This is already a field filter, so we return a list of size one. + return Arrays.asList(this); + } + @Override public String toString() { return field.canonicalString() + " " + operator + " " + Values.canonicalId(value); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java index 6f3ab2a7700..3f3c181719e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore.core; import com.google.firebase.firestore.model.Document; +import java.util.List; public abstract class Filter { /** Returns true if a document matches the filter. */ @@ -22,4 +23,7 @@ public abstract class Filter { /** A unique ID identifying the filter; used when serializing queries. */ public abstract String getCanonicalId(); + + /** Returns a list of all field filters that are contained within this filter */ + public abstract List getAllFieldFilters(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index b508b68b23f..fd1c7391f9a 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -17,7 +17,6 @@ import static com.google.firebase.firestore.util.Assert.hardAssert; import androidx.annotation.Nullable; -import com.google.firebase.firestore.core.FieldFilter.Operator; import com.google.firebase.firestore.core.OrderBy.Direction; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.DocumentKey; @@ -206,22 +205,11 @@ public FieldPath inequalityField() { if (fieldfilter.isInequality()) { return fieldfilter.getField(); } - } - } - return null; - } - - /** - * Checks if any of the provided filter operators are included in the query and returns the first - * one that is, or null if none are. - */ - @Nullable - public Operator findFilterOperator(List operators) { - for (Filter filter : filters) { - if (filter instanceof FieldFilter) { - Operator filterOp = ((FieldFilter) filter).getOperator(); - if (operators.contains(filterOp)) { - return filterOp; + } else if (filter instanceof CompositeFilter) { + CompositeFilter compositeFilter = (CompositeFilter) filter; + FieldFilter found = compositeFilter.getInequalityFilter(); + if (found != null) { + return found.getField(); } } } @@ -239,6 +227,11 @@ public Query filter(Filter filter) { FieldPath newInequalityField = null; if (filter instanceof FieldFilter && ((FieldFilter) filter).isInequality()) { newInequalityField = ((FieldFilter) filter).getField(); + } else if (filter instanceof CompositeFilter) { + FieldFilter inequalityFilter = ((CompositeFilter) filter).getInequalityFilter(); + if (inequalityFilter != null) { + newInequalityField = inequalityFilter.getField(); + } } FieldPath queryInequalityField = inequalityField(); @@ -543,6 +536,16 @@ public Target toTarget() { return this.memoizedTarget; } + /** Returns true if the query contains any composite filters (AND/OR). Returns false otherwise. */ + public boolean containsCompositeFilters() { + for (Filter filter : filters) { + if (filter instanceof CompositeFilter) { + return true; + } + } + return false; + } + /** * Returns a canonical string representing this query. This should match the iOS and Android * canonical ids for a query exactly. diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java index e078d706491..467cbda8683 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java @@ -105,7 +105,8 @@ public ImmutableSortedMap getDocumentsMatchingQuery( */ private @Nullable ImmutableSortedMap performQueryUsingIndex( Query query, Target target) { - if (query.matchesAllDocuments()) { + // TODO(orquery): Update this condition when we are able to serve or queries from the index. + if (query.matchesAllDocuments() || query.containsCompositeFilters()) { // Don't use index queries that can be executed by scanning the collection. return null; } From 9238952cfe3467fae3e4fb063e47485e0f00571a Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 5 Jan 2022 11:16:50 -0600 Subject: [PATCH 2/8] add inequalityField to Filter class. --- .../com/google/firebase/firestore/Query.java | 11 ++++---- .../firestore/core/CompositeFilter.java | 24 +++++++++++------- .../firebase/firestore/core/FieldFilter.java | 8 ++++++ .../firebase/firestore/core/Filter.java | 4 +++ .../google/firebase/firestore/core/Query.java | 25 +++---------------- 5 files changed, 36 insertions(+), 36 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index b44e3b19755..62c366f312e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -433,11 +433,10 @@ private FieldFilter parseFieldFilterData(@NonNull FieldFilterData fieldFilterDat } /** - * Takes a {@code CompositeFilterData} object, parses the values in each of its subfilters, and - * returns a new {@code CompositeFilter} that is constructed using the parsed field filters. - * - * @param compositeFilterData The CompositeFilterData object to parse. - * @return The created {@code CompositeFilter}. + * Takes a {@code CompositeFilterData} object, parses each of its subfilters, and a new {@code + * CompositeFilter} that is constructed using the parsed values. Returns null if the given + * CompositeFilterData does not contain any subfilters. Returns a {@code FieldFilter} if the given + * CompositeFilterData contains only one field filter. */ @Nullable private com.google.firebase.firestore.core.Filter parseCompositeFilterData( @@ -658,7 +657,7 @@ private void validateAddingNewFilter(com.google.firebase.firestore.core.Filter f * returns the first one that is, or null if none are. */ @Nullable - public Operator findFilterOperator( + private Operator findFilterOperator( List filters, List operators) { for (com.google.firebase.firestore.core.Filter filter : filters) { if (filter instanceof FieldFilter) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java index e91a4afb2dc..5fd685e9756 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore.core; import com.google.firebase.firestore.model.Document; +import com.google.firebase.firestore.model.FieldPath; import com.google.firebase.firestore.util.Function; import com.google.firestore.v1.StructuredQuery.CompositeFilter.Operator; import java.util.ArrayList; @@ -47,6 +48,19 @@ public List getAllFieldFilters() { return result; } + /** + * Returns the first inequality filter contained within this composite filter. Returns null if it + * does not contain any inequalities. + */ + @Override + public FieldPath inequalityField() { + FieldFilter found = firstFieldFilterWhere(f -> f.isInequality()); + if (found != null) { + return found.getField(); + } + return null; + } + public boolean isConjunction() { return operator == Operator.AND; } @@ -60,7 +74,7 @@ public boolean isDisjunction() { * Performs a depth-first search to find and return the first FieldFilter in the composite filter * that satisfies the condition. Returns null if none of the FieldFilters satisfy the condition. */ - public FieldFilter firstFieldFilterWhere(Function condition) { + private FieldFilter firstFieldFilterWhere(Function condition) { for (Filter filter : filters) { if (filter instanceof FieldFilter && condition.apply(((FieldFilter) filter))) { return (FieldFilter) filter; @@ -74,14 +88,6 @@ public FieldFilter firstFieldFilterWhere(Function conditio return null; } - /** - * Returns the first inequality filter contained within this composite filter. Returns null if it - * does not contain any inequalities. - */ - public FieldFilter getInequalityFilter() { - return firstFieldFilterWhere(f -> f.isInequality()); - } - @Override public boolean matches(Document doc) { if (isConjunction()) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java index 484434862ee..289a19ab7a1 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java @@ -165,6 +165,14 @@ public List getAllFieldFilters() { return Arrays.asList(this); } + @Override + public FieldPath inequalityField() { + if (isInequality()) { + return getField(); + } + return null; + } + @Override public String toString() { return field.canonicalString() + " " + operator + " " + Values.canonicalId(value); diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java index 3f3c181719e..af8f9fa48f0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java @@ -15,6 +15,7 @@ package com.google.firebase.firestore.core; import com.google.firebase.firestore.model.Document; +import com.google.firebase.firestore.model.FieldPath; import java.util.List; public abstract class Filter { @@ -26,4 +27,7 @@ public abstract class Filter { /** Returns a list of all field filters that are contained within this filter */ public abstract List getAllFieldFilters(); + + /** Returns the field of the first filter that's an inequality, or null if none. */ + public abstract FieldPath inequalityField(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index fd1c7391f9a..be9704078ad 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -200,17 +200,9 @@ public FieldPath getFirstOrderByField() { @Nullable public FieldPath inequalityField() { for (Filter filter : filters) { - if (filter instanceof FieldFilter) { - FieldFilter fieldfilter = (FieldFilter) filter; - if (fieldfilter.isInequality()) { - return fieldfilter.getField(); - } - } else if (filter instanceof CompositeFilter) { - CompositeFilter compositeFilter = (CompositeFilter) filter; - FieldFilter found = compositeFilter.getInequalityFilter(); - if (found != null) { - return found.getField(); - } + FieldPath result = filter.inequalityField(); + if (result != null) { + return result; } } return null; @@ -224,16 +216,7 @@ public FieldPath inequalityField() { */ public Query filter(Filter filter) { hardAssert(!isDocumentQuery(), "No filter is allowed for document query"); - FieldPath newInequalityField = null; - if (filter instanceof FieldFilter && ((FieldFilter) filter).isInequality()) { - newInequalityField = ((FieldFilter) filter).getField(); - } else if (filter instanceof CompositeFilter) { - FieldFilter inequalityFilter = ((CompositeFilter) filter).getInequalityFilter(); - if (inequalityFilter != null) { - newInequalityField = inequalityFilter.getField(); - } - } - + FieldPath newInequalityField = filter.inequalityField(); FieldPath queryInequalityField = inequalityField(); Assert.hardAssert( queryInequalityField == null From d206710806b8386655f2af25ce0984a43dc2a1c4 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 12 Jan 2022 14:27:02 -0600 Subject: [PATCH 3/8] Address comments. --- .../com/google/firebase/firestore/Filter.java | 121 +++++++++--------- .../com/google/firebase/firestore/Query.java | 46 +++---- .../firestore/core/CompositeFilter.java | 30 ++--- .../firebase/firestore/core/FieldFilter.java | 7 +- .../firebase/firestore/core/Filter.java | 4 +- .../google/firebase/firestore/core/Query.java | 4 +- 6 files changed, 107 insertions(+), 105 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java index 8b7b512e190..51939f247cc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java @@ -17,7 +17,7 @@ import androidx.annotation.NonNull; import androidx.annotation.Nullable; import androidx.annotation.RestrictTo; -import com.google.firebase.firestore.core.FieldFilter; +import com.google.firebase.firestore.core.FieldFilter.Operator; import com.google.firestore.v1.StructuredQuery; import java.util.Arrays; import java.util.List; @@ -25,6 +25,54 @@ /** @hide */ @RestrictTo(RestrictTo.Scope.LIBRARY) public class Filter { + /** @hide */ + @RestrictTo(RestrictTo.Scope.LIBRARY) + static class FieldFilter extends Filter { + private final FieldPath field; + private final Operator operator; + private final Object value; + + public FieldFilter(FieldPath field, Operator operator, @Nullable Object value) { + this.field = field; + this.operator = operator; + this.value = value; + } + + public FieldPath getField() { + return field; + } + + public Operator getOperator() { + return operator; + } + + @Nullable + public Object getValue() { + return value; + } + } + + /** @hide */ + @RestrictTo(RestrictTo.Scope.LIBRARY) + static class CompositeFilter extends Filter { + private final List filters; + private final StructuredQuery.CompositeFilter.Operator operator; + + public CompositeFilter( + @NonNull List filters, StructuredQuery.CompositeFilter.Operator operator) { + this.filters = filters; + this.operator = operator; + } + + public List getFilters() { + return filters; + } + + public StructuredQuery.CompositeFilter.Operator getOperator() { + return operator; + } + } + @NonNull public static Filter equalTo(@NonNull String field, @Nullable Object value) { return equalTo(FieldPath.fromDotSeparatedPath(field), value); @@ -32,7 +80,7 @@ public static Filter equalTo(@NonNull String field, @Nullable Object value) { @NonNull public static Filter equalTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.EQUAL, value); + return new FieldFilter(fieldPath, Operator.EQUAL, value); } @NonNull @@ -42,7 +90,7 @@ public static Filter notEqualTo(@NonNull String field, @Nullable Object value) { @NonNull public static Filter notEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.NOT_EQUAL, value); + return new FieldFilter(fieldPath, Operator.NOT_EQUAL, value); } @NonNull @@ -52,7 +100,7 @@ public static Filter greaterThan(@NonNull String field, @Nullable Object value) @NonNull public static Filter greaterThan(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.GREATER_THAN, value); + return new FieldFilter(fieldPath, Operator.GREATER_THAN, value); } @NonNull @@ -62,7 +110,7 @@ public static Filter greaterThanOrEqualTo(@NonNull String field, @Nullable Objec @NonNull public static Filter greaterThanOrEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.GREATER_THAN_OR_EQUAL, value); + return new FieldFilter(fieldPath, Operator.GREATER_THAN_OR_EQUAL, value); } @NonNull @@ -72,7 +120,7 @@ public static Filter lessThan(@NonNull String field, @Nullable Object value) { @NonNull public static Filter lessThan(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.LESS_THAN, value); + return new FieldFilter(fieldPath, Operator.LESS_THAN, value); } @NonNull @@ -82,7 +130,7 @@ public static Filter lessThanOrEqualTo(@NonNull String field, @Nullable Object v @NonNull public static Filter lessThanOrEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.LESS_THAN_OR_EQUAL, value); + return new FieldFilter(fieldPath, Operator.LESS_THAN_OR_EQUAL, value); } @NonNull @@ -92,7 +140,7 @@ public static Filter arrayContains(@NonNull String field, @Nullable Object value @NonNull public static Filter arrayContains(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS, value); + return new FieldFilter(fieldPath, Operator.ARRAY_CONTAINS, value); } @NonNull @@ -102,7 +150,7 @@ public static Filter arrayContainsAny(@NonNull String field, @Nullable Object va @NonNull public static Filter arrayContainsAny(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS_ANY, value); + return new FieldFilter(fieldPath, Operator.ARRAY_CONTAINS_ANY, value); } @NonNull @@ -112,7 +160,7 @@ public static Filter inArray(@NonNull String field, @Nullable Object value) { @NonNull public static Filter inArray(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.IN, value); + return new FieldFilter(fieldPath, Operator.IN, value); } @NonNull @@ -122,66 +170,19 @@ public static Filter notInArray(@NonNull String field, @Nullable Object value) { @NonNull public static Filter notInArray(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilterData(fieldPath, FieldFilter.Operator.NOT_IN, value); + return new FieldFilter(fieldPath, Operator.NOT_IN, value); } @NonNull public static Filter or(Filter... filters) { // TODO(orquery): Change this to Operator.OR once it is available. - return new CompositeFilterData( + return new CompositeFilter( Arrays.asList(filters), StructuredQuery.CompositeFilter.Operator.OPERATOR_UNSPECIFIED); } @NonNull public static Filter and(Filter... filters) { - return new CompositeFilterData( + return new CompositeFilter( Arrays.asList(filters), StructuredQuery.CompositeFilter.Operator.AND); } } - -/** @hide */ -@RestrictTo(RestrictTo.Scope.LIBRARY) -class FieldFilterData extends Filter { - private final FieldPath field; - private final FieldFilter.Operator operator; - private final Object value; - - public FieldFilterData(@NonNull FieldPath field, FieldFilter.Operator operator, Object value) { - this.field = field; - this.operator = operator; - this.value = value; - } - - public FieldPath getField() { - return field; - } - - public FieldFilter.Operator getOperator() { - return operator; - } - - public Object getValue() { - return value; - } -} - -/** @hide */ -@RestrictTo(RestrictTo.Scope.LIBRARY) -class CompositeFilterData extends Filter { - private final List filters; - private final StructuredQuery.CompositeFilter.Operator operator; - - public CompositeFilterData( - @NonNull List filters, StructuredQuery.CompositeFilter.Operator operator) { - this.filters = filters; - this.operator = operator; - } - - public List getFilters() { - return filters; - } - - public StructuredQuery.CompositeFilter.Operator getOperator() { - return operator; - } -} diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 62c366f312e..7acaebd3c85 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -388,14 +388,14 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List parsedFilters = new ArrayList<>(); for (Filter filter : compositeFilterData.getFilters()) { - com.google.firebase.firestore.core.Filter parsedFilter = parseFilterData(filter); + com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter); if (parsedFilter != null) { parsedFilters.add(parsedFilter); } @@ -466,24 +466,24 @@ private com.google.firebase.firestore.core.Filter parseCompositeFilterData( * FieldFilter or CompositeFilter with parsed values. */ @Nullable - private com.google.firebase.firestore.core.Filter parseFilterData(Filter filter) { + private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) { hardAssert( - filter instanceof FieldFilterData || filter instanceof CompositeFilterData, - "Parsing is only supported for FieldFilterData and CompositeFilterData."); - if (filter instanceof FieldFilterData) { - return parseFieldFilterData((FieldFilterData) filter); + filter instanceof Filter.FieldFilter || filter instanceof Filter.CompositeFilter, + "Parsing is only supported for Filter.FieldFilter and Filter.CompositeFilter."); + if (filter instanceof Filter.FieldFilter) { + return parseFieldFilter((Filter.FieldFilter) filter); } - return parseCompositeFilterData((CompositeFilterData) filter); + return parseCompositeFilter((Filter.CompositeFilter) filter); } // TODO(orquery): This method will become public API. Change visibility and add documentation. private Query where(Filter filter) { - com.google.firebase.firestore.core.Filter parsedFilter = parseFilterData(filter); + com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter); if (parsedFilter == null) { // Return the existing query if not adding any more filters (e.g. an empty composite filter). return this; } - validateAddingNewFilter(parsedFilter); + validateNewFilter(parsedFilter); return new Query(query.filter(parsedFilter), firestore); } @@ -603,8 +603,8 @@ private List conflictingOps(Operator op) { } } - /** Checks that adding the given field filter to the given query is valid */ - private void validateAddingNewFieldFilter( + /** Checks that adding the given field filter to the given query yields a valid query */ + private void validateNewFieldFilter( com.google.firebase.firestore.core.Query query, com.google.firebase.firestore.core.FieldFilter fieldFilter) { Operator filterOp = fieldFilter.getOperator(); @@ -644,10 +644,10 @@ private void validateAddingNewFieldFilter( } /** Checks that adding the given filter to the current query is valid */ - private void validateAddingNewFilter(com.google.firebase.firestore.core.Filter filter) { + private void validateNewFilter(com.google.firebase.firestore.core.Filter filter) { com.google.firebase.firestore.core.Query testQuery = query; - for (FieldFilter subfilter : filter.getAllFieldFilters()) { - validateAddingNewFieldFilter(testQuery, subfilter); + for (FieldFilter subfilter : filter.getFlattenedFilters()) { + validateNewFieldFilter(testQuery, subfilter); testQuery = query.filter(subfilter); } } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java index 5fd685e9756..23a4ba44e8c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java @@ -14,6 +14,8 @@ package com.google.firebase.firestore.core; +import android.text.TextUtils; +import androidx.annotation.Nullable; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.FieldPath; import com.google.firebase.firestore.util.Function; @@ -40,10 +42,10 @@ public Operator getOperator() { } @Override - public List getAllFieldFilters() { + public List getFlattenedFilters() { List result = new ArrayList<>(); for (Filter subfilter : filters) { - result.addAll(subfilter.getAllFieldFilters()); + result.addAll(subfilter.getFlattenedFilters()); } return result; } @@ -53,8 +55,8 @@ public List getAllFieldFilters() { * does not contain any inequalities. */ @Override - public FieldPath inequalityField() { - FieldFilter found = firstFieldFilterWhere(f -> f.isInequality()); + public FieldPath getFirstInequalityField() { + FieldFilter found = findFirstMatchingFilter(f -> f.isInequality()); if (found != null) { return found.getField(); } @@ -72,14 +74,16 @@ public boolean isDisjunction() { /** * Performs a depth-first search to find and return the first FieldFilter in the composite filter - * that satisfies the condition. Returns null if none of the FieldFilters satisfy the condition. + * that satisfies the condition. Returns {@code null} if none of the FieldFilters satisfy the + * condition. */ - private FieldFilter firstFieldFilterWhere(Function condition) { + @Nullable + private FieldFilter findFirstMatchingFilter(Function condition) { for (Filter filter : filters) { if (filter instanceof FieldFilter && condition.apply(((FieldFilter) filter))) { return (FieldFilter) filter; } else if (filter instanceof CompositeFilter) { - FieldFilter found = ((CompositeFilter) filter).firstFieldFilterWhere(condition); + FieldFilter found = ((CompositeFilter) filter).findFirstMatchingFilter(condition); if (found != null) { return found; } @@ -113,15 +117,11 @@ public boolean matches(Document doc) { public String getCanonicalId() { // TODO(orquery): Add special case for flat AND filters. + List canonicalIds = new ArrayList<>(); + for (Filter filter : filters) canonicalIds.add(filter.getCanonicalId()); StringBuilder builder = new StringBuilder(); - for (Filter filter : filters) { - if (builder.length() == 0) { - builder.append(isConjunction() ? "and(" : "or("); - } else { - builder.append(","); - } - builder.append(filter.getCanonicalId()); - } + builder.append(isConjunction() ? "and(" : "or("); + TextUtils.join(",", canonicalIds); builder.append(")"); return builder.toString(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java index 289a19ab7a1..a18f354a1b2 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java @@ -22,6 +22,7 @@ import com.google.firebase.firestore.util.Assert; import com.google.firestore.v1.Value; import java.util.Arrays; +import java.util.Collections; import java.util.List; /** Represents a filter to be applied to query. */ @@ -160,13 +161,13 @@ public String getCanonicalId() { } @Override - public List getAllFieldFilters() { + public List getFlattenedFilters() { // This is already a field filter, so we return a list of size one. - return Arrays.asList(this); + return Collections.singletonList(this); } @Override - public FieldPath inequalityField() { + public FieldPath getFirstInequalityField() { if (isInequality()) { return getField(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java index af8f9fa48f0..bc43b91dcdc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java @@ -26,8 +26,8 @@ public abstract class Filter { public abstract String getCanonicalId(); /** Returns a list of all field filters that are contained within this filter */ - public abstract List getAllFieldFilters(); + public abstract List getFlattenedFilters(); /** Returns the field of the first filter that's an inequality, or null if none. */ - public abstract FieldPath inequalityField(); + public abstract FieldPath getFirstInequalityField(); } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java index be9704078ad..896e8292cdb 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Query.java @@ -200,7 +200,7 @@ public FieldPath getFirstOrderByField() { @Nullable public FieldPath inequalityField() { for (Filter filter : filters) { - FieldPath result = filter.inequalityField(); + FieldPath result = filter.getFirstInequalityField(); if (result != null) { return result; } @@ -216,7 +216,7 @@ public FieldPath inequalityField() { */ public Query filter(Filter filter) { hardAssert(!isDocumentQuery(), "No filter is allowed for document query"); - FieldPath newInequalityField = filter.inequalityField(); + FieldPath newInequalityField = filter.getFirstInequalityField(); FieldPath queryInequalityField = inequalityField(); Assert.hardAssert( queryInequalityField == null From b146b811aed70167da6fb0130b74a93f3c0c4601 Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Wed, 12 Jan 2022 14:54:53 -0600 Subject: [PATCH 4/8] Return empty filter rather than null from parsing empty composite filters. --- .../java/com/google/firebase/firestore/Query.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 7acaebd3c85..df3aa4cb4cf 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -394,7 +394,6 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List parsedFilters = new ArrayList<>(); for (Filter filter : compositeFilterData.getFilters()) { com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter); - if (parsedFilter != null) { + if (!parsedFilter.getFlattenedFilters().isEmpty()) { parsedFilters.add(parsedFilter); } } - // For empty composite filters, return null. - // For example: and(or(), and()). - if (parsedFilters.isEmpty()) { - return null; - } + // For composite filters containing 1 filter, return the only filter. // For example: AND(FieldFilter1) == FieldFilter1 if (parsedFilters.size() == 1) { @@ -465,7 +459,6 @@ private com.google.firebase.firestore.core.Filter parseCompositeFilter( * Takes a filter whose value has not been parsed, parses the value object and returns a * FieldFilter or CompositeFilter with parsed values. */ - @Nullable private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) { hardAssert( filter instanceof Filter.FieldFilter || filter instanceof Filter.CompositeFilter, @@ -479,7 +472,7 @@ private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) { // TODO(orquery): This method will become public API. Change visibility and add documentation. private Query where(Filter filter) { com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter); - if (parsedFilter == null) { + if (parsedFilter.getFlattenedFilters().isEmpty()) { // Return the existing query if not adding any more filters (e.g. an empty composite filter). return this; } From 651e95329bca5c2f2b5c54ec5a6b332669cac2ca Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Fri, 14 Jan 2022 13:33:36 -0600 Subject: [PATCH 5/8] Address comments. --- .../java/com/google/firebase/firestore/Filter.java | 4 ---- .../java/com/google/firebase/firestore/Query.java | 14 ++++++-------- .../firebase/firestore/core/CompositeFilter.java | 1 + .../firebase/firestore/core/FieldFilter.java | 6 ++++++ .../com/google/firebase/firestore/core/Filter.java | 5 +++++ 5 files changed, 18 insertions(+), 12 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java index 51939f247cc..de5d9ce4244 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java @@ -25,8 +25,6 @@ /** @hide */ @RestrictTo(RestrictTo.Scope.LIBRARY) public class Filter { - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) static class FieldFilter extends Filter { private final FieldPath field; private final Operator operator; @@ -52,8 +50,6 @@ public Object getValue() { } } - /** @hide */ - @RestrictTo(RestrictTo.Scope.LIBRARY) static class CompositeFilter extends Filter { private final List filters; private final StructuredQuery.CompositeFilter.Operator operator; diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index df3aa4cb4cf..32f72454cfc 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -432,17 +432,15 @@ private FieldFilter parseFieldFilter(Filter.FieldFilter fieldFilterData) { } /** - * Takes a {@link Filter.CompositeFilter} object, parses each of its subfilters, and a new {@link - * CompositeFilter} that is constructed using the parsed values. Returns null if the given - * Filter.CompositeFilter does not contain any subfilters. Returns a {@link FieldFilter} if the - * given Filter.CompositeFilter contains only one field filter. + * Takes a {@link Filter.CompositeFilter} object, parses each of its subfilters, and returns a new + * {@link Filter} that is constructed using the parsed values. */ private com.google.firebase.firestore.core.Filter parseCompositeFilter( Filter.CompositeFilter compositeFilterData) { List parsedFilters = new ArrayList<>(); for (Filter filter : compositeFilterData.getFilters()) { com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter); - if (!parsedFilter.getFlattenedFilters().isEmpty()) { + if (!parsedFilter.getFilters().isEmpty()) { parsedFilters.add(parsedFilter); } } @@ -472,7 +470,7 @@ private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) { // TODO(orquery): This method will become public API. Change visibility and add documentation. private Query where(Filter filter) { com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter); - if (parsedFilter.getFlattenedFilters().isEmpty()) { + if (parsedFilter.getFilters().isEmpty()) { // Return the existing query if not adding any more filters (e.g. an empty composite filter). return this; } @@ -619,7 +617,7 @@ private void validateNewFieldFilter( validateOrderByFieldMatchesInequality(firstOrderByField, newInequality); } } - Operator conflictingOp = findFilterOperator(query.getFilters(), conflictingOps(filterOp)); + Operator conflictingOp = findFilterWithOperator(query.getFilters(), conflictingOps(filterOp)); if (conflictingOp != null) { // We special case when it's a duplicate op to give a slightly clearer error message. if (conflictingOp == filterOp) { @@ -650,7 +648,7 @@ private void validateNewFilter(com.google.firebase.firestore.core.Filter filter) * returns the first one that is, or null if none are. */ @Nullable - private Operator findFilterOperator( + private Operator findFilterWithOperator( List filters, List operators) { for (com.google.firebase.firestore.core.Filter filter : filters) { if (filter instanceof FieldFilter) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java index 23a4ba44e8c..c6f7c40a40e 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java @@ -33,6 +33,7 @@ public CompositeFilter(List filters, Operator operator) { this.operator = operator; } + @Override public List getFilters() { return filters; } diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java index a18f354a1b2..e57a5369dd7 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/FieldFilter.java @@ -166,6 +166,12 @@ public List getFlattenedFilters() { return Collections.singletonList(this); } + @Override + public List getFilters() { + // This is the only filter within this object, so we return a list of size one. + return Collections.singletonList(this); + } + @Override public FieldPath getFirstInequalityField() { if (isInequality()) { diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java index bc43b91dcdc..b69708f5245 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/Filter.java @@ -14,6 +14,7 @@ package com.google.firebase.firestore.core; +import androidx.annotation.Nullable; import com.google.firebase.firestore.model.Document; import com.google.firebase.firestore.model.FieldPath; import java.util.List; @@ -28,6 +29,10 @@ public abstract class Filter { /** Returns a list of all field filters that are contained within this filter */ public abstract List getFlattenedFilters(); + /** Returns a list of all filters that are contained within this filter */ + public abstract List getFilters(); + /** Returns the field of the first filter that's an inequality, or null if none. */ + @Nullable public abstract FieldPath getFirstInequalityField(); } From 867f3b77c63b2e357cc48d879e3cc22ea6dfaf9a Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Mon, 17 Jan 2022 10:58:55 -0600 Subject: [PATCH 6/8] Use UnaryFilter. --- .../com/google/firebase/firestore/Filter.java | 24 +++++++++---------- .../com/google/firebase/firestore/Query.java | 14 +++++------ 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java index de5d9ce4244..4843259e4a0 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java @@ -25,12 +25,12 @@ /** @hide */ @RestrictTo(RestrictTo.Scope.LIBRARY) public class Filter { - static class FieldFilter extends Filter { + static class UnaryFilter extends Filter { private final FieldPath field; private final Operator operator; private final Object value; - public FieldFilter(FieldPath field, Operator operator, @Nullable Object value) { + public UnaryFilter(FieldPath field, Operator operator, @Nullable Object value) { this.field = field; this.operator = operator; this.value = value; @@ -76,7 +76,7 @@ public static Filter equalTo(@NonNull String field, @Nullable Object value) { @NonNull public static Filter equalTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.EQUAL, value); + return new UnaryFilter(fieldPath, Operator.EQUAL, value); } @NonNull @@ -86,7 +86,7 @@ public static Filter notEqualTo(@NonNull String field, @Nullable Object value) { @NonNull public static Filter notEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.NOT_EQUAL, value); + return new UnaryFilter(fieldPath, Operator.NOT_EQUAL, value); } @NonNull @@ -96,7 +96,7 @@ public static Filter greaterThan(@NonNull String field, @Nullable Object value) @NonNull public static Filter greaterThan(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.GREATER_THAN, value); + return new UnaryFilter(fieldPath, Operator.GREATER_THAN, value); } @NonNull @@ -106,7 +106,7 @@ public static Filter greaterThanOrEqualTo(@NonNull String field, @Nullable Objec @NonNull public static Filter greaterThanOrEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.GREATER_THAN_OR_EQUAL, value); + return new UnaryFilter(fieldPath, Operator.GREATER_THAN_OR_EQUAL, value); } @NonNull @@ -116,7 +116,7 @@ public static Filter lessThan(@NonNull String field, @Nullable Object value) { @NonNull public static Filter lessThan(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.LESS_THAN, value); + return new UnaryFilter(fieldPath, Operator.LESS_THAN, value); } @NonNull @@ -126,7 +126,7 @@ public static Filter lessThanOrEqualTo(@NonNull String field, @Nullable Object v @NonNull public static Filter lessThanOrEqualTo(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.LESS_THAN_OR_EQUAL, value); + return new UnaryFilter(fieldPath, Operator.LESS_THAN_OR_EQUAL, value); } @NonNull @@ -136,7 +136,7 @@ public static Filter arrayContains(@NonNull String field, @Nullable Object value @NonNull public static Filter arrayContains(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.ARRAY_CONTAINS, value); + return new UnaryFilter(fieldPath, Operator.ARRAY_CONTAINS, value); } @NonNull @@ -146,7 +146,7 @@ public static Filter arrayContainsAny(@NonNull String field, @Nullable Object va @NonNull public static Filter arrayContainsAny(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.ARRAY_CONTAINS_ANY, value); + return new UnaryFilter(fieldPath, Operator.ARRAY_CONTAINS_ANY, value); } @NonNull @@ -156,7 +156,7 @@ public static Filter inArray(@NonNull String field, @Nullable Object value) { @NonNull public static Filter inArray(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.IN, value); + return new UnaryFilter(fieldPath, Operator.IN, value); } @NonNull @@ -166,7 +166,7 @@ public static Filter notInArray(@NonNull String field, @Nullable Object value) { @NonNull public static Filter notInArray(@NonNull FieldPath fieldPath, @Nullable Object value) { - return new FieldFilter(fieldPath, Operator.NOT_IN, value); + return new UnaryFilter(fieldPath, Operator.NOT_IN, value); } @NonNull diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java index 32f72454cfc..7c8f863a3b4 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java @@ -388,13 +388,13 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List Date: Thu, 20 Jan 2022 10:24:25 -0600 Subject: [PATCH 7/8] address comments. --- .../com/google/firebase/firestore/core/CompositeFilter.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java index c6f7c40a40e..580c71cc09c 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java @@ -44,6 +44,7 @@ public Operator getOperator() { @Override public List getFlattenedFilters() { + // TODO(orquery): memoize this result if this method is used more than once. List result = new ArrayList<>(); for (Filter subfilter : filters) { result.addAll(subfilter.getFlattenedFilters()); @@ -52,8 +53,8 @@ public List getFlattenedFilters() { } /** - * Returns the first inequality filter contained within this composite filter. Returns null if it - * does not contain any inequalities. + * Returns the first inequality filter contained within this composite filter. Returns {@code + * null} if it does not contain any inequalities. */ @Override public FieldPath getFirstInequalityField() { From 993be7abb8fde73a0f73bdbe385a77d85e50cc2e Mon Sep 17 00:00:00 2001 From: Ehsan Nasiri Date: Thu, 20 Jan 2022 10:49:00 -0600 Subject: [PATCH 8/8] Don't use an index for serving queries with composite filters yet. --- .../java/com/google/firebase/firestore/local/QueryEngine.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java index 467cbda8683..b3ef29dd168 100644 --- a/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java +++ b/firebase-firestore/src/main/java/com/google/firebase/firestore/local/QueryEngine.java @@ -131,7 +131,8 @@ public ImmutableSortedMap getDocumentsMatchingQuery( Query query, ImmutableSortedSet remoteKeys, SnapshotVersion lastLimboFreeSnapshotVersion) { - if (query.matchesAllDocuments()) { + // TODO(orquery): Update this condition when we are able to serve or queries from the index. + if (query.matchesAllDocuments() || query.containsCompositeFilters()) { // Don't use index queries that can be executed by scanning the collection. return null; }