fix(firestore, android): firestore instance cache key mismatch#9097
fix(firestore, android): firestore instance cache key mismatch#9097russellwheatley wants to merge 4 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a cache key mismatch issue on Android that prevented the Firestore instance cache from being correctly utilized. By aligning the cache storage key with the lookup key, the fix ensures that subsequent operations on an initialized Firestore instance do not trigger redundant settings applications, which previously caused runtime exceptions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an Android Firestore instance cache key mismatch (issue 8981) by updating the cache write key to use firestoreKey instead of appName, and adds a corresponding E2E regression test. However, because instanceCache is a WeakHashMap, using dynamically created String keys will cause the keys to be garbage collected almost immediately, evicting the cache entries. It is recommended to change instanceCache to a standard ConcurrentHashMap since the values are already wrapped in WeakReference.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9097 +/- ##
============================================
+ Coverage 58.61% 58.67% +0.06%
- Complexity 1616 1620 +4
============================================
Files 503 503
Lines 39178 39180 +2
Branches 5807 5743 -64
============================================
+ Hits 22962 22983 +21
+ Misses 14811 14739 -72
- Partials 1405 1458 +53
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Bugbot is not enabled for this team, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
mikehardy
left a comment
mikehardy
left a comment
There was a problem hiding this comment.
the fix looks good - one question on the test which cuts to the problem I had with it - can you trigger an expected failure in e2e bet altering settings in an already started database?
+1 because the fix is the obvious fix, a negative probe is just a hope
| fail( | ||
| `Regression in issue 8981: switching database on the same app and reapplying settings should not throw, but got: ${e}`, | ||
| ); |
There was a problem hiding this comment.
so here's the question - if you specifically do the pathological thing - now taking an already started firestore database and try to change it's settings in a way that should throw, does it throw?
2 participants
Description
UniversalFirebaseFirestoreCommon.getFirestoreForApp() on Android read the instance cache using the key appName:databaseId but wrote it back using only appName, so the cache lookup could never hit — for the default database or any custom database.
Because of this, every native Firestore call re-derived and reapplied settings via setFirestoreSettings() against the same underlying FirebaseFirestore instance. Once that instance had already been used, reapplying settings that differ from what's currently active throws:
IllegalStateException: FirebaseFirestore has already been started and its settings can no longer be changed.
This surfaced in production (Crashlytics) when firestore.settings(...) was called during app startup while document listeners were mounting concurrently. Fix: write the cache entry using the same firestoreKey that's used to read it, matching the (already-correct) iOS implementation.
Related issues
fixes #8981
Release Summary
Checklist
AndroidiOSOther(macOS, web)e2etests added or updated inpackages/\*\*/e2ejesttests added or updated inpackages/\*\*/__tests__Test Plan
Think
react-native-firebaseis great? Please consider supporting the project with any of the below:React Native FirebaseandInvertaseon Twitter