Skip to content

Commit be18970

Browse files
ehsannasehsannas
committed
Fix validation bug for composite filters.
1 parent e3ae02d commit be18970

File tree

4 files changed

+145
-14
lines changed

4 files changed

+145
-14
lines changed

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

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,12 @@
1414

1515
package com.google.firebase.firestore;
1616

17+
import static com.google.firebase.firestore.Filter.and;
18+
import static com.google.firebase.firestore.Filter.equalTo;
19+
import static com.google.firebase.firestore.Filter.greaterThan;
20+
import static com.google.firebase.firestore.Filter.inArray;
21+
import static com.google.firebase.firestore.Filter.notInArray;
22+
import static com.google.firebase.firestore.Filter.or;
1723
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.nullList;
1824
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.querySnapshotToIds;
1925
import static com.google.firebase.firestore.testutil.IntegrationTestUtil.querySnapshotToValues;
@@ -28,6 +34,7 @@
2834
import static org.junit.Assert.assertEquals;
2935
import static org.junit.Assert.assertFalse;
3036
import static org.junit.Assert.assertNull;
37+
import static org.junit.Assert.assertThrows;
3138
import static org.junit.Assert.assertTrue;
3239

3340
import androidx.test.ext.junit.runners.AndroidJUnit4;
@@ -37,6 +44,7 @@
3744
import com.google.firebase.firestore.testutil.EventAccumulator;
3845
import com.google.firebase.firestore.testutil.IntegrationTestUtil;
3946
import java.util.ArrayList;
47+
import java.util.Arrays;
4048
import java.util.LinkedHashMap;
4149
import java.util.List;
4250
import java.util.Map;
@@ -364,6 +372,72 @@ public void testCanListenForQueryMetadataChanges() {
364372
listener2.remove();
365373
}
366374

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

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

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -468,7 +468,7 @@ private com.google.firebase.firestore.core.Filter parseFilter(Filter filter) {
468468
}
469469

470470
// TODO(orquery): This method will become public API. Change visibility and add documentation.
471-
private Query where(Filter filter) {
471+
protected Query where(Filter filter) {
472472
com.google.firebase.firestore.core.Filter parsedFilter = parseFilter(filter);
473473
if (parsedFilter.getFilters().isEmpty()) {
474474
// Return the existing query if not adding any more filters (e.g. an empty composite filter).
@@ -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

@@ -648,13 +648,12 @@ private void validateNewFilter(com.google.firebase.firestore.core.Filter filter)
648648
* returns the first one that is, or null if none are.
649649
*/
650650
@Nullable
651-
private Operator findFilterWithOperator(
651+
protected 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
/**

‎firebase-firestore/src/test/java/com/google/firebase/firestore/QueryRoboTest.java

Lines changed: 55 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,22 @@
1414

1515
package com.google.firebase.firestore;
1616

17+
import static com.google.firebase.firestore.TestUtil.query;
18+
import static com.google.firebase.firestore.testutil.TestUtil.andFilters;
19+
import static com.google.firebase.firestore.testutil.TestUtil.filter;
20+
import static com.google.firebase.firestore.testutil.TestUtil.orFilters;
1721
import static org.junit.Assert.assertEquals;
1822
import static org.junit.Assert.assertNotEquals;
23+
import static org.junit.Assert.assertNotNull;
24+
import static org.junit.Assert.assertNull;
1925

26+
import com.google.firebase.firestore.core.CompositeFilter;
27+
import com.google.firebase.firestore.core.FieldFilter;
28+
import com.google.firebase.firestore.core.FieldFilter.Operator;
29+
import com.google.firebase.firestore.core.Filter;
30+
import java.util.Arrays;
31+
import java.util.Collections;
32+
import java.util.List;
2033
import org.junit.Test;
2134
import org.junit.runner.RunWith;
2235
import org.robolectric.RobolectricTestRunner;
@@ -29,13 +42,52 @@ public class QueryRoboTest {
2942

3043
@Test
3144
public void testEquals() {
32-
Query foo = TestUtil.query("foo");
33-
Query fooDup = TestUtil.query("foo");
34-
Query bar = TestUtil.query("bar");
45+
Query foo = query("foo");
46+
Query fooDup = query("foo");
47+
Query bar = query("bar");
3548
assertEquals(foo, fooDup);
3649
assertNotEquals(foo, bar);
3750

3851
assertEquals(foo.hashCode(), fooDup.hashCode());
3952
assertNotEquals(foo.hashCode(), bar.hashCode());
4053
}
54+
55+
@Test
56+
public void testFindFilterWithOperator() {
57+
CompositeFilter compositeFilter =
58+
andFilters(
59+
orFilters(filter("a", "==", "b"), filter("c", ">", "d")),
60+
orFilters(filter("a", "==", "b"), filter("c", "!=", "d")));
61+
FieldFilter fieldFilter = filter("r", "<", "s");
62+
List<Filter> filters = Arrays.asList(compositeFilter, fieldFilter);
63+
64+
// Check that '==', '>', and '!=' (nested within the composite filter) and '<' (in the
65+
// field filter) are found.
66+
Query query1 = query("coll");
67+
Operator foundEqual =
68+
query1.findFilterWithOperator(filters, Collections.singletonList(Operator.EQUAL));
69+
Operator foundGreaterThan =
70+
query1.findFilterWithOperator(filters, Collections.singletonList(Operator.GREATER_THAN));
71+
Operator foundNotEqual =
72+
query1.findFilterWithOperator(filters, Collections.singletonList(Operator.NOT_EQUAL));
73+
Operator foundLessThan =
74+
query1.findFilterWithOperator(filters, Collections.singletonList(Operator.LESS_THAN));
75+
assertNotNull(foundEqual);
76+
assertNotNull(foundGreaterThan);
77+
assertNotNull(foundNotEqual);
78+
assertNotNull(foundLessThan);
79+
80+
// Check that all other operators cannot be found.
81+
Operator foundOtherOperators =
82+
query1.findFilterWithOperator(
83+
filters,
84+
Arrays.asList(
85+
Operator.LESS_THAN_OR_EQUAL,
86+
Operator.GREATER_THAN_OR_EQUAL,
87+
Operator.ARRAY_CONTAINS,
88+
Operator.ARRAY_CONTAINS_ANY,
89+
Operator.IN,
90+
Operator.NOT_IN));
91+
assertNull(foundOtherOperators);
92+
}
4193
}

0 commit comments

Comments
 (0)