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 2 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
111 changes: 73 additions & 38 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 @@ -18,46 +18,21 @@
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);
}

@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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

s/FieldFilterData/Filter.UnaryFilter ?

We don't use non-public classes: https://engdoc.corp.google.com/eng/doc/devguide/java/style/index.md?cl=head#s3.4.1-one-top-level-class. You can make this an inner class instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using an inner class now.

I don't want to use UnaryFilter as it could get confused with

firebase-android-sdk/firebase-firestore/src/proto/google/firestore/v1/query.proto

Lines 168 to 170 in ac3276c

// A filter with a single operand.
message UnaryFilter {
// A unary operator.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is also already a FieldFilter class. Between two FieldFilters and two UnaryFilters, I would pick two UnaryFilters as the naming UnaryFilter vs CompositeFilter offers clear and obvious distinctions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pinging this once more as I find the current naming a bit hard to follow. It is not obvious to me that a CompositeFilter is not a FieldFilter.

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

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

public FieldFilterData(@NonNull FieldPath field, FieldFilter.Operator operator, Object value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not 100% certain but it seems like we don't use @NonNull in non-public methods, so I would drop it here and below. NonNull is assumed to be the default.

While we are at it "value" is nullable though.

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

this.field = field;
this.operator = operator;
this.value = value;
}

public FieldPath getField() {
return field;
}

public FieldFilter.Operator getOperator() {
return operator;
}

public Object getValue() {
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 actually Nullable.

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

return value;
}
}

/** @hide */
@RestrictTo(RestrictTo.Scope.LIBRARY)
class CompositeFilterData extends Filter {
private final List<Filter> filters;
private final StructuredQuery.CompositeFilter.Operator operator;

public CompositeFilterData(
@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;
}
}
163 changes: 120 additions & 43 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,17 @@ 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 {@code FieldFilterData} object, parses the value object and returns a new {@code
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Takes a {@code FieldFilterData} object, parses the value object and returns a new {@code
* Takes a {@link FieldFilterData} object, parses the value object and returns a new {@code

That way, we can click on it and it gets renamed if we use IDE refactoring.

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

* FieldFilter} for the field, operator, and parsed value.
*
* @param fieldPath The field to compare
* @param op The operator
* @param value The value for parsing
* @param fieldFilterData The FieldFilterData object to parse.
* @return The created {@code FieldFilter}.
*/
private FieldFilter parseFieldFilter(@NonNull FieldPath fieldPath, Operator op, Object value) {
@NonNull
private FieldFilter parseFieldFilterData(@NonNull FieldFilterData fieldFilterData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I think we can drop NonNull here, even though we are a bit inconsistent with the replaced method.

s/parseFieldFilterData/parseUnaryFilter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dropped NonNull. Don't think using UnaryFilter is a good idea.

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 +429,62 @@ 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 {@code CompositeFilterData} object, parses each of its subfilters, and a new {@code
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Takes a {@code CompositeFilterData} object, parses each of its subfilters, and a new {@code
* Takes a {@link CompositeFilterData} object, parses each of its subfilters, and creates a new {@link

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
* 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(
@NonNull CompositeFilterData compositeFilterData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private com.google.firebase.firestore.core.Filter parseCompositeFilterData(
@NonNull CompositeFilterData compositeFilterData) {
private com.google.firebase.firestore.core.Filter parseCompositeFilter(
CompositeFilter compositeFilterData) {

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> 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()) {
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 simplify our code if we returned an empty filter instead?

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

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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Random though: Would it be possible to have a Filter.parse() method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the idea because it'll get rid of the instanceof checking here. But that'd require passing both FirebaseFirestore object and the core/Query object to the Filter.parse() method.

the signature would have to be the following:

public com.google.firebase.firestore.core.Filter parse(FirebaseFirestore firestore, com.google.firebase.firestore.core.Query query);

because the parsing logic needs these.

not sure if it's worth it.

}

// 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;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (parsedFilter == null) {
// Return the existing query if not adding any more filters (e.g. an empty composite filter).
return this;
}

Found a simplification if we make filters non-nullable :)

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

validateAddingNewFilter(parsedFilter);
return new Query(query.filter(parsedFilter), firestore);
}

private void validateOrderByField(com.google.firebase.firestore.model.FieldPath field) {
Expand Down Expand Up @@ -553,44 +603,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();
/** Checks that adding the given field filter to the given query is valid */
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: "yields a valid query"?

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

private void validateAddingNewFieldFilter(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ultranit: "adding" and "new" is kinda the same. Feel free to ignore.

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

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
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