get() reimplementation #3887

maneesht · 2022-07-11T16:18:52Z

Fixes the issue first seen on iOS: firebase/firebase-ios-sdk#8286. Essentially, the wrong path in the SyncTree was being updated. Before, regardless of what type of query was being sent, we were always updating the default query path. Now, we add the query to the SyncTree (along with Persistence) by adding an event registration (just like for a listener) and then remove the event registration once the data has come back.

github-actions · 2022-07-11T16:23:44Z

Unit Test Results

401 tests - 689 399 ✔️ - 675 27s ⏱️ -26s
  27 suites -   51     2 💤 -   14
  27 files -   51     0 ❌ ±    0

Results for commit 2d7b15f. ± Comparison against base commit 310ce36.

♻️ This comment has been updated with latest results.

google-oss-bot · 2022-07-11T16:24:18Z

Coverage Report ¹

Affected Products

`firebase-database`

Overall coverage changed from 50.18% (d4fd4a8) to 50.15% (c559af1) by -0.03%.

Filename	Base (`d4fd4a8`)	Merge (c559af1)	Diff
PersistentConnectionImpl.java	17.04%	17.07%	+0.02%
Repo.java	10.45%	10.31%	-0.14%
SyncPoint.java	88.79%	88.89%	+0.10%
SyncTree.java	57.24%	57.14%	-0.10%
WriteTree.java	76.67%	77.22%	+0.56%

Test Logs

Notes

Commit (c559af1) is created by Prow via merging PR base commit (d4fd4a8) and head commit (2d7b15f).
Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

https://storage.googleapis.com/firebase-sdk-metric-reports/mnhsSmJHlu.html

google-oss-bot · 2022-07-11T16:30:20Z

Size Report ¹

Affected Products

firebase-database
Type Base (d4fd4a8) Merge (c559af1) Diff
aar 488 kB 489 kB +1.24 kB (+0.3%)
apk (release) 1.15 MB 1.15 MB +328 B (+0.0%)

Test Logs

Notes

Commit (c559af1) is created by Prow via merging PR base commit (d4fd4a8) and head commit (2d7b15f).

https://storage.googleapis.com/firebase-sdk-metric-reports/dI5Ak7cGRY.html

…ree temporarily

jmwski

Reviewed the tests, will review the rest later.

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

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

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

jmwski

This is great, those skipDedup + skipListenerSetup parameters make this much easier to think about! I'm ready to approve once these comments are addressed.

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

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

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

firebase-database/src/main/java/com/google/firebase/database/core/view/View.java

jmwski

LGTM, did you run ./gradlew :firebase-database:googleJavaFormat?

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

jmwski · 2022-07-29T16:25:35Z

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

+      DatabaseReference parentReadNode = readDb.getReference().child(parentNodeKey);
+      IntegrationTestHelpers.waitFor(semaphore);
+      DataSnapshot snapshot = await(parentReadNode.get());
+      assertEquals(snapshot.child(childNodeKey).getValue(), val);


Just a note, no need to make any changes.

I think in the future for assertions like this we should assert the contents of snapshot as opposed to indexing into it with .child():

assertEquals(MapBuilder.put(childNodeKey, val).build(), snapshot.getValue());

I think in general it's easier to think about "values of references" as on L4765 below. It's also more correct because it tests that unexpected keys don't exist at snapshot.getValue().

jmwski · 2022-07-29T16:29:07Z

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

+    FirebaseApp readApp =
+        appForDatabaseUrl(IntegrationTestValues.getDatabaseUrl(), UUID.randomUUID().toString());
+    FirebaseApp writeApp =
+        appForDatabaseUrl(IntegrationTestValues.getDatabaseUrl(), UUID.randomUUID().toString());
+    FirebaseDatabase readDb = FirebaseDatabase.getInstance(readApp);
+    FirebaseDatabase writeDb = FirebaseDatabase.getInstance(writeApp);
+    String parentKey = UUID.randomUUID().toString();
+    DatabaseReference writeRef = writeDb.getReference().child(parentKey);
+    DatabaseReference child1Ref = readDb.getReference().child(parentKey).child("child1");


Feel free to do this in a separate PR, we could definitely abstract some of this setup into a helper, potentially even a some sort of TestFixture class.

maneesht · 2022-07-29T19:54:11Z

LGTM, did you run ./gradlew :firebase-database:googleJavaFormat?

Yes. Tests fail otherwise

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

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

Fixed `get()` issue where wrong cache paths were being updated in the SyncTree. Co-authored-by: Jan Wyszynski <[email protected]>

jmwski · 2022-11-15T15:25:09Z

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

