Skip to content

Commit 7a59eed

Browse files
committed
Fix validation bug for composite filters.
1 parent e3ae02d commit 7a59eed

File tree

3 files changed

+83
-9
lines changed

3 files changed

+83
-9
lines changed

firebase-firestore/src/androidTest/java/com/google/firebase/firestore/QueryTest.java

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -364,6 +364,75 @@ public void testCanListenForQueryMetadataChanges() {
364364
listener2.remove();
365365
}
366366

367+
/*
368+
// TODO(orquery): Re-enable this test once the `where` API is public.
369+
@Test
370+
public void testInvalidQueryFilters() {
371+
CollectionReference collection = testCollection();
372+
// Multiple inequalities, one of which is inside a nested composite filter.
373+
assertThrows(
374+
IllegalArgumentException.class,
375+
() -> {
376+
Query query =
377+
collection
378+
.where(
379+
or(
380+
and(equalTo("a", "b"), greaterThan("c", "d")),
381+
and(equalTo("e", "f"), equalTo("g", "h"))))
382+
.where(greaterThan("r", "s"));
383+
});
384+
// OrderBy and inequality on different fields. Inequality inside a nested composite filter.
385+
assertThrows(
386+
IllegalArgumentException.class,
387+
() -> {
388+
Query query =
389+
collection
390+
.where(
391+
or(
392+
and(equalTo("a", "b"), greaterThan("c", "d")),
393+
and(equalTo("e", "f"), equalTo("g", "h"))))
394+
.orderBy("r");
395+
});
396+
// Conflicting operations within a composite filter.
397+
assertThrows(
398+
IllegalArgumentException.class,
399+
() -> {
400+
Query query =
401+
collection.where(
402+
or(
403+
and(equalTo("a", "b"), inArray("c", Arrays.asList("d", "e"))),
404+
and(equalTo("e", "f"), notInArray("c", Arrays.asList("d", "e")))));
405+
});
406+
// Conflicting operations between a field filter and a composite filter.
407+
assertThrows(
408+
IllegalArgumentException.class,
409+
() -> {
410+
Query query =
411+
collection
412+
.where(
413+
or(
414+
and(equalTo("a", "b"), inArray("c", Arrays.asList("d", "e"))),
415+
and(equalTo("e", "f"), equalTo("g", "h"))))
416+
.where(notInArray("c", Arrays.asList("d", "e")));
417+
});
418+
// Conflicting operations between two composite filters.
419+
assertThrows(
420+
IllegalArgumentException.class,
421+
() -> {
422+
Query query =
423+
collection
424+
.where(
425+
or(
426+
and(equalTo("a", "b"), inArray("c", Arrays.asList("d", "e"))),
427+
and(equalTo("e", "f"), equalTo("g", "h"))))
428+
.where(
429+
or(
430+
and(equalTo("a", "b"), notInArray("c", Arrays.asList("d", "e"))),
431+
and(equalTo("e", "f"), equalTo("g", "h"))));
432+
});
433+
}
434+
*/
435+
367436
@Test
368437
public void testCanExplicitlySortByDocumentId() {
369438
Map<String, Map<String, Object>> testDocs =

firebase-firestore/src/main/java/com/google/firebase/firestore/Query.java

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -639,7 +639,7 @@ private void validateNewFilter(com.google.firebase.firestore.core.Filter filter)
639639
com.google.firebase.firestore.core.Query testQuery = query;
640640
for (FieldFilter subfilter : filter.getFlattenedFilters()) {
641641
validateNewFieldFilter(testQuery, subfilter);
642-
testQuery = query.filter(subfilter);
642+
testQuery = testQuery.filter(subfilter);
643643
}
644644
}
645645

@@ -651,10 +651,9 @@ private void validateNewFilter(com.google.firebase.firestore.core.Filter filter)
651651
private Operator findFilterWithOperator(
652652
List<com.google.firebase.firestore.core.Filter> filters, List<Operator> operators) {
653653
for (com.google.firebase.firestore.core.Filter filter : filters) {
654-
if (filter instanceof FieldFilter) {
655-
Operator filterOp = ((FieldFilter) filter).getOperator();
656-
if (operators.contains(filterOp)) {
657-
return filterOp;
654+
for (FieldFilter fieldFilter : filter.getFlattenedFilters()) {
655+
if (operators.contains(fieldFilter.getOperator())) {
656+
return fieldFilter.getOperator();
658657
}
659658
}
660659
}

firebase-firestore/src/main/java/com/google/firebase/firestore/core/CompositeFilter.java

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ public class CompositeFilter extends Filter {
2828
private final List<Filter> filters;
2929
private final Operator operator;
3030

31+
// Memoized list of all field filters that can be found by traversing the tree of filters
32+
// contained in this composite filter.
33+
private List<FieldFilter> flattenedFilters;
34+
3135
public CompositeFilter(List<Filter> filters, Operator operator) {
3236
this.filters = filters;
3337
this.operator = operator;
@@ -44,12 +48,14 @@ public Operator getOperator() {
4448

4549
@Override
4650
public List<FieldFilter> getFlattenedFilters() {
47-
// TODO(orquery): memoize this result if this method is used more than once.
48-
List<FieldFilter> result = new ArrayList<>();
51+
if (flattenedFilters != null) {
52+
return flattenedFilters;
53+
}
54+
flattenedFilters = new ArrayList<>();
4955
for (Filter subfilter : filters) {
50-
result.addAll(subfilter.getFlattenedFilters());
56+
flattenedFilters.addAll(subfilter.getFlattenedFilters());
5157
}
52-
return result;
58+
return flattenedFilters;
5359
}
5460

5561
/**

0 commit comments

Comments
 (0)