Skip to content

Commit e01b3a1

Browse files
milaGGLmilaGGL
authored
fix: Add tests for multiple inequality support (#1392)
1 parent 0eadae0 commit e01b3a1

File tree

7 files changed

+773
-110
lines changed

7 files changed

+773
-110
lines changed

‎README.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,13 +57,13 @@ implementation 'com.google.cloud:google-cloud-firestore'
5757
If you are using Gradle without BOM, add this to your dependencies:
5858

5959
```Groovy
60-
implementation 'com.google.cloud:google-cloud-firestore:3.14.2'
60+
implementation 'com.google.cloud:google-cloud-firestore:3.14.3'
6161
```
6262

6363
If you are using SBT, add this to your dependencies:
6464

6565
```Scala
66-
libraryDependencies += "com.google.cloud" % "google-cloud-firestore" % "3.14.2"
66+
libraryDependencies += "com.google.cloud" % "google-cloud-firestore" % "3.14.3"
6767
```
6868
<!-- {x-version-update-end} -->
6969

@@ -222,7 +222,7 @@ Java is a registered trademark of Oracle and/or its affiliates.
222222
[kokoro-badge-link-5]: http://storage.googleapis.com/cloud-devrel-public/java/badges/java-firestore/java11.html
223223
[stability-image]: https://img.shields.io/badge/stability-stable-green
224224
[maven-version-image]: https://img.shields.io/maven-central/v/com.google.cloud/google-cloud-firestore.svg
225-
[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-firestore/3.14.2
225+
[maven-version-link]: https://central.sonatype.com/artifact/com.google.cloud/google-cloud-firestore/3.14.3
226226
[authentication]: https://github.com/googleapis/google-cloud-java#authentication
227227
[auth-scopes]: https://developers.google.com/identity/protocols/oauth2/scopes
228228
[predefined-iam-roles]: https://cloud.google.com/iam/docs/understanding-roles#predefined_roles

‎google-cloud-firestore/src/main/java/com/google/cloud/firestore/FieldPath.java‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
import com.google.common.base.Preconditions;
2121
import com.google.common.collect.ImmutableList;
2222
import com.google.firestore.v1.StructuredQuery;
23+
import java.util.ArrayList;
24+
import java.util.List;
2325
import java.util.regex.Pattern;
2426
import javax.annotation.Nonnull;
2527

@@ -83,6 +85,62 @@ static FieldPath fromDotSeparatedString(String field) {
8385
return empty().append(field);
8486
}
8587

88+
/**
89+
* Creates a {@code FieldPath} from a server-encoded field path.
90+
*
91+
* <p>Copied from Firebase Android SDK:
92+
* https://github.com/firebase/firebase-android-sdk/blob/2d3b2be7d2d00d693eb74986f20a6265c918848f/firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldPath.java#L47
93+
*/
94+
public static FieldPath fromServerFormat(String path) {
95+
List<String> res = new ArrayList<>();
96+
StringBuilder builder = new StringBuilder();
97+
98+
int i = 0;
99+
100+
// If we're inside '`' backticks, then we should ignore '.' dots.
101+
boolean inBackticks = false;
102+
103+
while (i < path.length()) {
104+
char c = path.charAt(i);
105+
if (c == '\\') {
106+
if (i + 1 == path.length()) {
107+
throw new IllegalArgumentException("Trailing escape character is not allowed");
108+
}
109+
i++;
110+
builder.append(path.charAt(i));
111+
} else if (c == '.') {
112+
if (!inBackticks) {
113+
String elem = builder.toString();
114+
if (elem.isEmpty()) {
115+
throw new IllegalArgumentException(
116+
"Invalid field path ("
117+
+ path
118+
+ "). Paths must not be empty, begin with '.', end with '.', or contain '..'");
119+
}
120+
builder = new StringBuilder();
121+
res.add(elem);
122+
} else {
123+
// escaped, append to current segment
124+
builder.append(c);
125+
}
126+
} else if (c == '`') {
127+
inBackticks = !inBackticks;
128+
} else {
129+
builder.append(c);
130+
}
131+
i++;
132+
}
133+
String lastElem = builder.toString();
134+
if (lastElem.isEmpty()) {
135+
throw new IllegalArgumentException(
136+
"Invalid field path ("
137+
+ path
138+
+ "). Paths must not be empty, begin with '.', end with '.', or contain '..'");
139+
}
140+
res.add(lastElem);
141+
return FieldPath.of(res.toArray(new String[0]));
142+
}
143+
86144
/** Returns an empty field path. */
87145
static FieldPath empty() {
88146
// NOTE: This is not static since it would create a circular class dependency during

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

Lines changed: 56 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,13 @@
6363
import java.util.ArrayList;
6464
import java.util.Collections;
6565
import java.util.Comparator;
66+
import java.util.HashSet;
6667
import java.util.Iterator;
6768
import java.util.List;
6869
import java.util.Objects;
6970
import java.util.Set;
71+
import java.util.SortedSet;
72+
import java.util.TreeSet;
7073
import java.util.concurrent.Executor;
7174
import java.util.concurrent.atomic.AtomicReference;
7275
import java.util.logging.Logger;
@@ -285,7 +288,9 @@ boolean isInequalityFilter() {
285288
return operator.equals(GREATER_THAN)
286289
|| operator.equals(GREATER_THAN_OR_EQUAL)
287290
|| operator.equals(LESS_THAN)
288-
|| operator.equals(LESS_THAN_OR_EQUAL);
291+
|| operator.equals(LESS_THAN_OR_EQUAL)
292+
|| operator.equals(NOT_EQUAL)
293+
|| operator.equals(NOT_IN);
289294
}
290295

291296
@Nullable
@@ -327,6 +332,11 @@ static final class FieldOrder {
327332
this.direction = direction;
328333
}
329334

335+
FieldOrder(String field, Direction direction) {
336+
this.fieldReference = FieldPath.fromServerFormat(field).toProto();
337+
this.direction = direction;
338+
}
339+
330340
Order toProto() {
331341
Order.Builder result = Order.newBuilder();
332342
result.setField(fieldReference);
@@ -462,39 +472,57 @@ private static boolean isUnaryComparison(@Nullable Object value) {
462472
return value == null || value.equals(Double.NaN) || value.equals(Float.NaN);
463473
}
464474

465-
/** Computes the backend ordering semantics for DocumentSnapshot cursors. */
466-
private ImmutableList<FieldOrder> createImplicitOrderBy() {
467-
List<FieldOrder> implicitOrders = new ArrayList<>(options.getFieldOrders());
468-
469-
// If no explicit ordering is specified, use the first inequality to define an implicit order.
470-
if (implicitOrders.isEmpty()) {
471-
for (FilterInternal filter : options.getFilters()) {
472-
FieldReference fieldReference = filter.getFirstInequalityField();
473-
if (fieldReference != null) {
474-
implicitOrders.add(new FieldOrder(fieldReference, Direction.ASCENDING));
475-
break;
475+
/** Returns the sorted set of inequality filter fields used in this query. */
476+
private SortedSet<FieldPath> getInequalityFilterFields() {
477+
SortedSet<FieldPath> result = new TreeSet<>();
478+
479+
for (FilterInternal filter : options.getFilters()) {
480+
for (FieldFilterInternal subFilter : filter.getFlattenedFilters()) {
481+
if (subFilter.isInequalityFilter()) {
482+
result.add(FieldPath.fromServerFormat(subFilter.fieldReference.getFieldPath()));
476483
}
477484
}
478485
}
479486

480-
boolean hasDocumentId = false;
481-
for (FieldOrder fieldOrder : implicitOrders) {
482-
if (FieldPath.isDocumentId(fieldOrder.fieldReference.getFieldPath())) {
483-
hasDocumentId = true;
487+
return result;
488+
}
489+
490+
/** Computes the backend ordering semantics for DocumentSnapshot cursors. */
491+
ImmutableList<FieldOrder> createImplicitOrderBy() {
492+
// Any explicit order by fields should be added as is.
493+
List<FieldOrder> result = new ArrayList<>(options.getFieldOrders());
494+
495+
HashSet<String> fieldsNormalized = new HashSet<>();
496+
for (FieldOrder order : result) {
497+
fieldsNormalized.add(order.fieldReference.getFieldPath());
498+
}
499+
500+
/** The order of the implicit ordering always matches the last explicit order by. */
501+
Direction lastDirection =
502+
result.isEmpty() ? Direction.ASCENDING : result.get(result.size() - 1).direction;
503+
504+
/**
505+
* Any inequality fields not explicitly ordered should be implicitly ordered in a
506+
* lexicographical order. When there are multiple inequality filters on the same field, the
507+
* field should be added only once.
508+
*
509+
* <p>Note: `SortedSet<FieldPath>` sorts the key field before other fields. However, we want the
510+
* key field to be sorted last.
511+
*/
512+
SortedSet<FieldPath> inequalityFields = getInequalityFilterFields();
513+
for (FieldPath field : inequalityFields) {
514+
if (!fieldsNormalized.contains(field.toString())
515+
&& !FieldPath.isDocumentId(field.toString())) {
516+
result.add(new FieldOrder(field.toProto(), lastDirection));
484517
}
485518
}
486519

487-
if (!hasDocumentId) {
488-
// Add implicit sorting by name, using the last specified direction.
489-
Direction lastDirection =
490-
implicitOrders.isEmpty()
491-
? Direction.ASCENDING
492-
: implicitOrders.get(implicitOrders.size() - 1).direction;
493-
494-
implicitOrders.add(new FieldOrder(FieldPath.documentId().toProto(), lastDirection));
520+
// Add the document key field to the last if it is not explicitly ordered.
521+
if (!fieldsNormalized.contains(FieldPath.documentId().toString())) {
522+
result.add(new FieldOrder(FieldPath.documentId().toProto(), lastDirection));
495523
}
496524

497-
return ImmutableList.<FieldOrder>builder().addAll(implicitOrders).build();
525+
return ImmutableList.<FieldOrder>builder().addAll(result).build();
498526
}
499527

500528
private Cursor createCursor(
@@ -506,11 +534,12 @@ private Cursor createCursor(
506534
if (FieldPath.isDocumentId(path)) {
507535
fieldValues.add(documentSnapshot.getReference());
508536
} else {
509-
FieldPath fieldPath = FieldPath.fromDotSeparatedString(path);
537+
FieldPath fieldPath = FieldPath.fromServerFormat(path);
510538
Preconditions.checkArgument(
511539
documentSnapshot.contains(fieldPath),
512540
"Field '%s' is missing in the provided DocumentSnapshot. Please provide a document "
513-
+ "that contains values for all specified orderBy() and where() constraints.");
541+
+ "that contains values for all specified orderBy() and where() constraints.",
542+
fieldPath);
514543
fieldValues.add(documentSnapshot.get(fieldPath));
515544
}
516545
}

0 commit comments

Comments
 (0)