Add FirestoreStore backend (aio + generated sync) #249
Conversation
📝 WalkthroughWalkthroughAdds Firestore backend support to the py-key-value and py-key-value-aio libraries. Includes FirestoreStore implementations for async and sync variants (client- or credentials-initialized), documentation and dependency updates, tests with in-memory Firestore mocks, and an async→sync name mapping update for code generation. Changes
Sequence Diagram(s)mermaid Caller->>FirestoreStore: get(key, collection?) Caller->>FirestoreStore: delete(key, collection?) Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-value/key-value-sync/pyproject.toml (1)
93-101: cryptography>=45.0.0 is a yanked release and will cause dev installs to fail.Verification confirms the concern: cryptography 45.0.0 was released May 17, 2025 but was yanked, so pip cannot install it by default. This breaks the
devdependency group (line 100:py-key-value-sync[wrappers-encryption]) and thewrappers-encryptionoptional group (line 42).Pydantic 2.11.9 (released September 13, 2025) exists and is available—no issue there.
Specify an available cryptography version (e.g., >=44.0.0 or the next stable post-yank release).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
README.md(2 hunks)docs/getting-started.md(1 hunks)docs/stores.md(4 hunks)key-value/key-value-aio/pyproject.toml(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/firestore/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py(1 hunks)key-value/key-value-aio/tests/stores/firestore/__init__.py(1 hunks)key-value/key-value-aio/tests/stores/firestore/test_firestore.py(1 hunks)key-value/key-value-sync/pyproject.toml(3 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/stores/firestore/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/firestore/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py(1 hunks)scripts/build_sync_library.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/__init__.py (2)
key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py (1)
store(84-86)key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (1)
FirestoreStore(19-121)
key-value/key-value-aio/src/key_value/aio/stores/firestore/__init__.py (2)
key-value/key-value-aio/tests/stores/firestore/test_firestore.py (1)
store(81-83)key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (1)
FirestoreStore(20-122)
key-value/key-value-sync/src/key_value/sync/stores/firestore/__init__.py (2)
key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py (1)
store(84-86)key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (1)
FirestoreStore(19-121)
key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(187-199)load_dict(67-106)dump_dict(114-158)
key-value/key-value-aio/tests/stores/firestore/test_firestore.py (1)
key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (1)
FirestoreStore(20-122)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (2)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(187-199)load_dict(67-106)dump_dict(114-158)
🪛 LanguageTool
README.md
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...Valkey, Firestore, and In-memory, Disk, etc - TTL support: Automatic expiration...
(ETC_PERIOD)
🔇 Additional comments (24)
scripts/build_sync_library.py (1)
224-224: The mapping is correct. AllAsyncClientusage in the codebase is qualified with the Firestore module prefix (firestore.AsyncClient), and no other libraries use bareAsyncClientimports. The mapping follows the existing pattern for async-to-sync renaming and poses no conflict risk in the current codebase.key-value/key-value-sync/pyproject.toml (2)
109-111: Pyright exclude change seems fine; ensure path is stable post-codegen.
No concerns with excluding the generated redis sync store file, but please confirm the codegen path won’t change acrossmake codegenruns (otherwise pyright configs can drift).
52-75: google-auth in firestore extras is redundant but harmless.The
firestoreextras correctly includesgoogle-cloud-firestore>=2.13.0andgoogle-auth>=2.24.0. However,google-authis transitive viagoogle-api-core(whichgoogle-cloud-firestorerequires), so the explicit constraint is unnecessary. Keeping it is not wrong—just redundant.The
allextras structure is intentional: it includes all storage backends plus utility packages (pydantic, keyring), but correctly excludeswrappers-encryption(optional encryption feature) andkeyring-linux(platform-specific extension of keyring). This is consistent with the project's design.key-value/key-value-aio/tests/stores/firestore/__init__.py (1)
1-1: LGTM!Standard test package initialization.
key-value/key-value-sync/tests/code_gen/stores/firestore/__init__.py (1)
1-4: LGTM!Auto-generated test package stub.
key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py (1)
80-133: LGTM!Good test coverage including idempotent delete behavior specific to Firestore and default collection handling.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (3)
110-115:_delete_managed_entryalways returnsTrue.Firestore deletes are idempotent (no error for missing docs), but returning
Trueeven when the document didn't exist may be misleading for callers expecting a "was deleted" semantic. This is consistent with the test overrides, so likely intentional — just flagging for awareness.
117-121: LGTM!Correct resource management — only closes internally-created clients.
94-101: No issue found. Thedefault_collectionis guaranteed to be non-None byBaseStore.__init__, which enforcesself.default_collection = default_collection or DEFAULT_COLLECTION_NAME. The Firestore API call at line 97 is safe.key-value/key-value-aio/pyproject.toml (1)
74-74: LGTM!Firestore properly included in dev extras.
README.md (1)
135-137: LGTM!Firestore installation instructions are clear and consistent with other backends.
docs/getting-started.md (1)
33-34: LGTM!Firestore installation instructions follow the established pattern for other backends.
key-value/key-value-sync/src/key_value/sync/stores/firestore/__init__.py (1)
1-8: LGTM!Generated sync wrapper follows the established pattern for re-exporting from code_gen.
docs/stores.md (1)
471-499: LGTM!FirestoreStore documentation follows the established pattern and clearly marks the storage format as unstable, which is appropriate for a new backend.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/__init__.py (1)
1-8: LGTM!Generated code_gen module follows the project's code generation patterns.
key-value/key-value-aio/src/key_value/aio/stores/firestore/__init__.py (1)
1-5: LGTM!Clean public API export following the project's module organization pattern.
key-value/key-value-aio/tests/stores/firestore/test_firestore.py (4)
8-13: LGTM!Dependency handling follows the project's established pattern for optional backends.
78-118: LGTM!Test overrides are well-documented, explaining Firestore's idempotent delete behavior. The skip for unbounded tests is appropriate for distributed cloud stores.
119-130: LGTM!Custom tests appropriately verify Firestore-specific behavior (default collection usage and delete return values).
57-74: Missingsuper().__init__()is intentional for test isolation.The
InMemoryAsyncFirestoreClientinherits fromfirestore.AsyncClientbut bypasses parent initialization. This is by design—the mock isolates the test from Firestore SDK dependencies by implementing only the required subset of the API (collection(),close(), and the internal storage pattern). Tests are already passing with this approach, so no action needed.key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (4)
12-17: LGTM!Dependency handling is clear and provides actionable guidance for users missing the optional extras.
20-50: LGTM!Type overloads clearly document the two initialization patterns (client-based vs credentials-based) with comprehensive docstrings.
94-102: LGTM!Get operation correctly handles missing documents and uses the serialization adapter for deserialization.
104-109: LGTM!Put operation correctly serializes and stores entries. Firestore's
set()is idempotent by design.
| "rocksdict>=0.3.2 ; python_version < '3.12'" | ||
| ] | ||
| duckdb = ["duckdb>=1.1.1", "pytz>=2025.2"] | ||
| firestore = ["google-cloud-firestore>=2.13.0", "google-auth>=2.24.0"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check latest versions of firestore dependencies
curl -s https://pypi.org/pypi/google-cloud-firestore/json | jq '.info.version'
curl -s https://pypi.org/pypi/google-auth/json | jq '.info.version'Repository: strawgate/py-key-value
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Find and examine pyproject.toml to see other store backend patterns
cd key-value/key-value-aio
cat -n pyproject.toml | head -80Repository: strawgate/py-key-value
Length of output: 3015
Consider updating the firestore dependency versions. The specified constraints are functional but outdated—google-cloud-firestore>=2.13.0 (current: 2.21.0) and google-auth>=2.24.0 (current: 2.43.0) have significantly newer versions available. Update to reflect the latest stable releases for better dependency freshness and potential bug fixes.
🤖 Prompt for AI Agents
In key-value/key-value-aio/pyproject.toml around line 53, the firestore
dependency constraints are outdated; update the version specifiers to reflect
current stable releases by changing "google-cloud-firestore>=2.13.0" to
"google-cloud-firestore>=2.21.0" and "google-auth>=2.24.0" to
"google-auth>=2.43.0" (or to the exact desired newer pinned versions) so the
project uses the latest compatible packages.
| @override | ||
| async def _close(self) -> None: | ||
| """Close the Firestore client.""" | ||
| if self._client and not self._client_provided_by_user: | ||
| self._client.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
google-cloud-firestore AsyncClient.close() async coroutine documentation
💡 Result:
Short answer: AsyncClient.close() is an async coroutine — await it (e.g. await client.close()). See the AsyncClient reference for the Firestore async client. [1]
References:
- AsyncClient reference (google-cloud-firestore Python library). [1]
- Firestore Python library reference listing (module async_client). [2]
[1] https://cloud.google.com/python/docs/reference/firestore/latest/google.cloud.firestore_v1.async_client
[2] https://cloud.google.com/python/docs/reference/firestore/latest/
Fix async close() call in _close method.
AsyncClient.close() is an async method and must be awaited. Line 122 currently calls self._client.close() without await, which creates a coroutine that never executes, leaving the client unclosed.
Change line 122 to:
await self._client.close()🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py around
lines 118 to 122, the _close method calls the Firestore AsyncClient.close()
without awaiting it; change the call to await the coroutine (i.e., await
self._client.close()) so the client actually closes, ensuring the method remains
async and handles the case where the client was provided by the user unchanged.
There was a problem hiding this comment.
Google Client is closed synchronously. This is method from the source code:
class Client(_ClientFactoryMixin):
def close(self):
"""Clean up transport, if set.
Suggested use:
.. code-block:: python
import contextlib
with contextlib.closing(client): # closes on exit
do_something_with(client)
"""
if self._http_internal is not None:
self._http_internal.close()There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
google-cloud-firestore AsyncClient close method signature python
💡 Result:
The AsyncClient close method is asynchronous with this signature:
async def close(self) -> None
(See the google-cloud-firestore AsyncClient reference.) [1]
Sources
- Google Cloud Python Firestore AsyncClient reference. [1]
@alexeylark, thank you for checking! However, the code snippet you shared is from the synchronous Client class, while this PR uses firestore.AsyncClient (see lines 26, 29, and 78).
According to the latest documentation, AsyncClient.close() has the signature async def close(self) -> None and must be awaited. The sync Client.close() you showed is indeed synchronous, but that's a different class.
Since line 122 uses self._client (which is typed as firestore.AsyncClient), the call should be:
await self._client.close()✏️ Learnings added
Learnt from: alexeylark
Repo: strawgate/py-key-value PR: 249
File: key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py:118-122
Timestamp: 2025-12-14T15:23:26.567Z
Learning: In google-cloud-firestore, AsyncClient.close() is an async method with signature `async def close(self) -> None` and must be awaited, while the synchronous Client.close() is a regular synchronous method.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| except ImportError: # pragma: no cover | ||
| pytest.skip("Firestore dependencies not installed. Install with `py-key-value-aio[firestore]`.", allow_module_level=True) |
There was a problem hiding this comment.
Skip message references wrong package.
This is the sync library, but the skip message references py-key-value-aio[firestore]. Should be generic or reference the sync package.
- pytest.skip("Firestore dependencies not installed. Install with `py-key-value-aio[firestore]`.", allow_module_level=True)
+ pytest.skip("Firestore dependencies not installed. Install with the [firestore] extra.", allow_module_level=True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except ImportError: # pragma: no cover | |
| pytest.skip("Firestore dependencies not installed. Install with `py-key-value-aio[firestore]`.", allow_module_level=True) | |
| except ImportError: # pragma: no cover | |
| pytest.skip("Firestore dependencies not installed. Install with the [firestore] extra.", allow_module_level=True) |
🤖 Prompt for AI Agents
In key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py
around lines 15-16, the pytest.skip message incorrectly references the async
package `py-key-value-aio[firestore]`; update the skip message to reference the
sync package (for example `py-key-value[firestore]`) or use a generic message
like "Firestore dependencies not installed. Install the package with the
firestore extras." so the skip text correctly reflects this is the sync library
while leaving pytest.skip(..., allow_module_level=True) unchanged.
| class InMemoryAsyncFirestoreClient(firestore.Client): | ||
| """Minimal in-memory Firestore Client replacement for tests. | ||
| This client mimics the subset of the Firestore Client API that | ||
| FirestoreStore relies on: `collection().document().get/set/delete()`. | ||
| """ | ||
|
|
||
| __slots__ = ("_storage", "closed") | ||
|
|
||
| def __init__(self) -> None: | ||
| self._storage: dict[tuple[str, str], dict[str, Any]] = {} | ||
| self.closed = False | ||
|
|
||
| def collection(self, name: str) -> _InMemoryAsyncFirestoreCollection: | ||
| return _InMemoryAsyncFirestoreCollection(storage=self._storage, name=name) | ||
|
|
||
| def close(self) -> None: | ||
| self.closed = True | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mock client skips parent __init__ — fragile but acceptable for tests.
InMemoryAsyncFirestoreClient inherits from firestore.Client without calling super().__init__(). This works because tests only exercise collection() and close(), but could break if base class expectations change or if additional methods are called. Consider using unittest.mock.MagicMock(spec=firestore.Client) for more robust mocking, or document this limitation.
🤖 Prompt for AI Agents
key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py
around lines 60 to 78: the InMemoryAsyncFirestoreClient subclass of
firestore.Client does not call super().__init__(), which is fragile if the base
class adds required initialization; either call super().__init__() in
InMemoryAsyncFirestoreClient.__init__() to perform proper base-class
initialization, or replace this class with a
unittest.mock.MagicMock(spec=firestore.Client) configured to provide
collection()/close() behavior, and add a short comment documenting the chosen
approach and its testing limitation.
|
|
||
| - **Multiple backends**: DynamoDB, Elasticsearch, Memcached, MongoDB, Redis, | ||
| RocksDB, Valkey, and In-memory, Disk, etc | ||
| RocksDB, Valkey, Firestore, and In-memory, Disk, etc |
There was a problem hiding this comment.
Fix punctuation per American English style.
The static analysis tool correctly flags that "etc." requires a period in American English.
Apply this diff:
-- **Multiple backends**: DynamoDB, Elasticsearch, Memcached, MongoDB, Redis,
- RocksDB, Valkey, Firestore, and In-memory, Disk, etc
+- **Multiple backends**: DynamoDB, Elasticsearch, Memcached, MongoDB, Redis,
+ RocksDB, Valkey, Firestore, and In-memory, Disk, etc.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RocksDB, Valkey, Firestore, and In-memory, Disk, etc | |
| RocksDB, Valkey, Firestore, and In-memory, Disk, etc. |
🧰 Tools
🪛 LanguageTool
[style] ~21-~21: In American English, abbreviations like “etc.” require a period.
Context: ...Valkey, Firestore, and In-memory, Disk, etc - TTL support: Automatic expiration...
(ETC_PERIOD)
🤖 Prompt for AI Agents
In README.md around line 21, the list contains inconsistent capitalization and
is missing the period for "etc" per American English; change "RocksDB, Valkey,
Firestore, and In-memory, Disk, etc" to use consistent lowercase for the device
types and add the period — e.g. "RocksDB, Valkey, Firestore, and in-memory,
disk, etc." — ensuring the sentence ends with the period after "etc.".
|
Thanks, will try to find time to review this week. I've enabled the tests which may require some follow-up before this is ready |
|
Thank you! |
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py (1)
17-18: Skip message references wrong package.This is the sync library, but the skip message references
py-key-value-aio[firestore]. Should reference the sync package or use a generic message.- pytest.skip("Firestore dependencies not installed. Install with `py-key-value-aio[firestore]`.", allow_module_level=True) + pytest.skip("Firestore dependencies not installed. Install with the [firestore] extra.", allow_module_level=True)key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (1)
118-121: **AsyncClient.close()must be awaited.**Per past reviews and retrieved learnings,firestore.AsyncClient.close()isasync def close(self) -> Noneand must be awaited. Line 121 callsself._client.close()withoutawait, creating an unawaited coroutine that silently leaves the client open.async def _close(self) -> None: """Close the Firestore client.""" if self._client and not self._client_provided_by_user: - self._client.close() + await self._client.close()Based on learnings, ensure the sync variant in
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.pydoes NOT await (syncClient.close()is synchronous).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
AGENTS.md(1 hunks)docs/adapters.md(1 hunks)docs/wrappers.md(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py(1 hunks)key-value/key-value-aio/tests/stores/firestore/test_firestore.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-14T15:23:26.567Z
Learnt from: alexeylark
Repo: strawgate/py-key-value PR: 249
File: key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py:118-122
Timestamp: 2025-12-14T15:23:26.567Z
Learning: In files that use google-cloud-firestore in the async path, ensure that AsyncClient.close() is awaited (signature async def close(self) -> None) while the synchronous Client.close() remains a regular method and should not be awaited. Add tests or lint rules to enforce awaiting for async clients and not awaiting sync clients. This prevents runtime errors and ensures proper resource cleanup.
Applied to files:
key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py
📚 Learning: 2025-12-14T15:23:26.567Z
Learnt from: alexeylark
Repo: strawgate/py-key-value PR: 249
File: key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py:118-122
Timestamp: 2025-12-14T15:23:26.567Z
Learning: In google-cloud-firestore, AsyncClient.close() is an async method with signature `async def close(self) -> None` and must be awaited, while the synchronous Client.close() is a regular synchronous method.
Applied to files:
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py
🧬 Code graph analysis (4)
key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py (2)
key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (1)
FirestoreStore(20-121)key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (1)
FirestoreStore(19-120)
key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (3)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(187-199)load_dict(67-106)dump_dict(114-158)key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (6)
FirestoreStore(19-120)_connected_client(87-91)_get_managed_entry(94-101)_put_managed_entry(104-108)_delete_managed_entry(111-115)_close(117-120)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (4)
key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
BasicSerializationAdapter(187-199)load_dict(67-106)dump_dict(114-158)key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (6)
FirestoreStore(20-121)_connected_client(88-92)_get_managed_entry(95-102)_put_managed_entry(105-109)_delete_managed_entry(112-116)_close(118-121)key-value/key-value-shared/tests/utils/test_serialization.py (1)
serialization_adapter(26-27)
key-value/key-value-aio/tests/stores/firestore/test_firestore.py (1)
key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (1)
FirestoreStore(20-121)
🔇 Additional comments (16)
AGENTS.md (1)
251-268: Markdown table formatting is correct.The table structure follows standard Markdown syntax: header row with column titles, separator row with dashes, and data rows. The formatting aligns with the project's Markdown linting standards (line 72-73 of this file).
docs/wrappers.md (1)
10-10: Verify documentation linting passes for the table format.Line 10 shows valid Markdown table separator syntax. However, given the PR commit mentions "fixing documentation table linting," please confirm that:
- Documentation formatting tests now pass
- This table separator meets the project's Markdown linting standards (e.g., alignment indicators, pipe consistency)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/firestore/store.py (8)
1-3: LGTM: Clear auto-generation warning.The warning header appropriately prevents manual modifications to generated code.
11-16: LGTM: Clear import error handling.The ImportError provides actionable guidance for missing dependencies.
28-84: LGTM: Well-structured initialization with proper overloads.The constructor correctly:
- Provides type-safe overloads for client vs. credentials-based initialization
- Creates sync
firestore.Client(not AsyncClient)- Tracks client ownership for proper cleanup
- Uses
value_format="string"consistent with the async version
86-91: LGTM: Straightforward connection guard.The property provides a clear error if the client is unavailable.
94-101: LGTM: Correct synchronous get operation.The method properly uses sync Firestore operations and returns None for missing documents.
104-108: LGTM: Correct synchronous put operation.The method properly serializes and stores the managed entry using sync operations.
111-115: LGTM: Correct synchronous delete operation.The method uses sync operations and correctly returns True (Firestore deletes are idempotent).
117-120: LGTM: Correct synchronous cleanup.The method correctly calls sync
Client.close()without await. Per learnings, the synchronous Client.close() is a regular method (not async), so this implementation is correct.key-value/key-value-sync/tests/code_gen/stores/firestore/test_firestore.py (1)
82-135: Test coverage looks solid.Tests cover CRUD operations, idempotent deletes, bulk operations, and default collection behavior. The overrides for Firestore's idempotent delete semantics are appropriate.
key-value/key-value-aio/tests/stores/firestore/test_firestore.py (2)
75-76: Mock'sclose()is sync but realAsyncClient.close()is async.Per the google-cloud-firestore docs,
AsyncClient.close()isasync def close(self) -> None. The mock uses a sync method, which works here since tests don't await it, but this diverges from the real API behavior.Consider making this async to match the real client signature:
- def close(self) -> None: + async def close(self) -> None: self.closed = True
79-132: Async test coverage is comprehensive.Tests properly cover CRUD flows, idempotent deletes, bulk operations, and default collection fallback. All async operations correctly use
await.docs/adapters.md (1)
9-10: Trivial table formatting update.No functional changes.
key-value/key-value-aio/src/key_value/aio/stores/firestore/store.py (2)
51-85: Initialization logic is well-structured.Overloads provide clear API, and the implementation correctly tracks
client_provided_by_userfor proper resource management. Serialization adapter configured appropriately.
94-116: CRUD methods correctly use async/await.All Firestore operations are properly awaited. Collection fallback to
default_collectionis consistent across methods.
| class InMemoryAsyncFirestoreClient(firestore.AsyncClient): | ||
| """Minimal in-memory Firestore AsyncClient replacement for tests. | ||
| This client mimics the subset of the Firestore AsyncClient API that | ||
| FirestoreStore relies on: `collection().document().get/set/delete()`. | ||
| """ | ||
|
|
||
| __slots__ = ("_storage", "closed") | ||
|
|
||
| def __init__(self) -> None: | ||
| self._storage: dict[tuple[str, str], dict[str, Any]] = {} | ||
| self.closed = False | ||
|
|
||
| def collection(self, name: str) -> _InMemoryAsyncFirestoreCollection: | ||
| return _InMemoryAsyncFirestoreCollection(storage=self._storage, name=name) | ||
|
|
||
| def close(self) -> None: | ||
| self.closed = True |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mock client skips parent __init__ — fragile but acceptable for tests.
InMemoryAsyncFirestoreClient inherits from firestore.AsyncClient without calling super().__init__(). Works because tests only exercise collection() and close(), but could break if base class assumptions change. Consider documenting this limitation or using MagicMock(spec=firestore.AsyncClient).
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/firestore/test_firestore.py around lines
59 to 76, the InMemoryAsyncFirestoreClient subclass of firestore.AsyncClient
does not call super().__init__() which is fragile if the base class later
requires initialization; either call super().__init__() in the mock constructor
(passing any required args if needed) to properly initialize the base class, or
explicitly document the deliberate omission with a comment stating the mock only
implements collection()/close() for these tests, or replace this class with a
MagicMock(spec=firestore.AsyncClient) to avoid inheriting behavior from the real
client.
| class _InMemoryAsyncFirestoreDocument: | ||
| def __init__(self, storage: dict[tuple[str, str], dict[str, Any]], collection: str, key: str) -> None: | ||
| self._storage = storage | ||
| self._collection = collection | ||
| self._key = key | ||
|
|
||
| def get(self) -> "_InMemoryAsyncFirestoreDocumentSnapshot": | ||
| return _InMemoryAsyncFirestoreDocumentSnapshot(self._storage.get((self._collection, self._key))) | ||
|
|
||
| def set(self, data: dict[str, Any]) -> None: | ||
| self._storage[self._collection, self._key] = data | ||
|
|
||
| def delete(self) -> None: | ||
| self._storage.pop((self._collection, self._key), None) | ||
|
|
||
|
|
||
| class _InMemoryAsyncFirestoreDocumentSnapshot: | ||
| def __init__(self, data: dict[str, Any] | None) -> None: | ||
| self._data = data | ||
|
|
||
| @property | ||
| def exists(self) -> bool: | ||
| return self._data is not None | ||
|
|
||
| def to_dict(self) -> dict[str, Any] | None: | ||
| if self._data is None: | ||
| return None | ||
| return dict(self._data) | ||
|
|
||
|
|
||
| class _InMemoryAsyncFirestoreCollection: | ||
| def __init__(self, storage: dict[tuple[str, str], dict[str, Any]], name: str) -> None: | ||
| self._storage = storage | ||
| self._name = name | ||
|
|
||
| def document(self, key: str) -> _InMemoryAsyncFirestoreDocument: | ||
| return _InMemoryAsyncFirestoreDocument(storage=self._storage, collection=self._name, key=key) | ||
|
|
||
|
|
||
| class InMemoryAsyncFirestoreClient(firestore.Client): | ||
| """Minimal in-memory Firestore Client replacement for tests. | ||
| This client mimics the subset of the Firestore Client API that | ||
| FirestoreStore relies on: `collection().document().get/set/delete()`. | ||
| """ | ||
|
|
||
| __slots__ = ("_storage", "closed") | ||
|
|
||
| def __init__(self) -> None: | ||
| self._storage: dict[tuple[str, str], dict[str, Any]] = {} | ||
| self.closed = False | ||
|
|
||
| def collection(self, name: str) -> _InMemoryAsyncFirestoreCollection: | ||
| return _InMemoryAsyncFirestoreCollection(storage=self._storage, name=name) | ||
|
|
||
| def close(self) -> None: | ||
| self.closed = True | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Class names retain "Async" prefix in sync-generated code.
The in-memory mock classes (e.g., _InMemoryAsyncFirestoreDocument, InMemoryAsyncFirestoreClient) use "Async" in their names despite being synchronous. This is a code-gen artifact that doesn't affect functionality but could be confusing.
Consider updating the code generator to rename these classes appropriately for the sync variant (e.g., _InMemoryFirestoreDocument, InMemoryFirestoreClient).
|
fyi |
|
@strawgate
I reviewed the output from GitHub tests and from CodeRabbit.
Please let me know if you spot anything else that needs changing. |
|
Hi @strawgate ! I hope you're doing well. I reviewed the logs from the failing tests, and I don't see how my code could've affected them. The code seems to be fine, if not for these fails. May you please advise on the course of action here? Thank you in advance! |
|
Thanks, yeah test failures look unrelated. I'll take a look at this over the next couple of days. I assume you're running this and everything is looking good? @claude can you do a thorough review of the implementation and tests and verify it follows the patterns in the repo? |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
3 participants
Motivation
We needed a Firestore implementation for our FastMCP build on GCP, and developed FirestoreStore as an extension of
py-key-value, which has been working for our case. Now we're contributing the code, so it's available from the main repository for everybody to use.Summary
Adds a Firestore-backed store implementation (FirestoreStore) to py-key-value-aio, with corresponding tests, packaging extras, docs updates, and generated sync code via make codegen.
Installation
pip install py-key-value-aio[firestore]
Usage
Tests / Validation
Notes
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.