fix(firestore): serialize session state as JSON to avoid reserved field name errors#5549
Open
nileshpatil6 wants to merge 2 commits intogoogle:mainfrom
Open
fix(firestore): serialize session state as JSON to avoid reserved field name errors#5549nileshpatil6 wants to merge 2 commits intogoogle:mainfrom
nileshpatil6 wants to merge 2 commits intogoogle:mainfrom
Conversation
…served field name restrictions
Collaborator
|
Response from ADK Triaging Agent Hello @nileshpatil6, thank you for creating this PR! Could you please include a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5539
Problem
`FirestoreSessionService` stores session state as a raw Firestore map, writing each state key as a direct document field path. Firestore rejects field names matching `.*` with:
```
400 field name 'session_metadata' is reserved
```
The ADK Developer UI creates sessions with `session_metadata` in the initial state (used to store display names). This makes the UI incompatible with `FirestoreSessionService` -- the first chat message always fails with `500 Internal Server Error`.
Fix
Serialize the session state dict as a JSON string before writing to Firestore, and deserialize on read. This avoids Firestore field name restrictions entirely since the entire state is stored as a single opaque string field.
Changes:
Backward Compatibility
Existing sessions stored with the old dict format continue to work. The read path checks whether the stored value is a string (new format) or dict (old format) and handles both.
Why JSON string vs map
Firestore's `.*` restriction applies to all field paths including nested map sub-fields, so there is no way to store `session_metadata` as a Firestore map key at any level. Serializing the whole state as a JSON string sidesteps the restriction completely without any key escaping logic.
Testing Plan
Reproducing the original bug
The issue reporter's minimal repro already confirms the failure path. To verify locally before this fix:
```python
import asyncio
from google.adk.integrations.firestore.firestore_session_service import FirestoreSessionService
from google.cloud import firestore
async def main():
service = FirestoreSessionService(client=firestore.AsyncClient(), root_collection="adk-session")
await service.create_session(
app_name="test_app",
user_id="test_user",
session_id="test_session",
state={"session_metadata": {"displayName": "hello"}},
)
asyncio.run(main())
Before fix: raises google.api_core.exceptions.InvalidArgument: 400 field name 'session_metadata' is reserved
After fix: succeeds
```
What I verified manually
Unit test coverage
The existing unit tests in `tests/unittests/integrations/firestore/test_firestore_session_service.py` cover create/get/append_event for normal state keys. Running those tests after this change all pass (the mock Firestore client stores and returns the JSON string transparently).
A specific regression test for the reserved-key case would look like:
```python
async def test_create_session_with_reserved_key(firestore_session_service):
session = await firestore_session_service.create_session(
app_name="test_app",
user_id="test_user",
state={"session_metadata": {"displayName": "My Session"}},
)
assert session.state["session_metadata"]["displayName"] == "My Session"
```
Happy to add this as a formal test to the test file if that would help get this through review faster.