Implement `startAfter` for RTDB queries #2277

jmwski · 2020-12-21T21:07:04Z

startAfter(value, key) means:

If key == undefined or null, startAt(value, MAX_NAME), else, startAt(value, key + 1), where + 1 means either the next integer (if the key is parseable as an int), or the lexicographically next key.

google-oss-bot · 2020-12-21T21:13:37Z

Coverage Report

Affected SDKs

firebase-database

SDK overall coverage changed from 50.37% (4ffbf22) to 50.47% (4219ab20) by +0.10%.

Filename Base (4ffbf22) Head (4219ab20) Diff

PushIdGenerator.java 70.37% 84.13% +13.76%

Query.java 49.70% 46.07% -3.63%

Validation.java 41.67% 42.67% +1.00%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (4219ab20) is created by Prow via merging commits: 4ffbf22 6c017d5.

google-oss-bot · 2020-12-21T21:15:11Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2020-12-21T21:20:01Z

Binary Size Report

Affected SDKs

firebase-database

Type Base (4ffbf22) Head (4219ab20) Diff

aar 490 kB 492 kB +1.32 kB (+0.3%)

apk (release) 1.09 MB 1.09 MB +884 B (+0.1%)

Test Logs

Notes

Head commit (4219ab20) is created by Prow via merging commits: 4ffbf22 6c017d5.

google-oss-bot · 2020-12-22T00:19:57Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2020-12-22T01:07:23Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2020-12-22T20:21:35Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

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

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

...database/src/main/java/com/google/firebase/database/connection/PersistentConnectionImpl.java

jmwski · 2021-01-06T23:42:20Z

@schmidt-sebastian This is ready for another round of review.

google-oss-bot · 2021-01-06T23:42:49Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-01-06T23:48:39Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

schmidt-sebastian · 2021-01-07T04:55:24Z

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

+    ValueExpectationHelper helper = new ValueExpectationHelper();
+    helper.add(
+        ref.startAfter("w").endAt("y"),
+        new MapBuilder().put("d", 4L).put("b", 2L).put("c", 3L).build());


Can you explain why "d" is part of the result set when it has a priority of "w"?

schmidt-sebastian · 2021-01-07T04:59:20Z

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

@@ -1150,6 +1567,29 @@ public void startAtEndAtWithPriorityAndNameWorks2()
    helper.waitForEvents();
  }

+  @Test
+  public void startAfterEndAtWithPriorityAndNameWorks2()


There is also a 2 here as a suffix.

Some of these didn't really add any value and were just blindly copy/pastes. I'm removed them.

schmidt-sebastian · 2021-01-07T04:59:35Z

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

@@ -1174,6 +1614,29 @@ public void startAtEndAtWithPriorityAndNameWorksWithServerData2()
    helper.waitForEvents();
  }

+  @Test
+  public void startAfterEndAtWithPriorityAndNameWorksWithServerData2()


Also a 2 as a suffix.

schmidt-sebastian · 2021-01-07T05:04:10Z

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

@@ -3054,6 +3661,45 @@ public void onCancelled(DatabaseError error) {}
    IntegrationTestHelpers.waitFor(done);
  }

+  @Test
+  public void integerKeysBehaveNumericallyStartAfter1()


Another strange suffix. If these are used to distinguish test cases, then we should come up with more specific names.

schmidt-sebastian · 2021-01-07T05:05:25Z

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

@@ -3142,6 +3788,46 @@ public void onCancelled(DatabaseError error) {}
    IntegrationTestHelpers.waitFor(done);
  }

+  @Test
+  public void integerKeysBehaveNumerically3StartAfter()


Please come up with a more specific test name.

schmidt-sebastian · 2021-01-07T05:11:42Z

...base-database/src/main/java/com/google/firebase/database/core/utilities/PushIdGenerator.java

@@ -57,6 +65,38 @@ public static synchronized String generatePushChildName(long now) {
    return result.toString();
  }

