Skip to content

Commit 0de237d

Browse files
jmwskischmidt-sebastian
jmwski
and
schmidt-sebastian
authored
Fix startAfter/endBefore for orderByKeys queries (#2376)
* Fix startAfter/endBefore for orderByKeys queries * Undo generifying diff * address feedback * Update firebase-database/src/main/java/com/google/firebase/database/Query.java Co-authored-by: Sebastian Schmidt <[email protected]> * Update firebase-database/src/main/java/com/google/firebase/database/Query.java Co-authored-by: Sebastian Schmidt <[email protected]> * changelog * patch version * fix format * WIP: Fix get() * More code duplication * small fix + test cleanup * getView dedup * unfo run config changes * address review feedback * drop changelog * format * Have get check the pending writes cache Co-authored-by: Sebastian Schmidt <[email protected]>
1 parent 4596dd1 commit 0de237d

File tree

5 files changed

+238
-27
lines changed

5 files changed

+238
-27
lines changed

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

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

1515
package com.google.firebase.database;
1616

17+
import static org.junit.Assert.assertArrayEquals;
1718
import static org.junit.Assert.assertEquals;
1819
import static org.junit.Assert.assertFalse;
1920
import static org.junit.Assert.assertNotNull;
@@ -582,6 +583,111 @@ public void setVariousLimitsWithStartAtName() throws DatabaseException, Interrup
582583
expectations.waitForEvents();
583584
}
584585

