Skip to content

Commit 0adbab8

Browse files
nileshpatil6GWeale
authored andcommitted
fix(firestore): serialize session state as JSON to avoid reserved field name errors
Merge #5549 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: - \`create_session\`: write \`"state": json.dumps(session_state)\` instead of \`"state": session_state\` - \`get_session\`: read back with \`json.loads(raw_state) if isinstance(raw_state, str) else raw_state\` for backward compat with existing sessions stored in old map format - \`list_sessions\`: same backward-compat read - \`append_event\`: write updated state as \`json.dumps(session_only_state)\` ## 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** 1. \`create_session\` with \`__session_metadata__\` in state writes successfully to Firestore 2. \`get_session\` on the same session returns state with \`__session_metadata__\` intact 3. \`append_event\` that updates session state (e.g. sets a new key) writes and reads back correctly 4. Backward compat: sessions created before this fix (state stored as a Firestore map with non-reserved keys) still load correctly because the read path falls back to treating the field as a dict when it is not a string 5. ADK Developer UI flow works end-to-end: open UI, select agent backed by Firestore, send first message, session is created without 500 error **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" loaded = await firestore_session_service.get_session( app_name="test_app", user_id="test_user", session_id=session.id, ) assert loaded.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. Co-authored-by: George Weale <gweale@google.com> COPYBARA_INTEGRATE_REVIEW=#5549 from nileshpatil6:fix/firestore-reserved-state-keys 2eb917c PiperOrigin-RevId: 933935052
1 parent a181a39 commit 0adbab8

2 files changed

Lines changed: 16 additions & 7 deletions

File tree

src/google/adk/integrations/firestore/firestore_session_service.py

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
from contextlib import asynccontextmanager
1919
from datetime import datetime
2020
from datetime import timezone
21+
import json
2122
import logging
2223
import os
2324
from typing import Any
@@ -202,7 +203,7 @@ async def create_session(
202203
"id": session_id,
203204
"appName": app_name,
204205
"userId": user_id,
205-
"state": session_state,
206+
"state": json.dumps(session_state),
206207
"createTime": now,
207208
"updateTime": now,
208209
"revision": 1,
@@ -313,7 +314,10 @@ async def get_session(
313314
events.append(Event.model_validate(ed))
314315

315316
# Let's continue getting session.
316-
session_state = data.get("state", {})
317+
raw_state = data.get("state", {})
318+
session_state = (
319+
json.loads(raw_state) if isinstance(raw_state, str) else raw_state
320+
)
317321

318322
# Fetch shared state
319323
app_ref = self.client.collection(self.app_state_collection).document(
@@ -406,7 +410,12 @@ async def list_sessions(
406410
data = doc.to_dict()
407411
if data:
408412
u_id = data["userId"]
409-
s_state = data.get("state", {})
413+
raw_s_state = data.get("state", {})
414+
s_state = (
415+
json.loads(raw_s_state)
416+
if isinstance(raw_s_state, str)
417+
else raw_s_state
418+
)
410419
u_state = user_states_map.get(u_id, {})
411420
merged = self._merge_state(app_state, u_state, s_state)
412421

@@ -555,7 +564,7 @@ async def _append_txn(transaction: firestore.AsyncTransaction) -> int:
555564
transaction.update(
556565
session_ref,
557566
{
558-
"state": session_only_state,
567+
"state": json.dumps(session_only_state),
559568
"updateTime": firestore.SERVER_TIMESTAMP,
560569
"revision": new_revision,
561570
},

tests/unittests/integrations/firestore/test_firestore_session_service.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
from __future__ import annotations
1616

17+
import json
1718
from unittest import mock
1819

1920
from google.adk.events.event import Event
@@ -117,7 +118,7 @@ async def test_create_session(mock_firestore_client):
117118
assert args[1]["id"] == session.id
118119
assert args[1]["appName"] == app_name
119120
assert args[1]["userId"] == user_id
120-
assert args[1]["state"] == {}
121+
assert json.loads(args[1]["state"]) == {}
121122
assert args[1]["createTime"] == firestore.SERVER_TIMESTAMP
122123
assert args[1]["updateTime"] == firestore.SERVER_TIMESTAMP
123124

@@ -324,8 +325,7 @@ async def test_append_event_with_state_delta(mock_firestore_client):
324325

325326
transaction.update.assert_called_once()
326327
args, kwargs = transaction.update.call_args
327-
# In modular Firestore configurations alignments, updating variables mock assertions core setups
328-
assert args[1]["state"] == session.state
328+
assert json.loads(args[1]["state"]) == session.state
329329
assert args[1]["updateTime"] == firestore.SERVER_TIMESTAMP
330330

331331

0 commit comments

Comments
 (0)