Skip to content

Commit 16c8053

Browse files
schmidt-sebastianschmidt-sebastian
committed
Change Bound's before to inclusive
This is supposed to reduce confusion as to what means for startAt() and endAt().
1 parent c30a15a commit 16c8053

File tree

13 files changed

+111
-123
lines changed

13 files changed

+111
-123
lines changed

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

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -705,7 +705,7 @@ public Query limitToLast(long limit) {
705705
*/
706706
@NonNull
707707
public Query startAt(@NonNull DocumentSnapshot snapshot) {
708-
Bound bound = boundFromDocumentSnapshot("startAt", snapshot, /*before=*/ true);
708+
Bound bound = boundFromDocumentSnapshot("startAt", snapshot, /*inclusive=*/ true);
709709
return new Query(query.startAt(bound), firestore);
710710
}
711711

@@ -719,7 +719,7 @@ public Query startAt(@NonNull DocumentSnapshot snapshot) {
719719
*/
720720
@NonNull
721721
public Query startAt(Object... fieldValues) {
722-
Bound bound = boundFromFields("startAt", fieldValues, /*before=*/ true);
722+
Bound bound = boundFromFields("startAt", fieldValues, /*inclusive=*/ true);
723723
return new Query(query.startAt(bound), firestore);
724724
}
725725

@@ -733,7 +733,7 @@ public Query startAt(Object... fieldValues) {
733733
*/
734734
@NonNull
735735
public Query startAfter(@NonNull DocumentSnapshot snapshot) {
736-
Bound bound = boundFromDocumentSnapshot("startAfter", snapshot, /*before=*/ false);
736+
Bound bound = boundFromDocumentSnapshot("startAfter", snapshot, /*inclusive=*/ false);
737737
return new Query(query.startAt(bound), firestore);
738738
}
739739

@@ -748,7 +748,7 @@ public Query startAfter(@NonNull DocumentSnapshot snapshot) {
748748
*/
749749
@NonNull
750750
public Query startAfter(Object... fieldValues) {
751-
Bound bound = boundFromFields("startAfter", fieldValues, /*before=*/ false);
751+
Bound bound = boundFromFields("startAfter", fieldValues, /*inclusive=*/ false);
752752
return new Query(query.startAt(bound), firestore);
753753
}
754754

@@ -762,7 +762,7 @@ public Query startAfter(Object... fieldValues) {
762762
*/
763763
@NonNull
764764
public Query endBefore(@NonNull DocumentSnapshot snapshot) {
765-
Bound bound = boundFromDocumentSnapshot("endBefore", snapshot, /*before=*/ true);
765+
Bound bound = boundFromDocumentSnapshot("endBefore", snapshot, /*inclusive=*/ false);
766766
return new Query(query.endAt(bound), firestore);
767767
}
768768

@@ -776,7 +776,7 @@ public Query endBefore(@NonNull DocumentSnapshot snapshot) {
776776
*/
777777
@NonNull
778778
public Query endBefore(Object... fieldValues) {
779-
Bound bound = boundFromFields("endBefore", fieldValues, /*before=*/ true);
779+
Bound bound = boundFromFields("endBefore", fieldValues, /*inclusive=*/ false);
780780
return new Query(query.endAt(bound), firestore);
781781
}
782782

@@ -790,7 +790,7 @@ public Query endBefore(Object... fieldValues) {
790790
*/
791791
@NonNull
792792
public Query endAt(@NonNull DocumentSnapshot snapshot) {
793-
Bound bound = boundFromDocumentSnapshot("endAt", snapshot, /*before=*/ false);
793+
Bound bound = boundFromDocumentSnapshot("endAt", snapshot, /*inclusive=*/ true);
794794
return new Query(query.endAt(bound), firestore);
795795
}
796796

@@ -804,7 +804,7 @@ public Query endAt(@NonNull DocumentSnapshot snapshot) {
804804
*/
805805
@NonNull
806806
public Query endAt(Object... fieldValues) {
807-
Bound bound = boundFromFields("endAt", fieldValues, /*before=*/ false);
807+
Bound bound = boundFromFields("endAt", fieldValues, /*inclusive=*/ true);
808808
return new Query(query.endAt(bound), firestore);
809809
}
810810

@@ -818,7 +818,7 @@ public Query endAt(Object... fieldValues) {
818818
* any of the fields in the order by are an uncommitted server timestamp.
819819
*/
820820
private Bound boundFromDocumentSnapshot(
821-
String methodName, DocumentSnapshot snapshot, boolean before) {
821+
String methodName, DocumentSnapshot snapshot, boolean inclusive) {
822822
checkNotNull(snapshot, "Provided snapshot must not be null.");
823823
if (!snapshot.exists()) {
824824
throw new IllegalArgumentException(
@@ -857,11 +857,11 @@ private Bound boundFromDocumentSnapshot(
857857
}
858858
}
859859
}
860-
return new Bound(components, before);
860+
return new Bound(components, inclusive);
861861
}
862862

863863
/** Converts a list of field values to Bound. */
864-
private Bound boundFromFields(String methodName, Object[] values, boolean before) {
864+
private Bound boundFromFields(String methodName, Object[] values, boolean inclusive) {
865865
// Use explicit order by's because it has to match the query the user made
866866
List<OrderBy> explicitOrderBy = query.getExplicitOrderBy();
867867
if (values.length > explicitOrderBy.size()) {
@@ -913,7 +913,7 @@ private Bound boundFromFields(String methodName, Object[] values, boolean before
913913
}
914914
}
915915

916-
return new Bound(components, before);
916+
return new Bound(components, inclusive);
917917
}
918918

919919
/**

‎firebase-firestore/src/main/java/com/google/firebase/firestore/bundle/BundleSerializer.java

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -146,8 +146,8 @@ private BundledQuery decodeBundledQuery(JSONObject bundledQuery) throws JSONExce
146146

147147
List<Filter> filters = decodeWhere(structuredQuery.optJSONObject("where"));
148148
List<OrderBy> orderBys = decodeOrderBy(structuredQuery.optJSONArray("orderBy"));
149-
@Nullable Bound startAt = decodeBound(structuredQuery.optJSONObject("startAt"));
150-
@Nullable Bound endAt = decodeBound(structuredQuery.optJSONObject("endAt"));
149+
@Nullable Bound startAt = decodeStartAtBound(structuredQuery.optJSONObject("startAt"));
150+
@Nullable Bound endAt = decodeEndAtBound(structuredQuery.optJSONObject("endAt"));
151151

152152
verifyNoOffset(structuredQuery);
153153
int limit = decodeLimit(structuredQuery);
@@ -167,24 +167,35 @@ private int decodeLimit(JSONObject structuredQuery) {
167167
}
168168
}
169169

170-
private Bound decodeBound(@Nullable JSONObject bound) throws JSONException {
170+
private Bound decodeStartAtBound(@Nullable JSONObject bound) throws JSONException {
171171
if (bound != null) {
172-
List<Value> cursor = new ArrayList<>();
173172
boolean before = bound.optBoolean("before", false);
174-
175-
JSONArray values = bound.optJSONArray("values");
176-
if (values != null) {
177-
for (int i = 0; i < values.length(); ++i) {
178-
cursor.add(decodeValue(values.getJSONObject(i)));
179-
}
180-
}
181-
182-
return new Bound(cursor, before);
173+
List<Value> position = decodePosition(bound);
174+
return new Bound(position, before);
183175
}
176+
return null;
177+
}
184178

179+
private Bound decodeEndAtBound(@Nullable JSONObject bound) throws JSONException {
180+
if (bound != null) {
181+
boolean before = bound.optBoolean("before", false);
182+
List<Value> position = decodePosition(bound);
183+
return new Bound(position, !before);
184+
}
185185
return null;
186186
}
187187

188+
private List<Value> decodePosition(JSONObject bound) throws JSONException {
189+
List<Value> cursor = new ArrayList<>();
190+
JSONArray values = bound.optJSONArray("values");
191+
if (values != null) {
192+
for (int i = 0; i < values.length(); ++i) {
193+
cursor.add(decodeValue(values.getJSONObject(i)));
194+
}
195+
}
196+
return cursor;
197+
}
198+
188199
private List<OrderBy> decodeOrderBy(@Nullable JSONArray orderBys) throws JSONException {
189200
List<OrderBy> result = new ArrayList<>();
190201

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/Bound.java

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,33 +38,31 @@
3838
*/
3939
public final class Bound {
4040

41-
/** Whether this bound is just before or just after the provided position */
42-
private final boolean before;
41+
/**
42+
* Whether this bound includes the provided position (e.g. for {#code startAt()} or {#code
43+
* endAt()})
44+
*/
45+
private final boolean inclusive;
4346

4447
/** The index position of this bound */
4548
private final List<Value> position;
4649

47-
public Bound(List<Value> position, boolean before) {
50+
public Bound(List<Value> position, boolean inclusive) {
4851
this.position = position;
49-
this.before = before;
52+
this.inclusive = inclusive;
5053
}
5154

5255
public List<Value> getPosition() {
5356
return position;
5457
}
5558

56-
public boolean isBefore() {
57-
return before;
59+
public boolean isInclusive() {
60+
return inclusive;
5861
}
5962

60-
public String canonicalString() {
63+
public String positionString() {
6164
// TODO: Make this collision robust.
6265
StringBuilder builder = new StringBuilder();
63-
if (before) {
64-
builder.append("b:");
65-
} else {
66-
builder.append("a:");
67-
}
6866
boolean first = true;
6967
for (Value indexComponent : position) {
7068
if (!first) {
@@ -106,7 +104,7 @@ public boolean sortsBeforeDocument(List<OrderBy> orderBy, Document document) {
106104
}
107105
}
108106

109-
return before ? comparison <= 0 : comparison < 0;
107+
return inclusive ? comparison <= 0 : comparison < 0;
110108
}
111109

112110
@Override
@@ -120,21 +118,21 @@ public boolean equals(Object o) {
120118

121119
Bound bound = (Bound) o;
122120

123-
return before == bound.before && position.equals(bound.position);
121+
return inclusive == bound.inclusive && position.equals(bound.position);
124122
}
125123

126124
@Override
127125
public int hashCode() {
128-
int result = (before ? 1 : 0);
126+
int result = (inclusive ? 1 : 0);
129127
result = 31 * result + position.hashCode();
130128
return result;
131129
}
132130

133131
@Override
134132
public String toString() {
135133
StringBuilder builder = new StringBuilder();
136-
builder.append("Bound(before=");
137-
builder.append(before);
134+
builder.append("Bound(inclusive=");
135+
builder.append(inclusive);
138136
builder.append(", position=");
139137
for (int i = 0; i < position.size(); i++) {
140138
if (i > 0) {

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -520,10 +520,12 @@ public Target toTarget() {
520520

521521
// We need to swap the cursors to match the now-flipped query ordering.
522522
Bound newStartAt =
523-
this.endAt != null ? new Bound(this.endAt.getPosition(), !this.endAt.isBefore()) : null;
523+
this.endAt != null
524+
? new Bound(this.endAt.getPosition(), !this.endAt.isInclusive())
525+
: null;
524526
Bound newEndAt =
525527
this.startAt != null
526-
? new Bound(this.startAt.getPosition(), !this.startAt.isBefore())
528+
? new Bound(this.startAt.getPosition(), !this.startAt.isInclusive())
527529
: null;
528530

529531
this.memoizedTarget =

‎firebase-firestore/src/main/java/com/google/firebase/firestore/core/Target.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
180180
Value cursorValue = startAt.getPosition().get(i);
181181
if (max(segmentValue, cursorValue) == cursorValue) {
182182
segmentValue = cursorValue;
183-
segmentInclusive = startAt.isBefore();
183+
segmentInclusive = startAt.isInclusive();
184184
}
185185
break;
186186
}
@@ -256,7 +256,7 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
256256
Value cursorValue = endAt.getPosition().get(i);
257257
if (min(segmentValue, cursorValue) == cursorValue) {
258258
segmentValue = cursorValue;
259-
segmentInclusive = !endAt.isBefore();
259+
segmentInclusive = endAt.isInclusive();
260260
}
261261
break;
262262
}
@@ -276,7 +276,7 @@ public Bound getLowerBound(FieldIndex fieldIndex) {
276276
return null;
277277
}
278278

279-
return new Bound(values, !inclusive);
279+
return new Bound(values, inclusive);
280280
}
281281

282282
public List<OrderBy> getOrderBy() {
@@ -318,12 +318,14 @@ public String getCanonicalId() {
318318

319319
if (startAt != null) {
320320
builder.append("|lb:");
321-
builder.append(startAt.canonicalString());
321+
builder.append(startAt.isInclusive() ? "b:" : "a:");
322+
builder.append(startAt.positionString());
322323
}
323324

324325
if (endAt != null) {
325326
builder.append("|ub:");
326-
builder.append(endAt.canonicalString());
327+
builder.append(endAt.isInclusive() ? "a:" : "b:");
328+
builder.append(endAt.positionString());
327329
}
328330

329331
memoizedCannonicalId = builder.toString();

‎firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
176176
if (fieldIndex == null) return null;
177177

178178
Bound lowerBound = target.getLowerBound(fieldIndex);
179-
String lowerBoundOp = lowerBound.isBefore() ? ">=" : ">"; // `startAt()` versus `startAfter()`
179+
String lowerBoundOp = lowerBound.isInclusive() ? ">=" : ">";
180180

181181
@Nullable Bound upperBound = target.getUpperBound(fieldIndex);
182182

@@ -202,7 +202,7 @@ public Set<DocumentKey> getDocumentsMatchingTarget(Target target) {
202202
lowerBoundValues.size() == upperBoundValues.size(),
203203
"Expected upper and lower bound size to match");
204204

205-
String upperBoundOp = upperBound.isBefore() ? "<" : "<="; // `endBefore()` versus `endAt()`
205+
String upperBoundOp = upperBound.isInclusive() ? "<=" : "<";
206206

207207
// TODO(indexing): To avoid reading the same documents multiple times, we should ideally only
208208
// send one query that combines all clauses.

‎firebase-firestore/src/main/java/com/google/firebase/firestore/remote/RemoteSerializer.java

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -555,11 +555,17 @@ public QueryTarget encodeQueryTarget(com.google.firebase.firestore.core.Target t
555555
}
556556

557557
if (target.getStartAt() != null) {
558-
structuredQueryBuilder.setStartAt(encodeBound(target.getStartAt()));
558+
Cursor.Builder cursor = Cursor.newBuilder();
559+
cursor.addAllValues(target.getStartAt().getPosition());
560+
cursor.setBefore(target.getStartAt().isInclusive());
561+
structuredQueryBuilder.setStartAt(cursor);
559562
}
560563

561564
if (target.getEndAt() != null) {
562-
structuredQueryBuilder.setEndAt(encodeBound(target.getEndAt()));
565+
Cursor.Builder cursor = Cursor.newBuilder();
566+
cursor.addAllValues(target.getEndAt().getPosition());
567+
cursor.setBefore(!target.getEndAt().isInclusive());
568+
structuredQueryBuilder.setEndAt(cursor);
563569
}
564570

565571
builder.setStructuredQuery(structuredQueryBuilder);
@@ -609,12 +615,12 @@ public com.google.firebase.firestore.core.Target decodeQueryTarget(
609615

610616
Bound startAt = null;
611617
if (query.hasStartAt()) {
612-
startAt = decodeBound(query.getStartAt());
618+
startAt = new Bound(query.getStartAt().getValuesList(), query.getStartAt().getBefore());
613619
}
614620

615621
Bound endAt = null;
616622
if (query.hasEndAt()) {
617-
endAt = decodeBound(query.getEndAt());
623+
endAt = new Bound(query.getEndAt().getValuesList(), !query.getEndAt().getBefore());
618624
}
619625

620626
return new com.google.firebase.firestore.core.Target(
@@ -817,19 +823,6 @@ private OrderBy decodeOrderBy(Order proto) {
817823
return OrderBy.getInstance(direction, fieldPath);
818824
}
819825

820-
// Bounds
821-
822-
private Cursor encodeBound(Bound bound) {
823-
Cursor.Builder builder = Cursor.newBuilder();
824-
builder.addAllValues(bound.getPosition());
825-
builder.setBefore(bound.isBefore());
826-
return builder.build();
827-
}
828-
829-
private Bound decodeBound(Cursor proto) {
830-
return new Bound(proto.getValuesList(), proto.getBefore());
831-
}
832-
833826
// Watch changes
834827

835828
public WatchChange decodeWatchChange(ListenResponse protoChange) {

0 commit comments

Comments
 (0)