586+
@Test
587+
public void testStartAfterWithOrderByKey()
588+
throws DatabaseException, InterruptedException, ExecutionException, TimeoutException,
589+
TestFailure {
590+
DatabaseReference ref = IntegrationTestHelpers.getRandomNode();
591+
DatabaseReference childOne = ref.push();
592+
DatabaseReference childTwo = ref.push();
593+
Tasks.await(childOne.setValue(1L));
594+
Tasks.await(childTwo.setValue(2L));
595+
596+
DataSnapshot snapshot = Tasks.await(ref.orderByKey().startAfter(childOne.getKey()).get());
597+
598+
Map<String, Long> values = (Map<String, Long>) snapshot.getValue();
599+
600+
assertNotNull(values);
601+
assertArrayEquals(values.keySet().toArray(), new String[] {childTwo.getKey()});
602+
assertArrayEquals(values.values().toArray(), new Long[] {values.get(childTwo.getKey())});
603+
}
604+
605+
@Test
606+
public void testEndBeforeWithOrderByKey()
607+
throws DatabaseException, InterruptedException, ExecutionException {
608+
DatabaseReference ref = IntegrationTestHelpers.getRandomNode();
609+
DatabaseReference childOne = ref.push();
610+
DatabaseReference childTwo = ref.push();
611+
Tasks.await(childOne.setValue(1L));
612+
Tasks.await(childTwo.setValue(2L));
613+
614+
DataSnapshot snapshot = Tasks.await(ref.orderByKey().endBefore(childTwo.getKey()).get());
615+
Map<String, Long> values = (Map<String, Long>) snapshot.getValue();
616+
617+
assertNotNull(values);
618+
assertArrayEquals(values.keySet().toArray(), new String[] {childOne.getKey()});
619+
assertArrayEquals(values.values().toArray(), new Long[] {values.get(childOne.getKey())});
620+
}
621+
622+
// This test checks that range filters are applied to in-memory data if our active listeners
623+
// have already retrieved the data we need to satisfy the get().
624+
@Test
625+
public void testEndBeforeWithOrderByKeyOverlappingListener()
626+
throws DatabaseException, InterruptedException, ExecutionException {
627+
DatabaseReference ref = IntegrationTestHelpers.getRandomNode();
628+
DatabaseReference childOne = ref.push();
629+
DatabaseReference childTwo = ref.push();
630+
Tasks.await(childOne.setValue(1L));
631+
Tasks.await(childTwo.setValue(2L));
632+
633+
Semaphore semaphore = new Semaphore(0);
634+
ValueEventListener listener =
635+
new ValueEventListener() {
636+
@Override
637+
public void onDataChange(@NonNull DataSnapshot snapshot) {
638+
semaphore.release();
639+
}
640+
641+
@Override
642+
public void onCancelled(@NonNull DatabaseError error) {}
643+
};
644+
645+
ref.addValueEventListener(listener);
646+
647+
IntegrationTestHelpers.waitFor(semaphore);
648+
649+
DataSnapshot snapshot = Tasks.await(ref.orderByKey().endBefore(childTwo.getKey()).get());
650+
Map<String, Long> values = (Map<String, Long>) snapshot.getValue();
651+
652+
assertNotNull(values);
653+
assertArrayEquals(values.keySet().toArray(), new String[] {childOne.getKey()});
654+
assertArrayEquals(values.values().toArray(), new Long[] {values.get(childOne.getKey())});
655+
ref.removeEventListener(listener);
656+
}
657+
658+
@Test
659+
public void testStartAfterWithOrderByKeyOverlappingListener()
660+
throws DatabaseException, InterruptedException, ExecutionException {
661+
DatabaseReference ref = IntegrationTestHelpers.getRandomNode();
662+
DatabaseReference childOne = ref.push();
663+
DatabaseReference childTwo = ref.push();
664+
Tasks.await(childOne.setValue(1L));
665+
Tasks.await(childTwo.setValue(2L));
666+
667+
Semaphore semaphore = new Semaphore(0);
668+
ValueEventListener listener =
669+
new ValueEventListener() {
670+
@Override
671+
public void onDataChange(@NonNull DataSnapshot snapshot) {
672+
semaphore.release();
673+
}
674+
675+
@Override
676+
public void onCancelled(@NonNull DatabaseError error) {}
677+
};
678+
ref.addValueEventListener(listener);
679+
680+
IntegrationTestHelpers.waitFor(semaphore);
681+
682+
DataSnapshot snapshot = Tasks.await(ref.orderByKey().startAfter(childOne.getKey()).get());
683+
Map<String, Long> values = (Map<String, Long>) snapshot.getValue();
684+
685+
assertNotNull(values);
686+
assertArrayEquals(values.keySet().toArray(), new String[] {childTwo.getKey()});
687+
assertArrayEquals(values.values().toArray(), new Long[] {values.get(childTwo.getKey())});
688+
ref.removeEventListener(listener);
689+
}
690+
585691
@Test
586692
public void setVariousLimitsWithStartAfterName() throws DatabaseException, InterruptedException {
587693
DatabaseReference ref = IntegrationTestHelpers.getRandomNode();
@@ -4527,6 +4633,33 @@ public void onCancelled(@NonNull DatabaseError error) {}
45274633
}
45284634
}
45294635