+        new MapBuilder().put("child1", "test1").put("child2", "test2").build();
+    try {
+      await(writeRef.setValue(expected));
+      ReadFuture.untilEquals(readRef, expected).timedGet();


Was this test supposed to install a listener with a timeout and validate that we don't get two events in some window?

I believe so

jmwski · 2022-11-16T16:43:10Z

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

+               * that location, listen would be called twice on the same query. skipDedup allows us
+               * to skip this deduping process altogether.
+               */
+              if (skipDedup) {


I think we are leaking the tag created inside keepSynced(true) in Repo.getValue because we never hit L790.

Updated getValue so that it doesn't update the SyncTree

3e085d4

google-oss-bot added do-not-merge/work-in-progress size/L labels Jul 11, 2022

maneesht added 5 commits July 12, 2022 13:18

Fixed formatting issues

34e251d

WIP

dcdff66

Merge remote-tracking branch 'origin/master' into mtewani/fix-get-cache

7b6f9f4

Merge remote-tracking branch 'origin/master' into mtewani/fix-get-cache

bec3e87

Merge remote-tracking branch 'origin/master' into mtewani/fix-get-cache

0276e1c

maneesht requested a review from jmwski July 19, 2022 15:40

maneesht added 5 commits July 19, 2022 08:41

Used addEventRegistration and removeEventRegistration to add to the t…

606b9c1

…ree temporarily

Fixed test and ran formatter

299c2df

Added comments

02af45d

Fixed formatting

1babdc2

Added test annotation

989f124

jmwski reviewed Jul 19, 2022

View reviewed changes

jmwski reviewed Jul 20, 2022

View reviewed changes

maneesht and others added 10 commits July 25, 2022 09:46

WIP

8002947

get test suggest

b95be2c

WIP

6f5861a

Removed extra event propagation

3f3813a

Included more tests

13e2dc5

Added missing link

481acd7

Fixed formatting

69b4bef

Fixed formatting

cc6e3a1

Reverted retryrule to 3

cc70e55

Resolved all TODOs

1d58f9c

maneesht marked this pull request as ready for review July 27, 2022 19:52

Removed comment

94c91fc

maneesht assigned jmwski Jul 28, 2022

jmwski reviewed Jul 28, 2022

View reviewed changes

jmwski assigned maneesht and unassigned jmwski Jul 28, 2022

maneesht added 3 commits July 28, 2022 18:27

Addressed comments

844c8cc

Removed usage of 34L

3bdf63a

Removed question

a590590

maneesht assigned jmwski and unassigned maneesht Jul 29, 2022

jmwski approved these changes Jul 29, 2022

View reviewed changes

jmwski assigned maneesht and unassigned jmwski Jul 29, 2022

Addressed comments

67bf0ce

jmwski assigned jmwski and unassigned maneesht Jul 29, 2022

Fixed formatting

15bbeda

jmwski reviewed Jul 29, 2022

View reviewed changes

firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java Outdated Show resolved Hide resolved

jmwski reviewed Jul 29, 2022

View reviewed changes

firebase-database/src/androidTest/java/com/google/firebase/database/IntegrationTestHelpers.java Outdated Show resolved Hide resolved

jmwski reviewed Jul 29, 2022

View reviewed changes

firebase-database/src/androidTest/java/com/google/firebase/database/QueryTest.java Outdated Show resolved Hide resolved

Addressed comments

2d7b15f

jmwski assigned maneesht and unassigned jmwski Jul 29, 2022

maneesht merged commit 884e537 into master Jul 29, 2022

maneesht deleted the mtewani/fix-get-cache branch July 29, 2022 21:30

lfkellogg pushed a commit that referenced this pull request Aug 5, 2022

Fixed get() to make sure it updates the correct paths (#3887)

55a96e7

Fixed `get()` issue where wrong cache paths were being updated in the SyncTree. Co-authored-by: Jan Wyszynski <[email protected]>

firebase locked and limited conversation to collaborators Aug 29, 2022

jmwski reviewed Nov 15, 2022

View reviewed changes

jmwski reviewed Nov 16, 2022

View reviewed changes

get() reimplementation #3887

get() reimplementation #3887

Uh oh!

Conversation

maneesht commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

google-oss-bot commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

firebase-database

Test Logs

Notes

Uh oh!

google-oss-bot commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size Report 1

Affected Products

firebase-database

Test Logs

Notes

Uh oh!

jmwski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmwski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmwski left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmwski Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

jmwski Jul 29, 2022

Choose a reason for hiding this comment

Uh oh!

maneesht commented Jul 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmwski Nov 15, 2022

Choose a reason for hiding this comment

maneesht commented Jul 11, 2022 •

edited

Loading

github-actions bot commented Jul 11, 2022 •

edited

Loading

google-oss-bot commented Jul 11, 2022 •

edited

Loading

Coverage Report ¹

`firebase-database`

google-oss-bot commented Jul 11, 2022 •

edited

Loading

Size Report ¹

`firebase-database`