feat(database): get() uses the native one-shot read on Android and web

[nickcernera]

Description

Modular get() was once('value'): a listen → data → unlisten exchange of about three server responses and two round trips. On Android and web it now uses the SDKs' single request/response read (Query.get(), web get()). In our measurement that is about 129 B less billed per read and one round trip fewer (numbers in #9344).

• New native method: get(app, dbURL, path, modifiers) on NativeRNFBTurboDatabaseQuery.
  • Android: Query.get(), with the snapshot serialised through Tasks.call exactly as once does. It uses once instead when disk persistence is on (Query.get() then resolves from disk after 3 s, even online), when RTDB debug logging is on (a get in flight at disconnect is then never resent), and for a location where the app keeps anything synced. A keepSynced call made while a native get of the location is in flight waits for the get to finish. (On a cache miss, Query.get() turns keepSynced off for the location's default spec. That removes every keepSynced registration there, whatever its query, which cancels the app's keepSynced: Realtime Database Android: keepSynced(true) followed by get() causes AssertionError "listen() called twice for same QuerySpec" firebase/firebase-android-sdk#8433.) The bookkeeping is in RNFBDatabaseKeepSyncedRegistry.
  • Web: get().
  • iOS: the single-event observer, as today. FirebaseDatabase's getData has three open bugs (RTDB: calling get() on a path where a parent path is already subscribed to, will return the entire cached parent data rather than the data for the requested path firebase/firebase-ios-sdk#12168, RTDB: getData never completes if the connection drops after the get is sent firebase/firebase-ios-sdk#16717, RTDB: getData crashes with NSInvalidArgumentException on an error reply without a reason firebase/firebase-ios-sdk#16718), and the comment at RNFBDatabaseQueryHelper get: names them. Switching iOS to getData is a one-line follow-up once they ship.
• DatabaseQuery._get() backs modular get():
  • queries (any orderBy / limit / range modifier) and .info/* paths use once('value');
  • a permission denial passes straight through. On Android the native module maps the denied reply to once's code and message, so a denied read is not asked twice;
  • any other Android or web failure retries as once('value'), so the rejection carries the SDK's error code, as it does today.

once() is unchanged. Callers keep once('value') semantics with one exception. The one-shot read resolves with the server's reply as it was when it was sent, so a local write made to an overlapping path while a get() is in flight is not in its result. (once layered pending writes over the reply; the web SDK's get() behaves like this change.) The comment on _get documents it.

Related issues

Fixes #9344

Release Summary

get() in @react-native-firebase/database now performs a single request/response read on Android and web, which bills fewer responses and saves a round trip. iOS is unchanged until FirebaseDatabase fixes getData.

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
  • Other (macOS, web)
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

Run locally on this branch: yarn codegen (generated specs updated, additions only), yarn codegen:verify, yarn lint (eslint, depcruise, clang-format, google-java-format), yarn tsc:compile, yarn tsc:compile:consumer, and yarn jest packages/database (68 passed). For the keepSynced changes, the Android database e2e (Jet, against the Firebase emulators) and the database JVM unit tests were also run locally. The JVM tests were checked against builds with each part of the tracking removed.

• __tests__/get.test.ts, per platform:
  • a plain location uses the native get;
  • Android and web pass a permission denial through, retry any other failure as once, and surface once's error;
  • iOS rejects with the native (observer) error without a retry;
  • queries and .info/* paths skip the native get.
• __tests__/nativeModuleContract.test.ts: get is added to the query host (23 spec methods).
• e2e/query/get.e2e.js, new cases:
  • a child read while its parent is listened to returns only the child;
  • a query returns the same children as once;
  • .info/serverTimeOffset returns a number;
  • Android, with keepSynced: get() of a location that is kept synced, of one whose keepSynced is turned on while the read is in flight, and of one with a query there kept synced.
• RNFBDatabaseKeepSyncedRegistryTest (JVM, tests:android:unit): a location with any query kept synced reads with once, while other locations, and a location after keepSynced(false), read natively; keepSynced calls made during native gets wait for the location's last get and run in call order.
• Backport: the same change, backported to 23.8.6 as a patch-package patch, is in device QA in our app, together with a FirebaseDatabase source patch carrying the three iOS fixes.

🔥

[russellwheatley]

One change on the keepSynced fallback.

[russellwheatley]

keepSyncedUsed is wider than the bug.

Query.get() does cancel an app's own keepSynced, but only for the spec it just read. On success Repo.getValue calls keepSynced(spec, true, skipDedup=true) and then keepSynced(spec, false, skipDedup=true). SyncTree.keepSynced removes that spec from keepSyncedQueries. A get on a different path leaves the registration alone, and keepSynced(false) already drops it.

This flag is set on the first keepSynced(true) and never cleared. After that, every get() in the process uses once(), including other paths, other database URLs, and locations that have since been set back to keepSynced(false).

Could you track the specs that are currently keep-synced and fall back to once() only for those? Clear one when keepSynced(false) is called.

[nickcernera]

Good catch, thanks. It now tracks the locations that are currently keep-synced. keepSynced(true) adds one, keepSynced(false) removes it, and get() only falls back to once() for those.

One thing I found while checking: orderByPriority() on its own builds the same QuerySpec as the plain ref, since the default params already order by priority. So that counts as the plain location too.

[CLAassistant]

All committers have signed the CLA.

[nickcernera]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Nick Cernera seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Signed a minute ago, just not seeing this update yet.

[nickcernera]

@russellwheatley Apologies for the churn here. The E2E failures are from two bugs in the tests I added, not in get(). I pushed a fix (2a3fd35) and I'm running the E2E suites on my fork before asking you to approve CI again. I'll follow up once they're green.

[nickcernera]

Green now. Two bugs in get.e2e.js, both fixed in 2a3fd35:

• "waits for the connection while offline" called this.timeout(20000), which Jet doesn't support (this.timeout is 420000). Removed; Jet's global timeout covers the 5s wait.
• "returns the same children as once() for a query" checked order with Object.keys(snapshot.val()), which doesn't keep key order across the bridge. It now uses snapshot.forEach, like orderByKey.e2e.js.

E2E Android, Other and iOS all pass on my fork against this commit (Android, Other, iOS). The runs here need your approval whenever you get a chance.

[russellwheatley]

Thanks for narrowing this down. One more thing on the test side, see the inline comment.

[russellwheatley]

This tracking is the fix for the keepSynced issue, but nothing exercises it. The Android side is only covered by e2e, so a regression here would go unnoticed. Do you mind adding an e2e case for it?

I think something like this would catch it: keepSynced(true) on a ref, get() that same ref, goOffline(), then read it again. If get() cancelled the app's keepSynced, the data wouldn't be cached any more and the offline read would hang. Might also be worth a case where get() on a different path still takes the native route while another location is kept synced, and one where keepSynced(false) clears it again.

If you think that's not observable from JS, let me know and we can work out another way.

[nickcernera]

Thanks. Added in 0fbb691 and b2a8932, and writing the tests turned up two more gaps, so there's a fix too.

Two gaps. When Query.get() turns keepSynced off for the spec it read, SyncTree treats it as a removal on the default spec. That walks every view at the location, and KeepSyncedEventRegistration.isSameListener matches any keepSynced registration. So it also cancels:

• a keepSynced on a query at that location, e.g. keepSynced(query(ref, limitToLast(1)), true) and then get(ref);
• a keepSynced(true) that lands while a native get of the location is in flight.

The tracking now lives in a small RNFBDatabaseKeepSyncedRegistry, keyed by location for any query. get() uses once() while anything at the location is kept synced. A keepSynced call made during a native get there is replayed, in order, when the get completes.

On the e2e case. I built your case first, but it can't fail in the e2e harness. goOffline() doesn't keep the connection down there: a write made right after it is acked within a second. That's presumably also why .info/connected's offline case is xit. So the read after goOffline() succeeds whether or not keepSynced survived, and I couldn't find another way to see from JS whether keepSynced is still active. Instead:

• RNFBDatabaseKeepSyncedRegistryTest (JVM, runs in tests:android:unit) covers the tracking, including your other two cases: a get on another location still goes native while one is kept synced, and keepSynced(false) clears it. It also covers queries, the in-flight replay and its order, and several gets on one location.
• get.e2e.js keeps three Android cases that run get() next to keepSynced through each path of the wiring: a kept location, keepSynced turned on mid-read, and a query kept synced. They exercise the code end to end but can't tell a cancelled keepSynced from a live one.
• For the same reason, I removed my "waits for the connection while offline" case, since it couldn't fail either.

Checked locally (Android emulator against the Firebase emulators): the database e2e passes (189 passing, 0 failing). Each JVM case below fails against a registry build with that piece removed.

Removed	Fails
once() for a location with anything kept synced	beginNativeGet_locationKeptSynced_returnsFalse, …queryAtLocationKeptSynced…, and 2 more
replaying keepSynced made during a native get	keepSynced_nativeGetInFlight_waitsForItInCallOrder, endNativeGet_waitsForTheLastGetAtLocation
clearing on keepSynced(false)	beginNativeGet_afterKeepSyncedFalse_returnsTrue, …untilEveryQueryAtLocationIsCleared…
keying by location (any kept location forces once())	beginNativeGet_otherLocationKeptSynced_returnsTrue

[codecov]

Codecov Report

❌ Patch coverage is 56.92308% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.82%. Comparing base (dce165d) to head (2a3fd35).
⚠️ Report is 20 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9345      +/-   ##
============================================
- Coverage     69.82%   69.82%   -0.00%     
- Complexity     2131     2137       +6     
============================================
  Files           439      439              
  Lines         25518    25594      +76     
  Branches       4257     4275      +18     
============================================
+ Hits          17816    17869      +53     
- Misses         6359     6375      +16     
- Partials       1343     1350       +7     

Flag	Coverage Δ
android-native	65.80% <47.17%> (-0.14%)	⬇️
e2e-ts-android	54.44% <87.50%> (+0.02%)	⬆️
e2e-ts-ios	53.90% <87.50%> (+0.02%)	⬆️
e2e-ts-macos	49.99% <91.67%> (+0.06%)	⬆️
ios-ruby	100.00% <ø> (ø)
jest	48.89% <75.00%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.