Skip to content

Initial support for CompositeFilter class #3290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jan 20, 2022
Merged

Initial support for CompositeFilter class #3290

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
dc01726
Add composite filters.
ehsannas Dec 15, 2021
9238952
add inequalityField to Filter class.
ehsannas Jan 5, 2022
d206710
Address comments.
ehsannas Jan 12, 2022
b146b81
Return empty filter rather than null from parsing empty composite fil…
ehsannas Jan 12, 2022
651e953
Address comments.
ehsannas Jan 14, 2022
867f3b7
Use UnaryFilter.
ehsannas Jan 17, 2022
f33b042
address comments.
ehsannas Jan 20, 2022
993be7a
Don't use an index for serving queries with composite filters yet.
ehsannas Jan 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 66 additions & 30 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/Filter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,37 +17,60 @@
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;

/** @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;
}
static class FieldFilter extends Filter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not public and thus should not need the annotation. Same below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops. Done.

private final FieldPath field;
private final Operator operator;
private final Object value;

/** @hide */
@RestrictTo(RestrictTo.Scope.LIBRARY)
public FieldFilter.Operator getOperator() {
return operator;
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)
public Object getValue() {
return value;
static class CompositeFilter extends Filter {
private final List<Filter> filters;
private final StructuredQuery.CompositeFilter.Operator operator;

public CompositeFilter(
@NonNull List<Filter> filters, StructuredQuery.CompositeFilter.Operator operator) {
this.filters = filters;
this.operator = operator;
}

public List<Filter> getFilters() {
return filters;
}

public StructuredQuery.CompositeFilter.Operator getOperator() {
return operator;
}
}

@NonNull
Expand All @@ -57,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 Filter(fieldPath, FieldFilter.Operator.EQUAL, value);
return new FieldFilter(fieldPath, Operator.EQUAL, value);
}

@NonNull
Expand All @@ -67,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 Filter(fieldPath, FieldFilter.Operator.NOT_EQUAL, value);
return new FieldFilter(fieldPath, Operator.NOT_EQUAL, value);
}

@NonNull
Expand All @@ -77,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 Filter(fieldPath, FieldFilter.Operator.GREATER_THAN, value);
return new FieldFilter(fieldPath, Operator.GREATER_THAN, value);
}

@NonNull
Expand All @@ -87,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 Filter(fieldPath, FieldFilter.Operator.GREATER_THAN_OR_EQUAL, value);
return new FieldFilter(fieldPath, Operator.GREATER_THAN_OR_EQUAL, value);
}

@NonNull
Expand All @@ -97,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 Filter(fieldPath, FieldFilter.Operator.LESS_THAN, value);
return new FieldFilter(fieldPath, Operator.LESS_THAN, value);
}

@NonNull
Expand All @@ -107,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 Filter(fieldPath, FieldFilter.Operator.LESS_THAN_OR_EQUAL, value);
return new FieldFilter(fieldPath, Operator.LESS_THAN_OR_EQUAL, value);
}

@NonNull
Expand All @@ -117,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 Filter(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS, value);
return new FieldFilter(fieldPath, Operator.ARRAY_CONTAINS, value);
}

@NonNull
Expand All @@ -127,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 Filter(fieldPath, FieldFilter.Operator.ARRAY_CONTAINS_ANY, value);
return new FieldFilter(fieldPath, Operator.ARRAY_CONTAINS_ANY, value);
}

@NonNull
Expand All @@ -137,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 Filter(fieldPath, FieldFilter.Operator.IN, value);
return new FieldFilter(fieldPath, Operator.IN, value);
}

@NonNull
Expand All @@ -147,6 +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 Filter(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 CompositeFilter(
Arrays.asList(filters), StructuredQuery.CompositeFilter.Operator.OPERATOR_UNSPECIFIED);
}

@NonNull
public static Filter and(Filter... filters) {
return new CompositeFilter(
Arrays.asList(filters), StructuredQuery.CompositeFilter.Operator.AND);
}
}
160 changes: 115 additions & 45 deletions firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -387,15 +388,16 @@ public Query whereNotIn(@NonNull FieldPath fieldPath, @NonNull List<? extends Ob
}

/**
* Parses the given value object and creates a new {@code FieldFilter} with the given field,
* operator, and value. Also performs validation on the filter before retuning it.
* Takes a {@link Filter.FieldFilter} object, parses the value object and returns a new {@link
* FieldFilter} for the field, operator, and parsed value.
*
* @param fieldPath The field to compare
* @param op The operator
* @param value The value for parsing
* @return The created {@code FieldFilter}.
* @param fieldFilterData The Filter.FieldFilter object to parse.
* @return The created {@link FieldFilter}.
*/
private FieldFilter parseFieldFilter(@NonNull FieldPath fieldPath, Operator op, Object value) {
private FieldFilter parseFieldFilter(Filter.FieldFilter fieldFilterData) {
FieldPath fieldPath = fieldFilterData.getField();
Operator op = fieldFilterData.getOperator();
Object value = fieldFilterData.getValue();
checkNotNull(fieldPath, "Provided field path must not be null.");
checkNotNull(op, "Provided op must not be null.");
Value fieldValue;
Expand Down Expand Up @@ -426,15 +428,56 @@ private FieldFilter parseFieldFilter(@NonNull FieldPath fieldPath, Operator op,
.parseQueryValue(value, op == Operator.IN || op == Operator.NOT_IN);
}
FieldFilter filter = FieldFilter.create(fieldPath.getInternalPath(), op, fieldValue);
validateNewFilter(filter);
return filter;
}

/**
* Takes a {@link Filter.CompositeFilter} object, parses each of its subfilters, and a new {@link
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Word missing before "a new". The return type in the comment is also not correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Method no longer returns null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* given Filter.CompositeFilter contains only one field filter.
*/
private com.google.firebase.firestore.core.Filter parseCompositeFilter(
Filter.CompositeFilter compositeFilterData) {
List<com.google.firebase.firestore.core.Filter> parsedFilters = new ArrayList<>();
for (Filter filter : compositeFilterData.getFilters()) {
com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter);
if (!parsedFilter.getFlattenedFilters().isEmpty()) {
parsedFilters.add(parsedFilter);
}
}

// 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.
*/
private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) {
hardAssert(
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 parseCompositeFilter((Filter.CompositeFilter) 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 = parseFilter(filter);
if (parsedFilter.getFlattenedFilters().isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be more efficient to call getFilters() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it would be. Good suggestion. Done.

// Return the existing query if not adding any more filters (e.g. an empty composite filter).
return this;
}
validateNewFilter(parsedFilter);
return new Query(query.filter(parsedFilter), firestore);
}

private void validateOrderByField(com.google.firebase.firestore.model.FieldPath field) {
Expand Down Expand Up @@ -553,44 +596,71 @@ private List<Operator> 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();

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);
}
/** 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();
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()));
}
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 validateNewFilter(com.google.firebase.firestore.core.Filter filter) {
com.google.firebase.firestore.core.Query testQuery = query;
for (FieldFilter subfilter : filter.getFlattenedFilters()) {
validateNewFieldFilter(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
private Operator findFilterOperator(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/findFilterOperator/findFilterByOperarator or findFilterWithOperator?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

List<com.google.firebase.firestore.core.Filter> filters, List<Operator> 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;
}

/**
Expand Down
Loading