Skip to content

Commit b981250

Browse files
AkshayShah03copybara-github
authored andcommitted
fix: stop list_sessions(user_id="") from listing every user in Firestore
Merge #7663 `FirestoreSessionService.list_sessions(user_id="")` used to fall through to the all-users `collection_group` query and return every user's sessions in the app. It now returns no sessions: Firestore rejects an empty document ID, so no session can be stored under `""`. Fixes #7662 PiperOrigin-RevId: 1006024020
1 parent 6c20444 commit b981250

2 files changed

Lines changed: 38 additions & 2 deletions

File tree

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,10 @@ async def list_sessions(
368368
self, *, app_name: str, user_id: Optional[str] = None
369369
) -> ListSessionsResponse:
370370
"""Lists sessions from Firestore."""
371-
if user_id:
371+
if user_id == "":
372+
# Firestore rejects an empty document ID, so no session can exist here.
373+
return ListSessionsResponse()
374+
if user_id is not None:
372375
query = self._get_sessions_ref(app_name, user_id).where(
373376
filter=FieldFilter("appName", "==", app_name)
374377
)
@@ -392,7 +395,7 @@ def _iter_sessions_data() -> Iterator[dict[str, Any]]:
392395
app_state = app_doc.to_dict() if app_doc.exists else {}
393396

394397
user_states_map = {}
395-
if user_id:
398+
if user_id is not None:
396399
user_ref = (
397400
self.client.collection(self.user_state_collection)
398401
.document(app_name)

‎tests/unittests/integrations/firestore/test_firestore_session_service.py‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -961,6 +961,39 @@ async def mock_get_all(refs):
961961
assert _where_filter(where) == ("appName", "==", app_name)
962962

963963

964+
@pytest.mark.asyncio
965+
async def test_list_sessions_with_empty_user_id_returns_no_sessions(
966+
mock_firestore_client,
967+
):
968+
"""list_sessions(user_id="") returns no sessions without querying Firestore.
969+
970+
The collection_group and get_all stubs serve the all-users path only, so the
971+
test fails if user_id="" falls through to it.
972+
"""
973+
service = FirestoreSessionService(client=mock_firestore_client)
974+
other_user_session = _stored_snapshot({
975+
"id": "s-alice",
976+
"appName": "test_app",
977+
"userId": "alice",
978+
"state": {},
979+
})
980+
mock_firestore_client.collection_group.return_value.where.return_value.get = (
981+
mock.AsyncMock(return_value=[other_user_session])
982+
)
983+
984+
async def mock_get_all(refs):
985+
for _ in refs:
986+
yield mock.MagicMock(exists=False)
987+
988+
mock_firestore_client.get_all = mock_get_all
989+
990+
response = await service.list_sessions(app_name="test_app", user_id="")
991+
992+
assert response.sessions == []
993+
mock_firestore_client.collection_group.assert_not_called()
994+
mock_firestore_client.collection.assert_not_called()
995+
996+
964997
@pytest.mark.asyncio
965998
async def test_get_user_state_returns_stored_user_state(mock_firestore_client):
966999
"""get_user_state returns the user_states document for the app and user."""

0 commit comments

Comments
 (0)