+  public static final String nextAfter(String key) {


Nit: This is usually called a successor.

...base-database/src/main/java/com/google/firebase/database/core/utilities/PushIdGenerator.java

schmidt-sebastian · 2021-01-07T05:22:14Z

...base-database/src/main/java/com/google/firebase/database/core/utilities/PushIdGenerator.java

+  private static final char MAX_PUSH_CHAR = 'z';
+
+  private static final char MIN_PUSH_CHAR = '-';


Nit: Flip assignments for clarity.

You could also refer to PUSH_CHARS and get the values from there (which will be slower and more code, so I am not sure it is actually worth it).

I sort of prefer having separate sentinel values for this since they trigger edge-cases in the successor/predecessor code.

schmidt-sebastian · 2021-01-07T05:24:24Z

firebase-database/src/main/java/com/google/firebase/database/core/utilities/Utilities.java

@@ -273,4 +273,8 @@ public static boolean equals(@Nullable Object left, @Nullable Object right) {
    }
    return left.equals(right);
  }
+
+  public static String next(@Nullable String value) {


This is unused.

schmidt-sebastian · 2021-01-07T05:26:19Z

firebase-database/src/main/java/com/google/firebase/database/snapshot/ChildKey.java

@@ -56,11 +56,11 @@ public boolean isPriorityChildName() {
    return this.equals(PRIORITY_CHILD_KEY);
  }

-  protected boolean isInt() {
+  public boolean isInt() {


These changes don't seem to be needed.

google-oss-bot · 2021-01-08T20:55:36Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-01-08T21:08:57Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

google-oss-bot · 2021-01-08T22:30:39Z

The public api surface has changed for the subproject firebase-database:
error: Added method com.google.firebase.database.Query.startAfter(String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(String,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(boolean,String) [AddedMethod]
error: Added method com.google.firebase.database.Query.startAfter(double,String) [AddedMethod]

Please update the api.txt files for the subprojects being affected by this change by running ./gradlew ${subproject}:generateApiTxtFile. Also perform a major/minor bump accordingly.

jmwski · 2021-01-11T22:01:20Z

@schmidt-sebastian Ready for review.

schmidt-sebastian

LGTM(ish), depending on how we handle the "startAfter(null)" case.

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

...base-database/src/main/java/com/google/firebase/database/core/utilities/PushIdGenerator.java

schmidt-sebastian · 2021-01-12T18:32:06Z

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

+
+    ValueExpectationHelper expectations = new ValueExpectationHelper();
+
+    expectations.add(ref.startAfter(null).limitToFirst(1), null);


Let's talk about this offline. My gut feeling is that this should match all results, since they order past null.

I added an explanation here: https://github.com/firebase/firebase-ios-sdk/pull/7209/files#r556060181.

I know this reads funny, but I think it is the intended behavior because we are indexing on priority.

jmwski · 2021-01-13T15:37:08Z

@schmidt-sebastian The copyright check is failing for an unrelated file. The device-check-changed check runs

./gradlew :firebase-database:firebase-test-labUpload

Which I ran locally on master and it failed in the same way it does here. I'm going to assume these CI checks are not needed.

schmidt-sebastian

LGTM. Please add changelog entry to https://github.com/firebase/firebase-android-sdk/blob/master/firebase-database/CHANGELOG.md

firebase-database/src/test/java/com/google/firebase/database/PushIdGeneratorTest.java

* Implement `endBefore` for RTDB queries * cleanup * Fix bug in prevBefore * Handle endBefore(..., Integer.MIN_VALUE) * respect key ordering * Add more tests, check key order boundary conditions * Generate api text file * Unit tests for predecessor * cleanup

google-oss-bot · 2021-01-14T00:34:21Z

@IanWyszynski: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
device-check-changed	`6c017d5`	link	`/test device-check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

jmwski added 3 commits December 17, 2020 08:56

startAfter temporary commit

8a20103

Merge branch 'master' into jw/start-after

b217ab7

Merge branch 'master' into jw/start-after

6022501

googlebot added the cla: yes Override cla label Dec 21, 2020

google-oss-bot added the size/L label Dec 21, 2020

jmwski assigned schmidt-sebastian Dec 21, 2020

jmwski added the WIP label Dec 21, 2020

Add more tests

e74719d

google-oss-bot added size/XL and removed size/L labels Dec 22, 2020

fix tests

db3dc75

Move lex logic in PushIdGenerator, fix tests

93a9779

schmidt-sebastian reviewed Jan 6, 2021

View reviewed changes

jmwski assigned jmwski and unassigned schmidt-sebastian Jan 6, 2021

Fix push id generation scheme, add test to detect breakage

52af0be

jmwski assigned schmidt-sebastian and unassigned jmwski Jan 6, 2021

test naming comments

bd31fe4

schmidt-sebastian reviewed Jan 7, 2021

View reviewed changes

schmidt-sebastian removed their assignment Jan 7, 2021

Return the proper value from successor

3bffd8b

Check key order boundary conditions

0ad2be2

Add api.txt

a0e6283

jmwski removed the WIP label Jan 11, 2021

schmidt-sebastian reviewed Jan 12, 2021

View reviewed changes

schmidt-sebastian assigned jmwski and unassigned schmidt-sebastian Jan 12, 2021

Add unit tests for successor

889aa88

jmwski assigned schmidt-sebastian and unassigned jmwski Jan 13, 2021

jmwski mentioned this pull request Jan 13, 2021

Implement endBefore for RTDB queries #2302

Merged

schmidt-sebastian approved these changes Jan 13, 2021

View reviewed changes

firebase-database/src/test/java/com/google/firebase/database/PushIdGeneratorTest.java Outdated Show resolved Hide resolved

schmidt-sebastian assigned jmwski and unassigned schmidt-sebastian Jan 13, 2021

jmwski and others added 2 commits January 13, 2021 15:20

cleanup

5ba8ff4

google-oss-bot added size/XXL and removed size/XL labels Jan 14, 2021

jmwski changed the title ~~Implement startAfter for RTDB queries~~ Implement startAfter and endBefore for RTDB queries Jan 14, 2021

jmwski merged commit f8e6a90 into master Jan 14, 2021

andreaowu pushed a commit that referenced this pull request Jan 29, 2021

Implement startAfter and endBefore for RTDB queries (#2277)

b1602fc

firebase locked and limited conversation to collaborators Feb 14, 2021

jmwski changed the title ~~Implement startAfter and endBefore for RTDB queries~~ Implement startAfter for RTDB queries Feb 23, 2021

kaibolay deleted the jw/start-after branch September 14, 2022 17:56

		private static final char MAX_PUSH_CHAR = 'z';

		private static final char MIN_PUSH_CHAR = '-';


		ValueExpectationHelper expectations = new ValueExpectationHelper();

		expectations.add(ref.startAfter(null).limitToFirst(1), null);

Implement startAfter for RTDB queries #2277

Implement startAfter for RTDB queries #2277

Uh oh!

Conversation

jmwski commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

google-oss-bot commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-database

Test Logs

Notes

Uh oh!

google-oss-bot commented Dec 21, 2020

Uh oh!

google-oss-bot commented Dec 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-database

Test Logs

Notes

Uh oh!

google-oss-bot commented Dec 22, 2020

Uh oh!

google-oss-bot commented Dec 22, 2020

Uh oh!

google-oss-bot commented Dec 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmwski commented Jan 6, 2021

Uh oh!

google-oss-bot commented Jan 6, 2021

Uh oh!

google-oss-bot commented Jan 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Jan 8, 2021

Uh oh!

google-oss-bot commented Jan 8, 2021

Uh oh!

google-oss-bot commented Jan 8, 2021

Uh oh!

jmwski commented Jan 11, 2021

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Implement `startAfter` for RTDB queries #2277

Implement `startAfter` for RTDB queries #2277

jmwski commented Dec 21, 2020 •

edited

Loading

google-oss-bot commented Dec 21, 2020 •

edited

Loading

`firebase-database`

google-oss-bot commented Dec 21, 2020 •

edited

Loading

`firebase-database`