4636+
@Test
4637+
public void testGetWithPendingWrites() throws ExecutionException, InterruptedException {
4638+
DatabaseReference node = IntegrationTestHelpers.getRandomNode();
4639+
node.getDatabase().goOffline();
4640+
try {
4641+
Map<String, Object> expected = new MapBuilder().put("foo", "bar").build();
4642+
node.setValue(expected);
4643+
DataSnapshot snapshot = Tasks.await(node.get());
4644+
assertEquals(snapshot.getValue(), expected);
4645+
} finally {
4646+
node.getDatabase().goOnline();
4647+
}
4648+
}
4649+
4650+
@Test
4651+
public void testGetChildOfPendingWrites() throws ExecutionException, InterruptedException {
4652+
DatabaseReference node = IntegrationTestHelpers.getRandomNode();
4653+
node.getDatabase().goOffline();
4654+
try {
4655+
node.setValue(new MapBuilder().put("foo", "bar").build());
4656+
DataSnapshot snapshot = Tasks.await(node.child("foo").get());
4657+
assertEquals(snapshot.getValue(), "bar");
4658+
} finally {
4659+
node.getDatabase().goOnline();
4660+
}
4661+
}
4662+
45304663
@Test
45314664
public void testGetSendsServerProbesPersistenceCacheWhenOfflineWithNoListener()
45324665
throws DatabaseException, InterruptedException, ExecutionException, TestFailure,

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

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -83,9 +83,10 @@ public class Query {
8383
private void validateQueryEndpoints(QueryParams params) {
8484
if (params.getIndex().equals(KeyIndex.getInstance())) {
8585
String message =
86-
"You must use startAt(String value), endAt(String value) or "
87-
+ "equalTo(String value) in combination with orderByKey(). Other type of values or "
88-
+ "using the version with 2 parameters is not supported";
86+
"You must use startAt(String value), startAfter(String value), endAt(String value), "
87+
+ "endBefore(String value) or equalTo(String value) in combination with "
88+
+ "orderByKey(). Other type of values or using the version with 2 parameters is "
89+
+ "not supported";
8990
if (params.hasStart()) {
9091
Node startNode = params.getIndexStartValue();
9192
ChildKey startName = params.getIndexStartName();
@@ -105,8 +106,8 @@ private void validateQueryEndpoints(QueryParams params) {
105106
if ((params.hasStart() && !PriorityUtilities.isValidPriority(params.getIndexStartValue()))
106107
|| (params.hasEnd() && !PriorityUtilities.isValidPriority(params.getIndexEndValue()))) {
107108
throw new IllegalArgumentException(
108-
"When using orderByPriority(), values provided to startAt(), "
109-
+ "endAt(), or equalTo() must be valid priorities.");
109+
"When using orderByPriority(), values provided to startAt(), startAfter(), "
110+
+ "endAt(), endBefore(), or equalTo() must be valid priorities.");
110111
}
111112
}
112113
}
@@ -115,18 +116,18 @@ private void validateQueryEndpoints(QueryParams params) {
115116
private void validateLimit(QueryParams params) {
116117
if (params.hasStart() && params.hasEnd() && params.hasLimit() && !params.hasAnchoredLimit()) {
117118
throw new IllegalArgumentException(
118-
"Can't combine startAt(), endAt() and limit(). "
119+
"Can't combine startAt(), startAfter(), endAt(), endBefore(), and limit(). "
119120
+ "Use limitToFirst() or limitToLast() instead");
120121
}
121122
}
122123

123124
/** This method validates that the equalTo call can be made */
124125
private void validateEqualToCall() {
125126
if (params.hasStart()) {
126-
throw new IllegalArgumentException("Can't call equalTo() and startAt() combined");
127+
throw new IllegalArgumentException("Cannot combine equalTo() with startAt() or startAfter()");
127128
}
128129
if (params.hasEnd()) {
129-
throw new IllegalArgumentException("Can't call equalTo() and endAt() combined");
130+
throw new IllegalArgumentException("Cannot combine equalTo() with endAt() or endBefore()");
130131
}
131132
}
132133

@@ -289,6 +290,9 @@ public void run() {
289290
*/
290291
@NonNull
291292
public Query startAfter(@Nullable String value) {
293+
if (value != null && params.getIndex().equals(KeyIndex.getInstance())) {
294+
return startAt(PushIdGenerator.successor(value));
295+
}
292296
return startAt(value, ChildKey.getMaxName().asString());
293297
}
294298

@@ -330,6 +334,9 @@ public Query startAfter(boolean value) {
330334
*/
331335
@NonNull
332336
public Query startAfter(@Nullable String value, @Nullable String key) {
337+
if (value != null && params.getIndex().equals(KeyIndex.getInstance())) {
338+
value = PushIdGenerator.successor(value);
339+
}
333340
Node node =
334341
value != null ? new StringNode(value, PriorityUtilities.NullPriority()) : EmptyNode.Empty();
335342
return startAfter(node, key);
@@ -488,6 +495,9 @@ private Query startAt(Node node, String key) {
488495
*/
489496
@NonNull
490497
public Query endBefore(@Nullable String value) {
498+
if (value != null && params.getIndex().equals(KeyIndex.getInstance())) {
499+
return endAt(PushIdGenerator.predecessor(value));
500+
}
491501
return endAt(value, ChildKey.getMinName().asString());
492502
}
493503

@@ -529,6 +539,9 @@ public Query endBefore(boolean value) {
529539
*/
530540
@NonNull
531541
public Query endBefore(@Nullable String value, @Nullable String key) {
542+
if (value != null && params.getIndex().equals(KeyIndex.getInstance())) {
543+
value = PushIdGenerator.predecessor(value);
544+
}
532545
Node node =
533546
value != null ? new StringNode(value, PriorityUtilities.NullPriority()) : EmptyNode.Empty();
534547
return endBefore(node, key);

‎firebase-database/src/main/java/com/google/firebase/database/core/Repo.java

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -496,12 +496,11 @@ public Task<DataSnapshot> getValue(Query query) {
496496
public void run() {
497497
// Always check active-listener in-memory caches first. These are always at least as
498498
// up to date as the persistence cache.
499-
Node cached =
500-
serverSyncTree.calcCompleteEventCacheFromRoot(query.getPath(), new ArrayList<>());
501-
if (!cached.isEmpty()) {
499+
Node serverValue = serverSyncTree.getServerValue(query.getSpec());
500+
if (serverValue != null) {
502501
source.setResult(
503502
InternalHelpers.createDataSnapshot(
504-
query.getRef(), IndexedNode.from(cached, query.getSpec().getIndex())));
503+
query.getRef(), IndexedNode.from(serverValue)));
505504
return;
506505
}
507506
serverSyncTree.setQueryActive(query.getSpec());

‎firebase-database/src/main/java/com/google/firebase/database/core/SyncPoint.java

Lines changed: 27 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import com.google.firebase.database.core.view.View;
3232
import com.google.firebase.database.core.view.ViewCache;
3333
import com.google.firebase.database.snapshot.ChildKey;
34+
import com.google.firebase.database.snapshot.EmptyNode;
3435
import com.google.firebase.database.snapshot.IndexedNode;
3536
import com.google.firebase.database.snapshot.NamedNode;
3637
import com.google.firebase.database.snapshot.Node;
@@ -113,11 +114,7 @@ public List<DataEvent> applyOperation(
113114
}
114115

115116
/** Add an event callback for the specified query. */
116-
public List<DataEvent> addEventRegistration(
117-
@NotNull EventRegistration eventRegistration,
118-
WriteTreeRef writesCache,
119-
CacheNode serverCache) {
120-
QuerySpec query = eventRegistration.getQuerySpec();
117+
public View getView(QuerySpec query, WriteTreeRef writesCache, CacheNode serverCache) {
121118
View view = this.views.get(query.getParams());
122119
if (view == null) {
123120
// TODO: make writesCache take flag for complete server node
@@ -128,24 +125,39 @@ public List<DataEvent> addEventRegistration(
128125
if (eventCache != null) {
129126
eventCacheComplete = true;
130127
} else {
131-
eventCache = writesCache.calcCompleteEventChildren(serverCache.getNode());
128+
eventCache =
129+
writesCache.calcCompleteEventChildren(
130+
serverCache.getNode() != null ? serverCache.getNode() : EmptyNode.Empty());
132131
eventCacheComplete = false;
133132
}
134133
IndexedNode indexed = IndexedNode.from(eventCache, query.getIndex());
135134
ViewCache viewCache =
136135
new ViewCache(new CacheNode(indexed, eventCacheComplete, false), serverCache);
137-
view = new View(query, viewCache);
138-
// If this is a non-default query we need to tell persistence our current view of the data
139-
if (!query.loadsAllData()) {
140-
Set<ChildKey> allChildren = new HashSet<ChildKey>();
141-
for (NamedNode node : view.getEventCache()) {
142-
allChildren.add(node.getName());
143-
}
144-
this.persistenceManager.setTrackedQueryKeys(query, allChildren);
136+
return new View(query, viewCache);
137+
}
138+
139+
return view;
140+
}
141+
142+
/** Add an event callback for the specified query. */
143+
public List<DataEvent> addEventRegistration(
144+
@NotNull EventRegistration eventRegistration,
145+
WriteTreeRef writesCache,
146+
CacheNode serverCache) {
147+
QuerySpec query = eventRegistration.getQuerySpec();
148+
View view = getView(query, writesCache, serverCache);
149+
// If this is a non-default query we need to tell persistence our current view of the data
150+
if (!query.loadsAllData()) {
151+
Set<ChildKey> allChildren = new HashSet<ChildKey>();
152+
for (NamedNode node : view.getEventCache()) {
153+
allChildren.add(node.getName());
145154
}
155+
this.persistenceManager.setTrackedQueryKeys(query, allChildren);
156+
}
157+
if (!this.views.containsKey(query.getParams())) {
146158
this.views.put(query.getParams(), view);
147159
}
148-
160+
this.views.put(query.getParams(), view);
149161
// This is guaranteed to exist now, we just created anything that was missing
150162
view.addEventRegistration(eventRegistration);
151163
return view.getInitialEvents(eventRegistration);

‎firebase-database/src/main/java/com/google/firebase/database/core/SyncTree.java

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -484,6 +484,60 @@ public DataSnapshot persistenceServerCache(Query query) {
484484
query.getRef(), persistenceManager.serverCache(query.getSpec()).getIndexedNode());
485485
}
486486

487+
@Nullable
488+
public Node getServerValue(QuerySpec query) {
489+
return persistenceManager.runInTransaction(
490+
() -> {
491+
Path path = query.getPath();
492+
493+
Node serverCacheNode = null;
494+
boolean foundAncestorDefaultView = false;
495+
// Any covering writes will necessarily be at the root, so really all we need to find is
496+
// the server cache. Consider optimizing this once there's a better understanding of
497+
// what actual behavior will be.
498+
ImmutableTree<SyncPoint> tree = syncPointTree;
499+
Path currentPath = path;
500+
while (!tree.isEmpty()) {
501+
SyncPoint currentSyncPoint = tree.getValue();
502+
if (currentSyncPoint != null) {
503+
serverCacheNode =
504+
serverCacheNode != null
505+
? serverCacheNode
506+
: currentSyncPoint.getCompleteServerCache(currentPath);
507+
foundAncestorDefaultView =
508+
foundAncestorDefaultView || currentSyncPoint.hasCompleteView();
509+
}
510+
ChildKey front =
511+
currentPath.isEmpty() ? ChildKey.fromString("") : currentPath.getFront();
512+
tree = tree.getChild(front);
513+
currentPath = currentPath.popFront();
514+
}
515+
516+
SyncPoint syncPoint = syncPointTree.get(path);
517+
if (syncPoint == null) {
518+
syncPoint = new SyncPoint(persistenceManager);
519+
syncPointTree = syncPointTree.set(path, syncPoint);
520+
} else {
521+
serverCacheNode =
522+
serverCacheNode != null
523+
? serverCacheNode
524+
: syncPoint.getCompleteServerCache(Path.getEmptyPath());
525+
}
526+
527+
CacheNode serverCache =
528+
new CacheNode(
529+
IndexedNode.from(
530+
serverCacheNode != null ? serverCacheNode : EmptyNode.Empty(),
531+
query.getIndex()),
532+
serverCacheNode != null,
533+
false);
534+
535+
WriteTreeRef writesCache = pendingWriteTree.childWrites(path);
536+
View view = syncPoint.getView(query, writesCache, serverCache);
537+
return view.getCompleteNode();
538+
});
539+
}
540+
487541
/** Add an event callback for the specified query. */
488542
public List<? extends Event> addEventRegistration(
489543
@NotNull final EventRegistration eventRegistration) {

0 commit comments

Comments
 (0)