[paulb777]

Summary

DatabaseReference.get() and Query.get() no longer crash the app when the
server's response contains data that the SDK can't parse. The returned Task
now completes the same way as a failed request.

Problem

This comes from an expanded audit of how the SDK handles unexpected server data,
which followed
firebase/firebase-ios-sdk#16728.

Repo.getValue parses the server's response to a get() request with
NodeUtilities.NodeFromJSON inside a Task listener that runs on the database
run loop. NodeFromJSON throws DatabaseException for data it can't parse, for
example a node whose .priority is an object, an array, or a boolean (see
PriorityUtilities.parsePriority). The exception wasn't caught, so it reached
DefaultRunLoop.afterExecute, and the AndroidPlatform run loop rethrows it on
the main thread, which crashes the app. The get() Task also never completed.

The same data in a regular push is already handled: Repo.onDataUpdate catches
DatabaseException and logs FIREBASE INTERNAL ERROR.

The trigger needs a backend bug, regression, or protocol change. The bad value
isn't persisted, because the exception is thrown before the response is applied
to the cache. The crashes stop once the server is fixed, but until then every
get() that receives such data crashes the app.

The connection-level safety net in #8764 doesn't cover this case: the get()
result reaches Repo through a Task listener that runs as a separate run loop
task, after Connection.onMessage has returned.

Fix

In Repo.getValue, the call to NodeUtilities.NodeFromJSON is now wrapped in a
try/catch (DatabaseException e). On failure, the SDK logs an error through
operationLogger (a short message and the stack trace, not the payload) and
completes the Task the same way as a failed request: it returns the persisted
value if there is one, otherwise it fails the Task with the
DatabaseException. Parsing already happened before the temporary keep-synced
registration (keepSynced(spec, true, true)), and the new code returns before
it, so nothing is registered, applied to the sync tree, or written to the cache.

Alternatives considered:

• Catching RuntimeException around the whole listener: rejected, because it
  would also hide bugs in the sync tree code that runs after parsing.
  DatabaseException is what NodeFromJSON throws for data it can't parse (it
  also wraps ClassCastException).
• Returning an empty snapshot, or skipping only the children that can't be
  parsed: rejected, because get() would then report data that doesn't match
  the server without any error.
• Changing the run loop exception policy in AndroidPlatform: rejected, because
  it would also hide unrelated bugs.

Behavior change for reviewers: a get() that used to crash now fails with a
DatabaseException (for example "Node contains invalid priority: Must be a
string, double, ServerValue, or null"), or returns the persisted value when the
persistence cache has one. This matches what get() already does when the
request itself fails.

Testing

New Robolectric test class RepoGetValueTest in
firebase-database/src/test/java/com/google/firebase/database/core/. It builds a
Repo from a DatabaseConfig whose newPersistentConnection returns a Mockito
mock of PersistentConnection, so the tests control the response to get()
without a network connection or production test hooks. The run loop records
uncaught exceptions instead of crashing, and every test asserts that none
reached it. Responses are parsed with JsonMapper.parseJsonValue, the same way
as messages from the server.

• getFailsWhenPriorityIsAnObject: {"a": 1, ".priority": {"x": 1}}
• getFailsWhenPriorityIsABoolean: {".priority": true, ".value": 1}
• getFailsWhenListElementHasInvalidPriority:
  ["a", {".priority": ["x"], ".value": "b"}]
• queryGetFailsWhenChildHasInvalidPriority: Query.get() with
  orderByKey().limitToFirst(1). It also checks that no keep-synced
  registration is left behind.
• getReturnsPersistedValueWhenResponseCannotBeParsed: with a value in the
  persistence cache, get() returns that value.
• getReturnsServerValue and queryGetReturnsServerValue: valid responses,
  including a valid priority, still return the right snapshot.

Before the fix, 5 of the 7 tests failed. The four tests without a persisted
value timed out because the Task never completed. The persisted-value test
found the DatabaseException on the run loop; its Task only completed through
the existing 3-second persisted-value fallback. The two valid-response tests
passed before and after the fix.

Commands:

./gradlew --max-workers=4 :firebase-database:spotlessApply
./gradlew --max-workers=4 :firebase-database:testDebugUnitTest --tests "com.google.firebase.database.core.RepoGetValueTest"
./gradlew --max-workers=4 :firebase-database:testDebugUnitTest

Results: spotlessApply succeeded. RepoGetValueTest ran 7 tests with 0
failures (before the fix: 7 tests, 5 failures). The full module run ran 410
tests with 0 failures and 2 skipped (both already marked @Ignore).

[gemini-code-assist]

Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

[github-actions]

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request addresses a crash in the Firebase Database SDK when the server returns data that cannot be parsed during a get() call. The SDK now catches DatabaseException during parsing, failing the task or falling back to a locally cached value if one exists, and a new test suite has been added to verify these scenarios. Feedback was provided to format the CHANGELOG entry to comply with the repository's 78-character line limit rule.

[gemini-code-assist]

The first line of the CHANGELOG entry exceeds the 78-character limit specified in the repository style guide. Keeping the first line to at most 78 characters ensures it remains within 80 characters when the SCM/release-notes generator converts - [fixed] to * {{fixed}}.

Suggested change

	- [fixed] Fixed a crash when the server's response to a `get()` call contained
	data that the SDK couldn't parse. The `Task` now fails instead, or returns
	the locally cached value if there is one. (#8777)
	- [fixed] Fixed a crash when the server's response to a `get()` call
	contained data that the SDK couldn't parse. The `Task` now fails instead,
	or returns the locally cached value if there is one. (#8777)

References

1. When writing CHANGELOG.md entries, keep the first line to at most 78 characters so that it remains within 80 characters when the tag is converted in generated release notes. ^(link)

[paulb777]

Done in 9e53878: rewrapped the entry so the first line is 68 characters (it was exactly 78, so at the limit rather than over it).

[paulb777]

/gemini review

[gemini-code-assist]

Code Review

This pull request fixes a crash in the Firebase Database SDK that occurs when the server's response to a get() call contains unparseable data, such as an invalid priority. The fix wraps the JSON parsing logic in a try-catch block within Repo.java. If a DatabaseException is caught, the SDK now logs the error and either returns a locally cached value if available or fails the task with the exception, rather than crashing the application. Additionally, a comprehensive set of unit tests has been added in RepoGetValueTest.java to verify this behavior under various scenarios, and the CHANGELOG.md has been updated accordingly. I have no feedback to provide as there are no review comments.