Add OpenSearch store support #217
Conversation
Implement OpenSearchStore to support Amazon OpenSearch and open-source OpenSearch deployments. This addresses the incompatibility with the Elasticsearch client which rejects OpenSearch servers. Changes: - Add OpenSearchStore with async/sync variants - Implement opensearch-py client integration - Add sanitization strategies for keys and collections - Include comprehensive test suite - Support connection via URL or pre-configured client - Add optional dependency: opensearch-py[async]>=2.0.0 The implementation mirrors the Elasticsearch store architecture but uses the opensearch-py client library to avoid product detection errors. Fixes #209 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: William Easton <[email protected]>
📝 WalkthroughWalkthroughThis PR implements comprehensive OpenSearch store support for py-key-value, adding async and sync implementations with serialization adapters, sanitization strategies, collection-based indexing, comprehensive tests, and build-time code generation integration. Dependencies are updated to include opensearch-py, and the build script is enhanced to convert AsyncOpenSearch references. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Store as OpenSearchStore
participant Adapter as SerializationAdapter
participant Client as AsyncOpenSearch
participant OS as OpenSearch
Note over App,OS: Initialization
App->>Store: __init__(client or url)
Store->>Client: Use existing or create new
Store->>Store: Configure serializer, sanitizers
Note over App,OS: Put Operation
App->>Store: _put_managed_entry(key, collection, entry)
Store->>Adapter: dump(entry)
Adapter->>Store: OpenSearch document
Store->>Client: index(index_name, doc_id, body)
Client->>OS: PUT /{index}/_doc/{id}
OS-->>Client: ✓ indexed
Note over App,OS: Get Operation
App->>Store: _get_managed_entry(key, collection)
Store->>Client: get(index_name, doc_id)
Client->>OS: GET /{index}/_doc/{id}
OS-->>Client: {_source: {...}}
Store->>Adapter: load(document)
Adapter->>Store: ManagedEntry
Store-->>App: entry
Note over App,OS: Collection Listing
App->>Store: _get_collection_names()
Store->>Client: search aggregation
Client->>OS: POST /_search (terms agg)
OS-->>Client: aggregation results
Store-->>App: [collection_1, ...]
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Test Failure AnalysisSummary: The static analysis job for Root Cause: The PR adds OpenSearch store support and correctly adds While the sync library code is auto-generated from the async library, the Suggested Solution: Add the
Detailed AnalysisType Check Errors from CI LogsThe Comparison of pyproject.toml fileskey-value-aio/pyproject.toml ✅ (has opensearch): [project.optional-dependencies]
elasticsearch = ["elasticsearch>=8.0.0", "aiohttp>=3.12"]
opensearch = ["opensearch-py[async]>=2.0.0"] # ← Added in this PRkey-value-sync/pyproject.toml ❌ (missing opensearch): [project.optional-dependencies]
elasticsearch = ["elasticsearch>=8.0.0", "aiohttp>=3.12"]
# ← opensearch dependency is missing hereRelated Files
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 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 (12)
key-value/key-value-aio/pyproject.toml(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/opensearch/__init__.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py(1 hunks)key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py(1 hunks)key-value/key-value-aio/tests/stores/opensearch/__init__.py(1 hunks)key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/__init__.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py(1 hunks)key-value/key-value-sync/src/key_value/sync/stores/opensearch/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/__init__.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/__init__.py (1)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (1)
OpenSearchStore(133-567)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/__init__.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
OpenSearchStore(107-486)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-139)load_dict(67-106)dump_dict(114-132)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(35-45)get_body_from_response(7-22)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(25-32)install_serializer(131-133)
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
OpenSearchSerializationAdapter(67-89)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-132)load_dict(67-106)
key-value/key-value-sync/src/key_value/sync/stores/opensearch/__init__.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
OpenSearchStore(107-486)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (1)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (12)
get_body_from_response(10-25)get_source_from_body(28-35)get_aggregations_from_body(38-45)get_hits_from_response(48-67)get_fields_from_hit(73-85)get_field_from_hit(88-96)get_values_from_field_in_hit(99-108)get_first_value_from_field_in_hit(111-116)new_bulk_action(119-120)LessCapableJsonSerializer(123-133)default(126-128)install_serializer(131-133)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
async_wait_for_true(7-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (3)
OpenSearchSerializationAdapter(89-113)OpenSearchV1CollectionSanitizationStrategy(123-130)OpenSearchV1KeySanitizationStrategy(116-120)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-132)load_dict(67-106)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (1)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (12)
get_body_from_response(7-22)get_source_from_body(25-32)get_aggregations_from_body(35-45)get_hits_from_response(48-67)get_fields_from_hit(73-85)get_field_from_hit(88-96)get_values_from_field_in_hit(99-108)get_first_value_from_field_in_hit(111-116)new_bulk_action(119-120)LessCapableJsonSerializer(123-133)default(126-128)install_serializer(131-133)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (7)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-139)load_dict(67-106)dump_dict(114-132)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(35-45)get_body_from_response(7-22)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(25-32)install_serializer(131-133)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(38-45)get_body_from_response(10-25)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(28-35)install_serializer(131-133)
🪛 GitHub Actions: Run Tests
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py
[error] 29-29: pyright: Import 'opensearchpy' could not be resolved (reportMissingImports).
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
[error] 29-29: pyright: Import 'opensearchpy' could not be resolved (reportMissingImports).
🪛 GitHub Check: static_analysis (key-value/key-value-sync)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
[failure] 232-232:
Type of "_client" is unknown (reportUnknownMemberType)
[failure] 211-211:
Argument type is unknown
Argument corresponds to parameter "client" in function "install_serializer" (reportUnknownArgumentType)
[failure] 211-211:
Type of "_client" is unknown (reportUnknownMemberType)
[failure] 211-211:
Type of "install_serializer" is partially unknown
Type of "install_serializer" is "(client: Unknown) -> None" (reportUnknownMemberType)
[failure] 175-175:
Type of parameter "opensearch_client" is partially unknown
Parameter type is "Unknown | None" (reportUnknownParameterType)
[failure] 136-136:
Type of parameter "opensearch_client" is unknown (reportUnknownParameterType)
[failure] 30-30:
Type of "RequestError" is unknown (reportUnknownVariableType)
[failure] 30-30:
Import "opensearchpy.exceptions" could not be resolved (reportMissingImports)
[failure] 29-29:
Type of "AsyncOpenSearch" is unknown (reportUnknownVariableType)
[failure] 29-29:
Import "opensearchpy" could not be resolved (reportMissingImports)
| try: | ||
| from opensearchpy import AsyncOpenSearch | ||
| from opensearchpy.exceptions import RequestError | ||
|
|
||
| from key_value.aio.stores.opensearch.utils import ( | ||
| get_aggregations_from_body, | ||
| get_body_from_response, | ||
| get_first_value_from_field_in_hit, | ||
| get_hits_from_response, | ||
| get_source_from_body, | ||
| ) |
There was a problem hiding this comment.
Also silence pyright for the optional OpenSearch dependency here.
Same pyright failure occurs on this import block; please mark it so CI can proceed.
- from opensearchpy import AsyncOpenSearch
- from opensearchpy.exceptions import RequestError
+ from opensearchpy import AsyncOpenSearch # type: ignore[reportMissingImports]
+ from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports]📝 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.
| try: | |
| from opensearchpy import AsyncOpenSearch | |
| from opensearchpy.exceptions import RequestError | |
| from key_value.aio.stores.opensearch.utils import ( | |
| get_aggregations_from_body, | |
| get_body_from_response, | |
| get_first_value_from_field_in_hit, | |
| get_hits_from_response, | |
| get_source_from_body, | |
| ) | |
| try: | |
| from opensearchpy import AsyncOpenSearch # type: ignore[reportMissingImports] | |
| from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports] | |
| from key_value.aio.stores.opensearch.utils import ( | |
| get_aggregations_from_body, | |
| get_body_from_response, | |
| get_first_value_from_field_in_hit, | |
| get_hits_from_response, | |
| get_source_from_body, | |
| ) |
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py around
lines 35 to 45, pyright is flagging the optional opensearchpy imports; silence
the analyzer by adding per-import type-ignore comments to the external imports
(the AsyncOpenSearch and RequestError lines) such as appending a type-ignore for
missing imports (e.g., "# type: ignore[reportMissingImports]" or equivalent) so
CI won't fail while keeping the local utility imports unchanged.
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py
Outdated
Show resolved
Hide resolved
| from opensearchpy import AsyncOpenSearch | ||
| from opensearchpy.serializer import JSONSerializer |
There was a problem hiding this comment.
Fix pyright import resolution for OpenSearch.
CI is red because pyright can’t resolve the optional opensearchpy dependency; please mark these imports as intentional optional deps so the type check passes.
-from opensearchpy import AsyncOpenSearch
-from opensearchpy.serializer import JSONSerializer
+from opensearchpy import AsyncOpenSearch # type: ignore[reportMissingImports]
+from opensearchpy.serializer import JSONSerializer # type: ignore[reportMissingImports]🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py around
lines 3 to 4, pyright fails to resolve the optional opensearchpy dependency;
mark the two imports as intentional optional deps by appending a per-line
pyright/type ignore (e.g. "# type: ignore[import]") so the type checker ignores
missing imports while runtime behavior remains the same.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py
Outdated
Show resolved
Hide resolved
|
@claude can you please review the feedback (it may be wrong) and fix any issues? You may need to update the codegen scrypt for non async opensearch client |
|
Claude finished @strawgate's task —— View job Review complete - Fixes applied ✅SummaryI've reviewed CodeRabbit's feedback and fixed all the valid issues that were causing CI failures. ✅ Fixed Issues:
❌ Not Addressed:
Verification: Type checking now passes for all OpenSearch code. The remaining type errors are pre-existing issues in the Redis store (unrelated to this PR). Commit: 22bbfa2 --- | |
Test Failure AnalysisSummary: The static analysis job for Root Cause: The PR added OpenSearch support to the async library (
Suggested Solution:
Detailed AnalysisError logs show: Sync library imports (incorrect): # key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py:29
from opensearchpy import AsyncOpenSearch # ❌ Wrong for sync libraryShould be: from opensearchpy import OpenSearch # ✅ Correct for sync libraryAsync library pyproject.toml (line 43): opensearch = ["opensearch-py[async]>=2.0.0"]Sync library pyproject.toml (missing): # Line 43 should be added:
opensearch = ["opensearch-py>=2.0.0"]Related Files
|
- Add opensearch-py dependency to sync library's pyproject.toml - Update codegen script to transform AsyncOpenSearch → OpenSearch - Regenerate sync code with correct synchronous OpenSearch client - Improve ImportError message for missing opensearch dependency This fixes the type checking errors in the sync library where the generated code was incorrectly using AsyncOpenSearch instead of the synchronous OpenSearch client. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: William Easton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (2)
35-37: Add type ignore comments for optional opensearchpy imports.Past review identified this issue: pyright fails on these optional imports. Please add
# type: ignore[reportMissingImports]to lines 36-37.Apply this diff:
- from opensearchpy import AsyncOpenSearch - from opensearchpy.exceptions import RequestError + from opensearchpy import AsyncOpenSearch # type: ignore[reportMissingImports] + from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports]
353-408: CRITICAL: Persist collection and key fields in documents.This is the same critical bug from the past review that remains unfixed. The
collectionandkeyfields are never written to the document body, causing all collection-level operations (_get_collection_keys,_get_collection_names,_delete_collection) to fail even when data exists.Apply this diff to fix both single and bulk write paths:
async def _put_managed_entry( self, *, key: str, collection: str, managed_entry: ManagedEntry, ) -> None: index_name: str = self._get_index_name(collection=collection) document_id: str = self._get_document_id(key=key) document: dict[str, Any] = self._serializer.dump_dict(entry=managed_entry) + document["collection"] = collection + document["key"] = key try: _ = await self._client.index( @@ index_action: dict[str, Any] = new_bulk_action(action="index", index=index_name, document_id=document_id) document: dict[str, Any] = self._serializer.dump_dict(entry=managed_entry) + document["collection"] = collection + document["key"] = key operations.extend([index_action, document])key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
40-40: Fix the dependency hint to reference the sync package.The ImportError message directs users to install
py-key-value-aio[opensearch], which is the async package. This sync store should referencepy-key-value[opensearch]orpy-key-value-sync[opensearch]instead.Apply this diff to correct the package reference:
- msg = "OpenSearchStore requires opensearch-py[async]>=2.0.0. Install with: pip install 'py-key-value-aio[opensearch]'" + msg = "OpenSearchStore requires opensearch-py>=2.0.0. Install with: pip install 'py-key-value-sync[opensearch]'"
📜 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 (6)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py(1 hunks)key-value/key-value-sync/pyproject.toml(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py(1 hunks)scripts/build_sync_library.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (3)
OpenSearchSerializationAdapter(67-89)OpenSearchV1CollectionSanitizationStrategy(97-104)OpenSearchV1KeySanitizationStrategy(92-94)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-132)load_dict(67-106)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-139)load_dict(67-106)dump_dict(114-132)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(35-45)get_body_from_response(7-22)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(25-32)install_serializer(131-133)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (5)
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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-139)load_dict(67-106)dump_dict(114-132)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(38-45)get_body_from_response(10-25)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(28-35)install_serializer(131-133)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (1)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (4)
get_body_from_response(7-22)LessCapableJsonSerializer(123-133)default(126-128)install_serializer(131-133)
🔇 Additional comments (15)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (10)
51-87: LGTM!Constants and default mapping are well-defined. The mapping correctly defines
collectionandkeyas keyword fields for exact matching, and uses flattened type for the value field.
89-114: LGTM!Serialization adapter correctly wraps/unwraps values in the flattened structure matching the OpenSearch mapping schema.
116-131: LGTM!Sanitization strategies are appropriately configured for OpenSearch constraints on keys and index names.
198-252: LGTM with minor redundancy note.Initialization logic is solid. Note:
index_prefixis lowercased on line 244, and_get_index_name(line 274) calls.lower()again. The second call is redundant but harmless.
254-284: LGTM!Setup and helper methods are correctly implemented. The race condition handling in
_setup_collection(lines 268-271) is appropriate.
286-351: LGTM!Get operations correctly handle single and batch retrievals. The batch operation properly maintains input order and logs deserialization errors.
410-456: LGTM!Delete operations correctly handle single and bulk deletions with appropriate result checking.
458-526: Collection enumeration depends on critical bug fix.These methods query and aggregate on the
collectionfield, which is currently not being persisted (see critical issue in lines 353-408). Once that bug is fixed, these implementations will work correctly.
528-549: Collection deletion depends on critical bug fix.This method uses delete_by_query on the
collectionfield, which is currently not being persisted (see critical issue in lines 353-408). Once that bug is fixed, this implementation will work correctly.
551-567: LGTM!Culling logic correctly deletes expired entries using millisecond epoch comparison. The
_closemethod properly closes the client.key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (1)
38-49: LGTM! The sync client issue has been resolved.The previous critical issue has been fixed. The code now correctly uses the synchronous
OpenSearchclient throughout, with proper synchronous context managers and method calls.key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (1)
1-133: LGTM! Sync client types are correct throughout.The utility module correctly uses the synchronous
OpenSearchclient type in all type hints and method signatures, including theinstall_serializermethod.key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
29-30: LGTM! Sync client usage is now correct.The previous critical issue has been resolved. The store now correctly uses the synchronous
OpenSearchclient throughout all type hints, constructor parameters, and instantiation. All method calls are synchronous without await statements.Also applies to: 121-121, 136-136, 175-175, 206-206, 211-211
key-value/key-value-sync/pyproject.toml (1)
70-70: LGTM!The addition of
opensearchto the dev extras list is consistent with other store dependencies and enables proper testing of OpenSearch functionality during development.scripts/build_sync_library.py (1)
222-222: LGTM!The mapping from
AsyncOpenSearchtoOpenSearchis correctly implemented and consistent with other client mappings in the code generation script, such as theAsyncElasticsearchtoElasticsearchmapping on Line 221.
| vault = ["hvac>=2.3.0", "types-hvac>=2.3.0"] | ||
| memcached = ["aiomcache>=0.8.0"] | ||
| elasticsearch = ["elasticsearch>=8.0.0", "aiohttp>=3.12"] | ||
| opensearch = ["opensearch-py[async]>=2.0.0"] |
There was a problem hiding this comment.
🧩 Analysis chain
Verify if the [async] extra is necessary for sync usage.
The sync package is specifying opensearch-py[async]>=2.0.0, which includes async dependencies. Please verify whether the sync OpenSearch client requires the [async] extra or if the base opensearch-py>=2.0.0 package is sufficient.
Run the following script to check the opensearch-py package structure and determine if the sync client is available without the [async] extra:
Additionally, search for the latest opensearch-py documentation to confirm the correct dependency specification for sync-only usage:
🏁 Script executed:
#!/bin/bash
# Description: Check opensearch-py package structure and availability of sync client
# Check if sync client is available in base package
python -c "from opensearch_py import OpenSearch; print('Sync client available in base package')" 2>&1 || echo "Sync client not in base package"
# Check package extras and dependencies
pip show opensearch-py 2>/dev/null || echo "Package not installed"
# Look for documentation on sync vs async client requirements
curl -s https://pypi.org/pypi/opensearch-py/json | jq -r '.info.project_urls | to_entries[] | "\(.key): \(.value)"'Length of output: 439
🌐 Web query:
opensearch-py sync client dependency requirements
💡 Result:
Summary — sync (blocking) opensearch-py client requirements
- Python: >=3.8 (client supports 3.8+). [1][2]
- Required runtime packages:
- requests >= 2.32.0, < 3.0.0. [2]
- urllib3 (version constraints vary by Python; client pins compatible ranges). [2]
- python-dateutil. [2]
- certifi (for TLS). [2]
- events (small event helper used by the client). [2]
- Optional extras:
- async support: aiohttp (>=3.9.4,<4). [2]
- kerberos auth: requests_kerberos (extra: [kerberos]). [2]
- Install commands:
- basic sync client: pip install opensearch-py.
- with async: pip install "opensearch-py[async]". [1][2]
Sources: OpenSearch Python client docs and package metadata. [1][2]
References
[1] OpenSearch docs — Low-level Python client. (docs.opensearch.org)
[2] opensearch-py installation/compatibility & package metadata (setup/install_requires). (deepwiki.com)
Citations:
- 1: https://docs.opensearch.org/docs/latest/clients/python-low-level/?utm_source=openai
- 2: https://deepwiki.com/opensearch-project/opensearch-py/1.1-installation-and-compatibility?utm_source=openai
Remove the [async] extra from the opensearch dependency.
The sync client requires only the base opensearch-py package, as the [async] extra is optional and only needed for async support. Change line 43 from:
opensearch = ["opensearch-py[async]>=2.0.0"]
to:
opensearch = ["opensearch-py>=2.0.0"]
Including the [async] extra unnecessarily pulls aiohttp as a transitive dependency into sync-only environments.
🤖 Prompt for AI Agents
In key-value/key-value-sync/pyproject.toml around line 43, the opensearch
dependency includes the unnecessary [async] extra which pulls aiohttp into
sync-only environments; update the dependency entry to remove the extra so it
reads the base package (opensearch-py>=2.0.0) instead of
opensearch-py[async]>=2.0.0, saving the transitive aiohttp dependency.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (2)
35-45: Silence optional dependency imports for pyright.
Please append# type: ignore[reportMissingImports](or the project’s standard variant) to bothAsyncOpenSearchandRequestErrorimports so our optional dependency doesn’t break static analysis.
362-404: Persistcollectionandkeyfields before indexing.
_get_collection_keys,_get_collection_names, and_delete_collectionrely on those fields existing in the stored document, but neither the single-index nor bulk path writes them. Every collection-level query will return empty results, and delete-by-query never matches anything. Please injectdocument["collection"] = collectionanddocument["key"] = keyin both paths before sending the documents to OpenSearch.key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
40-41: Fix the dependency hint for the sync store.
This ImportError message still points to the async extra (py-key-value-aio[opensearch]). For the synchronous package it should direct users to the sync extra (e.g.pip install "py-key-value[opensearch]").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py(1 hunks)key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
async_wait_for_true(7-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (6)
OpenSearchStore(133-566)OpenSearchSerializationAdapter(89-113)OpenSearchV1CollectionSanitizationStrategy(123-130)OpenSearchV1KeySanitizationStrategy(116-120)_get_index_name(272-273)_get_document_id(275-276)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-132)load_dict(67-106)
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(67-89)OpenSearchV1CollectionSanitizationStrategy(97-104)OpenSearchV1KeySanitizationStrategy(92-94)_get_index_name(241-242)_get_document_id(244-245)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-132)load_dict(67-106)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-139)load_dict(67-106)dump_dict(114-132)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(38-45)get_body_from_response(10-25)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(28-35)install_serializer(131-133)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-139)load_dict(67-106)dump_dict(114-132)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(35-45)get_body_from_response(7-22)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(25-32)install_serializer(131-133)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-shared)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-shared)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
| from opensearchpy import OpenSearch | ||
| from opensearchpy.exceptions import RequestError | ||
|
|
||
| from key_value.sync.code_gen.stores.opensearch.utils import ( | ||
| get_aggregations_from_body, | ||
| get_body_from_response, | ||
| get_first_value_from_field_in_hit, | ||
| get_hits_from_response, | ||
| get_source_from_body, | ||
| ) | ||
| except ImportError as e: | ||
| msg = "OpenSearchStore requires opensearch-py[async]>=2.0.0. Install with: pip install 'py-key-value-aio[opensearch]'" | ||
| raise ImportError(msg) from e |
There was a problem hiding this comment.
Silence optional dependency imports for pyright.
Please add # type: ignore[reportMissingImports] (or the project’s agreed variant) to the OpenSearch and RequestError imports so the optional dependency doesn’t fail type-checking.
🤖 Prompt for AI Agents
In
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
around lines 29 to 41, the optional imports of OpenSearch and RequestError can
cause pyright to report missing imports; silence these by adding the project's
agreed type-ignore comment to each import (e.g., append # type:
ignore[reportMissingImports]) so type-checking won’t fail for the optional
dependency, and keep the existing ImportError fallback unchanged.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
Outdated
Show resolved
Hide resolved
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py
Show resolved
Hide resolved
Test Failure AnalysisSummary: The Root Cause: The test snapshot expects an abbreviated field name Failure DetailsExpected: {'value': {'f': {'test': 'test'}}, 'created_at': '2025-01-01T00:00:00+00:00', 'expires_at': '2025-01-01T00:00:10+00:00'}Actual: {'version': 1, 'created_at': '2025-01-01T00:00:00+00:00', 'expires_at': '2025-01-01T00:00:10+00:00', 'value': {'flat': {'test': 'test'}}}Differences:
Suggested SolutionThe test snapshot was incorrectly modified in commit Option 1: Use "flat" (Recommended)Update the test snapshot to match what the adapter actually produces: File: Change: assert document == snapshot(
{
"value": {"f": {"test": "test"}}, # ❌ Wrong
"created_at": "2025-01-01T00:00:00+00:00",
"expires_at": "2025-01-01T00:00:10+00:00",
}
)To: assert document == snapshot(
{
"value": {"flat": {"test": "test"}}, # ✅ Matches adapter output
"created_at": "2025-01-01T00:00:00+00:00",
"expires_at": "2025-01-01T00:00:10+00:00",
}
)Note: The
Option 2: Use abbreviated "f"If abbreviation is intentional for storage efficiency, update the adapter: File: Change: data["value"] = {
"flat": value, # ❌
}To: data["value"] = {
"f": value, # ✅ Use abbreviated field name
}Also update the mapping in And update data["value"] = data.pop("value").get("f") # Change from "flat" to "f"Additional IssuesDocker Timeout Failures (Linux only)On Ubuntu, OpenSearch container fails to start within the 10-second pytest timeout: Why this happens: OpenSearch takes longer to initialize than the per-test timeout allows. Not a critical issue: These are infrastructure flakiness issues, not code bugs. The macOS and Windows runners don't have Docker, so they skip these tests and only fail on the snapshot assertion. Related Files
Next Steps
|
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(67-89)OpenSearchV1CollectionSanitizationStrategy(97-104)OpenSearchV1KeySanitizationStrategy(92-94)_get_index_name(241-242)_get_document_id(244-245)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
async_wait_for_true(7-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(89-113)OpenSearchV1CollectionSanitizationStrategy(123-130)OpenSearchV1KeySanitizationStrategy(116-120)_get_index_name(272-273)_get_document_id(275-276)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-shared)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
🔇 Additional comments (12)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (6)
1-40: LGTM: Well-structured test setup.The imports, constants, and test configuration are clear and appropriate for OpenSearch integration testing. The parametrized version testing across 2.11.0 and 2.18.0 ensures good coverage.
43-65: LGTM: Robust test utilities.The helper functions properly implement OpenSearch client management, health checking, and cleanup operations. The use of context managers and exception handling is appropriate for test infrastructure.
68-89: LGTM: Comprehensive serialization validation.The test properly validates the OpenSearchSerializationAdapter's dump/load round-trip, correctly asserting on the "flat" key structure and verifying that expired entries have negative TTL values.
92-150: LGTM: Well-designed fixture architecture.The test fixtures properly manage OpenSearch container lifecycle, client connections, and store instances. The parametrized version testing and dual store fixtures (with/without sanitization) enable comprehensive test coverage.
182-190: LGTM: Multi-index behavior validation.The test properly validates that multiple collections create separate indices with correct naming conventions. The snapshot assertion on index names ensures naming consistency.
192-216: LGTM: Storage format verification.The test correctly validates that values are stored using the "flat" object structure (as per the OpenSearchSerializationAdapter) rather than JSON strings, and properly checks both with and without TTL.
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (6)
1-40: LGTM: Imports and configuration are correct.The sync OpenSearch client imports and test configuration constants are properly set up for the generated sync test file.
43-65: LGTM: Utility functions correctly use sync OpenSearch client.The client creation, ping, and cleanup utilities properly use the synchronous OpenSearch client API with appropriate context management and error handling.
68-86: LGTM: Serialization test validates adapter behavior correctly.The test properly verifies the OpenSearchSerializationAdapter's dump/load cycle, including the correct "flat" nested structure for values and appropriate TTL semantics for expired entries.
88-139: LGTM: Test fixtures are properly configured.The Docker-based OpenSearch setup, client management, and store fixtures correctly instantiate the sync OpenSearch client and stores with appropriate configurations.
141-170: LGTM: Basic store tests and sanitization tests are correct.The protocol validation, skip decorators for distributed cache behavior, and long name tests properly verify both error and success paths with appropriate use of sanitization strategies.
172-180: LGTM: Multi-collection and special character tests are correct.The tests for multiple indices and special character handling properly verify the expected behavior and delegate appropriately to the sanitizing store.
Also applies to: 205-208
| index_names: list[str] = list(indices.keys()) | ||
| assert index_names == snapshot(["opensearch-kv-store-e2e-test-test_collection", "opensearch-kv-store-e2e-test-test_collection_2"]) | ||
|
|
||
| async def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: AsyncElasticsearch): |
There was a problem hiding this comment.
Fix incorrect type annotation.
The opensearch_client parameter is typed as AsyncElasticsearch but should be AsyncOpenSearch. This creates type confusion since the parameter is used to query OpenSearch indices, not Elasticsearch.
Apply this diff:
- async def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: AsyncElasticsearch):
+ async def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: AsyncOpenSearch):📝 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.
| async def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: AsyncElasticsearch): | |
| async def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: AsyncOpenSearch): |
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py around
line 192, the test parameter opensearch_client is incorrectly typed as
AsyncElasticsearch; change its type annotation to AsyncOpenSearch and update/add
the corresponding import from opensearchpy (or the package used in the project)
at the top of the file so the test function signature reads with AsyncOpenSearch
and the import is present; ensure any other references in the file use
AsyncOpenSearch for that fixture.
| index_names: list[str] = list(indices.keys()) | ||
| assert index_names == snapshot(["opensearch-kv-store-e2e-test-test_collection", "opensearch-kv-store-e2e-test-test_collection_2"]) | ||
|
|
||
| def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: Elasticsearch): |
There was a problem hiding this comment.
Fix incorrect type annotation: use OpenSearch instead of Elasticsearch.
The opensearch_client fixture returns an OpenSearch client (line 109), but this parameter is annotated as Elasticsearch. This type mismatch would cause type-checker errors.
Apply this diff:
- def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: Elasticsearch):
+ def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: OpenSearch):📝 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.
| def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: Elasticsearch): | |
| def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: OpenSearch): |
🤖 Prompt for AI Agents
In key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py
around line 182, the test parameter opensearch_client is incorrectly annotated
as Elasticsearch; change the annotation to OpenSearch to match the actual
fixture return type, and if OpenSearch is not yet imported add or update the
import to bring OpenSearch from opensearchpy so the type-checker resolves
correctly.
| assert index_names == snapshot(["opensearch-kv-store-e2e-test-test_collection", "opensearch-kv-store-e2e-test-test_collection_2"]) | ||
|
|
||
| def test_value_stored_as_f_object(self, store: OpenSearchStore, opensearch_client: Elasticsearch): | ||
| """Verify values are stored as f objects, not JSON strings""" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the docstring terminology.
The phrase "f objects" is unclear. Consider updating it to "nested flat objects" or "structured objects" to better describe the {"flat": {...}} wrapper that the OpenSearchSerializationAdapter uses.
Apply this diff:
- """Verify values are stored as f objects, not JSON strings"""
+ """Verify values are stored as nested flat objects, not JSON strings"""📝 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.
| """Verify values are stored as f objects, not JSON strings""" | |
| """Verify values are stored as nested flat objects, not JSON strings""" |
🤖 Prompt for AI Agents
In key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py
around line 183, the docstring "Verify values are stored as f objects, not JSON
strings" uses unclear terminology; update the docstring to explicitly say
something like "Verify values are stored as nested flat objects (e.g. {'flat':
{...}}) rather than JSON strings" or "structured objects" to accurately describe
the {"flat": {...}} wrapper used by OpenSearchSerializationAdapter.
|
@claude can you please address the PR feedback and fix merge conflicts? |
|
Claude finished @strawgate's task —— View job PR Feedback Addressed ✅All CodeRabbit feedback has been addressed and changes have been pushed. Task List
Summary of Changes1. Fixed Type Annotations
2. Added Document Metadata FieldsFollowing the pattern from PR #204, updated both write methods to include metadata:
This ensures stored documents include:
These fields enable collection-level operations and future schema migrations. 3. Updated Test SnapshotsUpdated all test expectations to include the new fields:
4. Fixed ImportError Message
5. Ran CodegenSuccessfully regenerated the entire sync library, which automatically applied all fixes to the sync version of the code. Verification
Note on Type Check ErrorsThe type checker reports errors about
These errors will not occur when the dependency is installed, and CI tests that actually use OpenSearch will install the dependency first. | |
- Fix type annotation: AsyncElasticsearch → AsyncOpenSearch in async test - Add key/collection/version fields to serialized documents (following PR #204) - Update test snapshots to expect version, key, and collection fields - Fix ImportError message in sync store to reference correct package - Run codegen to regenerate sync library with fixes Co-authored-by: William Easton <[email protected]>
|
@claude please fix merge conflicts |
|
Claude finished @strawgate's task —— View job Merge conflicts resolved ✅All merge conflicts have been successfully fixed and pushed to the branch. Task Summary
Conflict Resolution DetailsIssue: The main branch was rebased/force-pushed after this PR was created, causing unrelated git histories. This required using Changes from main that were integrated:
Changes from this branch that were preserved:
Actions taken:
Commit: Type Check NotesThe type checker shows 177 errors, mostly for optional dependencies (OpenSearch and Redis stores). These are expected and not actual issues:
| |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
29-38: Silence optional OpenSearch imports for type checkingLine 29: the sync build hits the same optional-dependency issue—pyright flags
opensearchpywhenever the extra isn’t installed. Add the per-import# type: ignore[reportMissingImports]so static analysis passes in minimal environments.- from opensearchpy import OpenSearch - from opensearchpy.exceptions import RequestError + from opensearchpy import OpenSearch # type: ignore[reportMissingImports] + from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports]key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (1)
36-45: Silence optional OpenSearch imports for type checkingLine 36: when
opensearch-pyisn’t installed, pyright still flags these imports even though we guard them withtry/except, so CI fails for users who haven’t enabled the optional extra. Please append the customary# type: ignore[reportMissingImports]to both lines so optional dependencies don’t break static analysis.- from opensearchpy import AsyncOpenSearch - from opensearchpy.exceptions import RequestError + from opensearchpy import AsyncOpenSearch # type: ignore[reportMissingImports] + from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py(1 hunks)key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(67-89)OpenSearchV1CollectionSanitizationStrategy(97-104)OpenSearchV1KeySanitizationStrategy(92-94)_get_index_name(241-242)_get_document_id(244-245)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
async_wait_for_true(7-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(89-113)OpenSearchV1CollectionSanitizationStrategy(123-130)OpenSearchV1KeySanitizationStrategy(116-120)_get_index_name(272-273)_get_document_id(275-276)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(35-45)get_body_from_response(7-22)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(25-32)install_serializer(131-133)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (8)
LessCapableJsonSerializer(123-133)new_bulk_action(119-120)get_aggregations_from_body(38-45)get_body_from_response(10-25)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(28-35)install_serializer(131-133)
|
|
||
| try: | ||
| _ = await self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType] | ||
| except Exception as e: | ||
| msg = f"Failed to serialize bulk operations: {e}" | ||
| raise SerializationError(message=msg) from e |
There was a problem hiding this comment.
Fail fast on bulk write errors
Line 401: bulk returns a body even when individual items fail (mapping rejection, circuit breaker, etc.), but we drop it on the floor. That means _put_managed_entries reports success while documents are silently lost. Please capture the response and raise SerializationError when errors is true so callers never proceed after a partial write.
- try:
- _ = await self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType]
+ try:
+ bulk_response = await self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType]
except Exception as e:
msg = f"Failed to serialize bulk operations: {e}"
raise SerializationError(message=msg) from e
+ if bulk_response.get("errors"):
+ first_error = next(
+ (
+ item
+ for item in bulk_response.get("items", [])
+ if item.get("index", {}).get("error")
+ ),
+ None,
+ )
+ msg = f"Bulk index response reported errors: {first_error or bulk_response}"
+ raise SerializationError(message=msg)🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py around
lines 401 to 406, the bulk response is discarded so individual item failures are
ignored; change the try block to capture the bulk response (e.g., response =
await self._client.bulk(...)), then after the call check if
response.get("errors") is truthy and if so raise SerializationError with a
message including the response (or the failed items) so callers won't treat
partial writes as success; keep the existing except to wrap RPC exceptions as
SerializationError.
| try: | ||
| _ = self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType] | ||
| except Exception as e: | ||
| msg = f"Failed to serialize bulk operations: {e}" | ||
| raise SerializationError(message=msg) from e |
There was a problem hiding this comment.
Fail fast on bulk write errors
Line 356: the synchronous bulk path has the same silent-failure gap—bulk() may report per-item errors, but we ignore the payload and proceed. That can drop writes without warning. Please inspect errors on the response and raise SerializationError when OpenSearch reports a failure.
- try:
- _ = self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType]
+ try:
+ bulk_response = self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType]
except Exception as e:
msg = f"Failed to serialize bulk operations: {e}"
raise SerializationError(message=msg) from e
+ if bulk_response.get("errors"):
+ first_error = next(
+ (
+ item
+ for item in bulk_response.get("items", [])
+ if item.get("index", {}).get("error")
+ ),
+ None,
+ )
+ msg = f"Bulk index response reported errors: {first_error or bulk_response}"
+ raise SerializationError(message=msg)📝 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.
| try: | |
| _ = self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType] | |
| except Exception as e: | |
| msg = f"Failed to serialize bulk operations: {e}" | |
| raise SerializationError(message=msg) from e | |
| try: | |
| bulk_response = self._client.bulk(body=operations, params={"refresh": "true"}) # type: ignore[reportUnknownVariableType] | |
| except Exception as e: | |
| msg = f"Failed to serialize bulk operations: {e}" | |
| raise SerializationError(message=msg) from e | |
| if bulk_response.get("errors"): | |
| first_error = next( | |
| ( | |
| item | |
| for item in bulk_response.get("items", []) | |
| if item.get("index", {}).get("error") | |
| ), | |
| None, | |
| ) | |
| msg = f"Bulk index response reported errors: {first_error or bulk_response}" | |
| raise SerializationError(message=msg) |
🤖 Prompt for AI Agents
In
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py
around lines 356 to 360, the synchronous bulk call ignores per-item errors in
the OpenSearch response and can silently drop writes; after calling
self._client.bulk(...) capture the response, check the response.get("errors")
flag (and the per-item response items) and if any errors are reported raise
SerializationError with a descriptive message that includes the response or
failed item details (preserve the original exception as the cause if bulk()
itself raises), so failures are detected and surfaced instead of being ignored.
Resolved merge conflicts by: - Integrating FileTreeStore additions from main - Integrating test-concise command additions from main - Preserving OpenSearch store support from this branch - Using main's simplified MongoDB store (removed native_storage parameter) - Regenerated uv.lock and sync library code - Updated build_sync_library.py with FileTreeStore exclusions and AsyncOpenSearch mapping Co-authored-by: William Easton <[email protected]>
Test Failure AnalysisSummary: The codegen check failed because running Root Cause: The When the async OpenSearch store has: msg = "OpenSearchStore requires opensearch-py[async]>=2.0.0. Install with: pip install 'py-key-value-aio[opensearch]'"The generated sync version incorrectly keeps Suggested Solution: Option 1 (Quick Fix - Recommended for this PR): Update the async source file to use a pattern the codegen can handle: Modify msg = "OpenSearchStore requires opensearch-py[async]>=2.0.0. Install with: pip install opensearch-py[async]"Then run Option 2 (Proper Fix - Separate PR): Enhance the codegen script to transform string literals containing package names. Add a string literal transformation rule in Detailed AnalysisFailed Job Log Excerpt:Why This Happens:The codegen script's Scope of the Issue:This affects 11 store implementations in the sync library, all with incorrect error messages pointing users to install
Related FilesPrimary file to fix (for Option 1):
File to enhance (for Option 2):
All affected sync files (auto-generated, do not edit directly):
|
|
@claude let's fix the codegen script please, let's also remove bulk handling for now |
|
Claude finished @strawgate's task —— View job Codegen script fixed and bulk handling removed ✅Task List
SummaryAll requested fixes have been completed and pushed to the branch. Changes Made
Files Changed
Verification
Commit: | |
…k operations - Add visit_Constant method to codegen script to transform string literals - Replace py-key-value-aio with py-key-value-sync in error messages - Remove [async] extras from opensearch-py package install instructions - Remove bulk write and delete operations from OpenSearch store - Remove _put_managed_entries method - Remove _delete_managed_entries method - Remove new_bulk_action import - Regenerate all sync code with corrected package references This fixes the codegen check failures where error messages incorrectly pointed users to install the async package instead of the sync package. Co-authored-by: William Easton <[email protected]>
Test Failure AnalysisSummary: The test suite is failing due to timeouts in OpenSearch Docker container setup across all platforms (Ubuntu 22.04, Ubuntu latest), and a pytest-xdist internal error on Windows related to RocksDB tests. Root Cause: 1. OpenSearch Test Timeouts (Primary Issue)All OpenSearch store tests are failing during the fixture setup phase when trying to pull and start the OpenSearch Docker container. The tests timeout after 10 seconds while waiting for the Docker image pull operation to complete. The stack traces show the tests are stuck in: The fixture attempts to pull Affected Tests:
2. Windows RocksDB Internal Error (Secondary Issue)A Suggested Solution: Fix #1: Increase Timeout for OpenSearch TestsThe OpenSearch test fixture has a Modify the test files:
Add a class-level timeout marker: @pytest.mark.timeout(300) # 5 minutes for fixture setup + tests
@pytest.mark.skipif(should_skip_docker_tests(), reason="Docker is not available")
@pytest.mark.filterwarnings("ignore:A configured store is unstable...")
class TestOpenSearchStore(ContextManagerStoreTestMixin, BaseStoreTests):
@pytest.fixture(autouse=True, scope="session", params=OPENSEARCH_VERSIONS_TO_TEST)
async def setup_opensearch(self, request: pytest.FixtureRequest) -> AsyncGenerator[None, None]:
# ... existing codeOR, more targeted, increase the wait timeout in the fixture (line ~108): # Change from:
if not await async_wait_for_true(bool_fn=ping_opensearch, tries=WAIT_FOR_OPENSEARCH_TIMEOUT, wait_time=2):
# To:
if not await async_wait_for_true(bool_fn=ping_opensearch, tries=60, wait_time=2): # 2 minutes totalWhy these changes will fix the issue: The timeout needs to account for Docker image pull time (which can be 30-60+ seconds in CI) plus container startup time. The current 10-second timeout is insufficient for this operation. Fix #2: Address Windows pytest-xdist Error (Optional)The Windows error appears to be a pytest-xdist race condition that's not directly related to the OpenSearch changes. This is a pre-existing issue or an intermittent flaky test infrastructure problem. Options:
Detailed AnalysisOpenSearch Test Failure LogsError Pattern (repeated across all test platforms): Stack Trace Shows: tests/code_gen/conftest.py:156: in docker_container
docker_pull(image=image, raise_on_error=True)
tests/code_gen/conftest.py:81: in docker_pull
client.images.pull(image)
../../.venv/lib/python3.10/site-packages/urllib3/response.py:980: in read
data = self._raw_read(amt)The fixture is stuck reading data from the Docker daemon while pulling the OpenSearch image. Windows Internal ErrorFull Error: This occurs in pytest-xdist's work scheduling logic and is not directly caused by the PR changes. It's a timing/concurrency issue in the test infrastructure. Related FilesFiles Requiring Changes:
Files Analyzed:
|
|
@claude please fix merge conflicts! |
|
Claude Code is working… I'll analyze this and get back to you. |
There was a problem hiding this comment.
Actionable comments posted: 4
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/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
1-3: Update the source template instead of editing the generated file.The source template at
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(line 48) still contains the old message withpy-key-value-aio[elasticsearch]. While the codegen script has transformation logic to replace this automatically (lines 413-416), the source template should be the single source of truth and must be updated. Change line 48 in the source template from:msg = "ElasticsearchStore requires py-key-value-aio[elasticsearch]"to:
msg = "ElasticsearchStore requires py-key-value-sync[elasticsearch]"Then regenerate the sync variant to ensure consistency. The manual edit to the generated file violates the auto-generated file warning at the top of the file.
♻️ Duplicate comments (2)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (1)
28-29: Add type-ignore comments for optional dependency imports.The optional
opensearchpyimports trigger pyright'sreportMissingImportserror, causing CI failures. Add inline type-ignore comments to silence the analyzer for these external dependencies.Apply this diff:
- from opensearchpy import OpenSearch - from opensearchpy.exceptions import RequestError + from opensearchpy import OpenSearch # type: ignore[reportMissingImports] + from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports]Based on past review comments.
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (1)
35-36: Add type-ignore comments for optional dependency imports.The optional
opensearchpyimports trigger pyright'sreportMissingImportserror, causing CI failures. Add inline type-ignore comments to silence the analyzer for these external dependencies.Apply this diff:
- from opensearchpy import AsyncOpenSearch - from opensearchpy.exceptions import RequestError + from opensearchpy import AsyncOpenSearch # type: ignore[reportMissingImports] + from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports]Based on past review comments.
📜 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 (19)
Makefile(0 hunks)key-value/key-value-aio/pyproject.toml(2 hunks)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py(1 hunks)key-value/key-value-aio/tests/stores/base.py(1 hunks)key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/keyring/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/vault/store.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/base.py(1 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py(1 hunks)scripts/build_sync_library.py(2 hunks)
💤 Files with no reviewable changes (1)
- Makefile
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-aio/src/key_value/aio/stores/opensearch/utils.py (7)
LessCapableJsonSerializer(123-133)get_aggregations_from_body(35-45)get_body_from_response(7-22)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(25-32)install_serializer(131-133)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (5)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
async_wait_for_true(7-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(88-112)OpenSearchV1CollectionSanitizationStrategy(122-129)OpenSearchV1KeySanitizationStrategy(115-119)_get_index_name(271-272)_get_document_id(274-275)key-value/key-value-aio/tests/stores/base.py (2)
store(31-31)ContextManagerStoreTestMixin(266-278)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(66-88)OpenSearchV1CollectionSanitizationStrategy(96-103)OpenSearchV1KeySanitizationStrategy(91-93)_get_index_name(240-241)_get_document_id(243-244)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (6)
key-value/key-value-shared/src/key_value/shared/errors/key_value.py (2)
DeserializationError(14-15)SerializationError(10-11)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/sanitization.py (3)
AlwaysHashStrategy(104-128)HybridSanitizationStrategy(182-293)SanitizationStrategy(33-78)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (3)
SerializationAdapter(38-184)load_dict(67-106)dump_dict(114-158)key-value/key-value-shared/src/key_value/shared/utils/time_to_live.py (1)
now_as_epoch(14-16)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/utils.py (7)
LessCapableJsonSerializer(123-133)get_aggregations_from_body(38-45)get_body_from_response(10-25)get_first_value_from_field_in_hit(111-116)get_hits_from_response(48-67)get_source_from_body(28-35)install_serializer(131-133)
🔇 Additional comments (22)
key-value/key-value-sync/src/key_value/sync/code_gen/stores/vault/store.py (1)
19-19: LGTM! Corrects the ImportError message.The error message now correctly references
py-key-value-sync[vault]instead of the previously incorrectpy-key-value-aio[vault], ensuring users receive accurate installation instructions for the sync variant.key-value/key-value-sync/src/key_value/sync/code_gen/stores/memory/store.py (1)
24-24: LGTM! Corrects the ImportError message.The error message now correctly references
py-key-value-sync[memory]instead of the previously incorrectpy-key-value-aio[memory], ensuring users receive accurate installation instructions for the sync variant.key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/store.py (1)
17-17: LGTM! Corrects the ImportError message.The error message now correctly references
py-key-value-sync[disk]instead of the previously incorrectpy-key-value-aio[disk], ensuring users receive accurate installation instructions for the sync variant.key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (1)
41-42: ImportError message correctly updated for sync variant.The message now accurately reflects the sync library dependency
py-key-value-sync[elasticsearch]instead of the async variant. This aligns with the sync codebase's naming convention and will help users install the correct extras package.key-value/key-value-sync/src/key_value/sync/code_gen/stores/mongodb/store.py (1)
1-24: The change was generated by the codegen script, not manually edited—no action needed.The
build_sync_library.pyscript explicitly handles this transformation. ItsRenameAsyncToSync.visit_Constant()method automatically replaces"py-key-value-aio"with"py-key-value-sync"in all string constants during AST conversion. This is exactly why line 24 in the generated file shows"py-key-value-sync[mongodb]"while the source async file (line 21) contains"py-key-value-aio[mongodb]". The 3-line difference accounts for the auto-generated header comment prepended to the output file.This is the correct functioning of the code generation system—no violation of the auto-generation contract.
Likely an incorrect or invalid review comment.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/rocksdb/store.py (1)
19-19: Correct fix for package reference in error message.The ImportError message now correctly references
py-key-value-sync[rocksdb]instead of the async variant. Verification confirms the package name matches the actual configuration inkey-value/key-value-sync/pyproject.toml, where the rocksdb optional dependency is properly defined. This provides accurate installation instructions to users.key-value/key-value-sync/src/key_value/sync/code_gen/stores/redis/store.py (1)
21-21: No action needed—the fix is sustainable and correctly positioned.The sync file's package name correction is the intended result of the codegen process. The
build_sync_library.pyscript already contains the transformation logic to replacepy-key-value-aiowithpy-key-value-syncin string literals, so this fix will be preserved on future codegen runs and doesn't require changes to the async source file.key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py (1)
1-3: Verification passed: Changes were properly generated by the codegen script.The generated sync file contains the correct transformed package name
py-key-value-sync[disk]on line 19. Thebuild_sync_library.pyscript explicitly handles this transformation through AST visitor logic (lines 415-416) that replaces all occurrences ofpy-key-value-aiowithpy-key-value-syncin string literals. The file header confirms proper auto-generation, and this was not a manual edit.key-value/key-value-sync/tests/code_gen/stores/base.py (1)
30-33: Timeout bump makes senseAllowing 90 s gives the OpenSearch container and initial indexing room to settle, preventing spurious flakes under heavier integration load.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/valkey/store.py (1)
18-19: Accurate install hintPointing to
py-key-value-sync[valkey]aligns the guidance with the sync package extras, reducing support noise.key-value/key-value-aio/pyproject.toml (1)
44-73: Dependency wiring looks goodAdding
opensearch-py[async]>=2.0.0under the newopensearchextra and pulling it into the dev group ensures aio tests can exercise the OpenSearch store without manual steps.scripts/build_sync_library.py (1)
224-248: Nice coverage for OpenSearch renamesExtending the rename table and normalizing literal strings keeps the generated sync tree free of aio-specific identifiers and
[async]extras—exactly what we need for the new store.Also applies to: 412-421
key-value/key-value-aio/tests/stores/base.py (1)
33-33: LGTM!The timeout increase from 60 to 90 seconds is reasonable given the addition of OpenSearch integration tests that require docker container startup and initialization.
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (3)
66-89: LGTM!The serialization adapter test comprehensively validates round-trip conversion, TTL semantics, timestamp handling, and the nested "flat" value structure. The test correctly asserts the expired TTL becomes negative after the expiration datetime has passed.
94-114: LGTM!The docker-based OpenSearch setup with health checks and multi-version testing (2.11.0 and 2.18.0) provides robust coverage. The startup wait logic with
async_wait_for_trueensures containers are ready before tests run, and the custom exception for startup failures is appropriate.
175-184: LGTM!The multi-index test validates that different collections create separate indices and correctly retrieves data from each. The snapshot assertion on index names ensures the naming convention remains stable.
key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (6)
88-113: LGTM!The
OpenSearchSerializationAdaptercorrectly implements the prepare_dump/prepare_load hooks to transform values into the nested{"flat": ...}structure required by OpenSearch's flat_object type. The date format (isoformat) and value format (dict) choices align with the DEFAULT_MAPPING.
115-130: LGTM!The sanitization strategies are well-designed:
OpenSearchV1KeySanitizationStrategyuses consistent 64-character hashing for keys, ensuring fixed-length document IDs.OpenSearchV1CollectionSanitizationStrategyapplies hybrid sanitization with character replacement, length limits, and mandatory hash fragments to ensure safe index names.
197-250: LGTM!The constructor properly supports both direct client injection and URL-based initialization. The LessCapableJsonSerializer installation prevents datetime/float auto-serialization issues. The index_prefix is correctly lowercased to align with OpenSearch index naming conventions.
350-373: LGTM!The
_put_managed_entrymethod correctly persists the collection and key fields by passing them as parameters todump_dict, ensuring they're included in the indexed document for collection-level queries. Error handling appropriately wraps exceptions asSerializationError.
392-426: LGTM!The
_get_collection_keysmethod correctly queries by collection term and extracts keys from the fields array. The limit enforcement and safe extraction viaget_first_value_from_field_in_hitprevent malformed results from breaking enumeration.
485-498: LGTM!The
_cullmethod correctly converts epoch seconds to milliseconds for OpenSearch's date field comparison. The contextlib.suppress ensures cull failures don't break the store, which is appropriate for background maintenance operations.
| with pytest.raises(Exception): # noqa: B017, PT011 | ||
| await store.put(collection="test_collection" * 100, key="test_key", value={"test": "test"}) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider more specific exception types in test assertions.
The tests use pytest.raises(Exception) which is overly broad. Consider catching more specific exception types (e.g., ValueError, RequestError) to make the tests more precise and self-documenting about what failures are expected when collection or key names exceed limits.
Example for line 160:
- with pytest.raises(Exception): # noqa: B017, PT011
+ with pytest.raises(ValueError): # Or the specific OpenSearch exception
await store.put(collection="test_collection" * 100, key="test_key", value={"test": "test"})This would require verifying which specific exception the store raises for invalid collection/key names.
Also applies to: 169-170
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py around
lines 160-161 (and similarly for 169-170), the tests currently use
pytest.raises(Exception) which is too broad; update each assertion to catch the
specific exception the store actually raises for invalid/overlong collection or
key names (for example ValueError, RequestError, or a custom store exception) by
running the failing case once to observe the exact exception type and then
replace Exception with that specific exception type (or parametrize tests to
assert each expected error type) so the tests are precise and self-documenting.
| from pathvalidate import sanitize_filename | ||
| except ImportError as e: | ||
| msg = "DiskStore requires py-key-value-aio[disk]" | ||
| msg = "DiskStore requires py-key-value-sync[disk]" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM! Package reference correction is accurate.
The change correctly updates the error message to reference py-key-value-sync[disk] for this sync variant of the store, replacing the incorrect py-key-value-aio[disk] reference.
Minor pre-existing inconsistency (optional): The error message mentions "DiskStore" but the class is named "MultiDiskStore". This doesn't affect functionality but could be clarified for better error messaging.
🤖 Prompt for AI Agents
In
key-value/key-value-sync/src/key_value/sync/code_gen/stores/disk/multi_store.py
around line 19, the error message currently reads "DiskStore requires
py-key-value-sync[disk]" which is inconsistent with the class name
MultiDiskStore; update the message to reference "MultiDiskStore requires
py-key-value-sync[disk]" (or a neutral "this store" wording) so the error
matches the class name and provides clearer context.
| from keyring.errors import PasswordDeleteError | ||
| except ImportError as e: | ||
| msg = "KeyringStore requires py-key-value-aio[keyring]" | ||
| msg = "KeyringStore requires py-key-value-sync[keyring]" |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Correct package reference for sync variant.
The error message now correctly references py-key-value-sync[keyring] instead of py-key-value-aio[keyring], which is appropriate for this sync code path.
Since this is auto-generated code (as noted in lines 1-3), please ensure the corresponding fix has been applied to the original source file to prevent regression during the next code generation run.
🏁 Script executed:
#!/bin/bash
# Verify the original source file also has the correct package reference
# Find the original async source file
fd -t f "store.py" key-value-aio | grep "stores/keyring/store.py" | head -1 | xargs cat | grep -n "KeyringStore requires"Length of output: 100
🏁 Script executed:
#!/bin/bash
# Explore repo structure and find all store.py files in keyring paths
echo "=== Repository structure (first 20 dirs) ==="
ls -la | head -20
echo -e "\n=== Finding all store.py files in keyring paths ==="
fd -t f "store.py" | grep -i keyring
echo -e "\n=== Checking if key-value-aio directory exists ==="
ls -d key-value-aio 2>/dev/null || echo "key-value-aio directory not found"
echo -e "\n=== Finding all store.py files in the repo ==="
fd -t f "store.py"Length of output: 3959
🏁 Script executed:
#!/bin/bash
# Check line 21 in the original async keyring store.py file
echo "=== Async store.py (keyring) - lines 18-24 ==="
sed -n '18,24p' key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py
echo -e "\n=== Sync store.py (keyring) - lines 18-24 (for comparison) ==="
sed -n '18,24p' key-value/key-value-sync/src/key_value/sync/code_gen/stores/keyring/store.py
echo -e "\n=== Async store.py - first 10 lines (to check file structure) ==="
head -10 key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py
echo -e "\n=== Sync store.py - first 10 lines ==="
head -10 key-value/key-value-sync/src/key_value/sync/code_gen/stores/keyring/store.pyLength of output: 1472
Source file not updated—fix will be lost on next code generation.
The async source file (key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py, line 18) still contains the old reference: "py-key-value-aio[keyring]". Only the generated sync file was updated. Since the sync file is auto-generated from the async source (per its header), the next code generation run will revert this change.
Update line 18 in key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py to reference py-key-value-aio[keyring] (which is correct for async), ensuring the generated sync file maintains py-key-value-sync[keyring].
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/keyring/store.py around line
18, the async source still has the old/incorrect package reference; update that
line to use the correct async package marker "py-key-value-aio[keyring]" (so the
generated sync file will continue to show "py-key-value-sync[keyring]" after
code generation).
| @pytest.fixture | ||
| def store(self, opensearch_client: OpenSearch) -> Generator[BaseStore, None, None]: | ||
| store = OpenSearchStore( | ||
| opensearch_client=opensearch_client, index_prefix="opensearch-kv-store-e2e-test", default_collection="test-collection" | ||
| ) | ||
|
|
||
| with store: | ||
| yield store | ||
|
|
There was a problem hiding this comment.
Don’t enter the store fixture twice
ContextManagerStoreTestMixin.enter_exit_store already exercises both with store: and manual close() paths. Wrapping the store fixture itself in with store: calls __enter__ twice for the context-managed run and still leaves the “manual close” run inside a context manager, so we never validate the non-context flow and risk double-closing the client. Let the mixin own lifecycle management:
@override
@pytest.fixture
def store(self, opensearch_client: OpenSearch) -> Generator[BaseStore, None, None]:
- store = OpenSearchStore(
- opensearch_client=opensearch_client, index_prefix="opensearch-kv-store-e2e-test", default_collection="test-collection"
- )
-
- with store:
- yield store
+ store = OpenSearchStore(
+ opensearch_client=opensearch_client,
+ index_prefix="opensearch-kv-store-e2e-test",
+ default_collection="test-collection",
+ )
+ yield storeThat keeps the autouse mixin meaningful and avoids double-enter/double-close behaviour.
🤖 Prompt for AI Agents
In key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py
around lines 121 to 129, the store fixture is incorrectly entering the context
manager itself (using "with store: yield store"), causing the mixin's
enter_exit_store to call __enter__ twice and preventing testing of the
manual-close path; change the fixture to construct and yield the OpenSearchStore
instance without wrapping it in "with", so the ContextManagerStoreTestMixin can
manage entering and closing the store lifecycle itself.
|
@claude please resolve merge conflicts between thsi branch and main |
|
Claude finished @strawgate's task —— View job Merge conflicts resolved ✅All merge conflicts between this branch and main have been successfully resolved and pushed. SummaryIssue: The main branch was rebased or force-pushed after this PR was created, causing unrelated git histories that required using Changes merged:
Conflict Resolution DetailsFiles Resolved (18 total):
Post-Resolution Steps:
Commit: |
Resolved merge conflicts by: - Merging opensearch (this branch) and duckdb (main) dependencies - Adding filetree store from main - Keeping 90s test timeout from this branch - Preserving correct py-key-value-sync package references - Keeping AsyncOpenSearch mapping and visit_Constant method in codegen - Regenerating uv.lock and running codegen Co-authored-by: William Easton <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
key-value/key-value-sync/pyproject.toml (1)
44-44:⚠️ Unresolved: Remove[async]extra from sync package OpenSearch dependency.This issue was previously flagged as critical in past reviews. The sync variant (
py-key-value-sync) should not include the[async]extra, which pulls unnecessary async dependencies (aiohttp) into sync-only environments. The baseopensearch-py>=2.0.0package is sufficient for sync client usage.Apply the recommended fix from the past review:
-opensearch = ["opensearch-py[async]>=2.0.0"] +opensearch = ["opensearch-py>=2.0.0"]
📜 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 (3)
key-value/key-value-aio/pyproject.toml(2 hunks)key-value/key-value-sync/pyproject.toml(2 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
🔇 Additional comments (3)
key-value/key-value-sync/pyproject.toml (1)
72-72: Dev dependency group correctly updated.The opensearch extra is properly included in the dev dependency group for comprehensive testing coverage.
key-value/key-value-aio/pyproject.toml (2)
44-44: ✅ Correct OpenSearch dependency for async variant.The
opensearch-py[async]>=2.0.0specification is appropriate for the aio package. The[async]extra correctly includes the async dependencies (aiohttp) needed for async client operations.
73-73: Dev dependency group correctly updated.The opensearch extra is properly included in the aio dev dependency group for comprehensive testing coverage.
| import duckdb | ||
| except ImportError as e: | ||
| msg = "DuckDBStore requires the duckdb extra from py-key-value-aio or py-key-value-sync" | ||
| msg = "DuckDBStore requires the duckdb extra from py-key-value-sync or py-key-value-sync" |
There was a problem hiding this comment.
Fix duplicate text in ImportError message.
The error message contains redundant text: "py-key-value-sync or py-key-value-sync". This appears to be a copy-paste error that will confuse users trying to resolve the missing dependency.
Based on the context and typical patterns, the message should likely specify the correct extra, such as:
- msg = "DuckDBStore requires the duckdb extra from py-key-value-sync or py-key-value-sync"
+ msg = "DuckDBStore requires the duckdb extra. Install with: pip install py-key-value-sync[disk]"Note: Since this file is auto-generated (line 1), you'll need to fix the source file in the async variant and regenerate, or update the code generation script if the duplication was introduced during the async-to-sync conversion process.
📝 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.
| msg = "DuckDBStore requires the duckdb extra from py-key-value-sync or py-key-value-sync" | |
| msg = "DuckDBStore requires the duckdb extra. Install with: pip install py-key-value-sync[disk]" |
🤖 Prompt for AI Agents
In key-value/key-value-sync/src/key_value/sync/code_gen/stores/duckdb/store.py
around line 19, the ImportError message repeats "py-key-value-sync or
py-key-value-sync"; remove the duplicate and update the message to reference the
correct extra once (for example use "py-key-value-sync[duckdb]" or
"py-key-value-sync" as appropriate). Fix the original source (the async variant
or the codegen template) so the regenerated sync file doesn't reintroduce the
duplication, then regenerate the file.
Test Failure AnalysisSummary: Elasticsearch tests are timing out due to a resource cleanup conflict between test fixtures. The Root Cause: The issue is in
This creates a race condition where:
The errors show: Suggested Solution: The Option 1: Remove the es_client fixture entirely (Recommended)Modify # Remove the es_client fixture entirely (lines 102-108)
@override
@pytest.fixture
async def store(self) -> AsyncGenerator[ElasticsearchStore, None]:
store = ElasticsearchStore(url=ES_URL, index_prefix="kv-store-e2e-test")
async with store:
yield store
@pytest.fixture
async def sanitizing_store(self) -> AsyncGenerator[ElasticsearchStore, None]:
store = ElasticsearchStore(
url=ES_URL,
index_prefix="kv-store-e2e-test",
key_sanitization_strategy=ElasticsearchV1KeySanitizationStrategy(),
collection_sanitization_strategy=ElasticsearchV1CollectionSanitizationStrategy(),
)
async with store:
yield store
@pytest.fixture(autouse=True)
async def cleanup_indices(self, store: ElasticsearchStore):
# Use the store's internal client for cleanup
async with AsyncElasticsearch(hosts=[ES_URL]) as cleanup_client:
await cleanup_elasticsearch_indices(elasticsearch_client=cleanup_client)
yield
async with AsyncElasticsearch(hosts=[ES_URL]) as cleanup_client:
await cleanup_elasticsearch_indices(elasticsearch_client=cleanup_client)Option 2: Make fixtures share a single client properlyIf sharing a client is desired, the @pytest.fixture
async def es_client(self) -> AsyncGenerator[AsyncElasticsearch, None]:
async with AsyncElasticsearch(hosts=[ES_URL]) as client:
yield client
# No explicit close needed - context manager handles it
@override
@pytest.fixture
async def store(self, es_client: AsyncElasticsearch) -> AsyncGenerator[ElasticsearchStore, None]:
store = ElasticsearchStore(
elasticsearch_client=es_client, # Share the client
index_prefix="kv-store-e2e-test"
)
async with store:
yield storeWhy Option 1 is better: Each store should manage its own client lifecycle. This matches the pattern used in other store tests and avoids complex fixture dependencies. Detailed AnalysisError Timeline from Logs:
This "already exists" error suggests tests are interfering with each other due to improper cleanup sequencing. Related Code Locations:
Related Files
|
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (2)
159-164: Overly broad exception type in test assertion.This continues to use
pytest.raises(Exception)which is too broad. The previous review recommended catching the specific exception type raised for invalid collection names (e.g.,ValueErroror an OpenSearch-specific exception). This makes tests more precise and self-documenting about expected failure modes.
167-173: Overly broad exception type in test assertion.This continues to use
pytest.raises(Exception)which is too broad. The previous review recommended catching the specific exception type raised for invalid key names. Consider determining the actual exception type and using it here for more precise test assertions.key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (1)
123-142: Let the mixin manage the store lifecycleReintroducing
with store:here meansContextManagerStoreTestMixinnow calls__enter__twice and never exercises the manualclose()path it is designed to validate. It also risks double-closing the underlying OpenSearch client. Please yield the store unchecked so the mixin can own lifecycle management, as previously requested.store = OpenSearchStore( opensearch_client=opensearch_client, index_prefix="opensearch-kv-store-e2e-test", default_collection="test-collection" ) - with store: - yield store + yield store @@ ) - with store: - yield store + yield store
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py(1 hunks)key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py(4 hunks)key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py(1 hunks)key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py(2 hunks)key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py(4 hunks)key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (1)
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (4)
cleanup_elasticsearch(118-122)get_elasticsearch_client(37-38)cleanup_elasticsearch_indices(48-51)store(105-106)
key-value/key-value-sync/tests/code_gen/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
wait_for_true(18-26)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-sync/src/key_value/sync/code_gen/stores/opensearch/store.py (3)
OpenSearchSerializationAdapter(66-88)_get_index_name(240-241)_get_document_id(243-244)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (4)
key-value/key-value-shared/src/key_value/shared/stores/wait.py (1)
async_wait_for_true(7-15)key-value/key-value-shared/src/key_value/shared/utils/managed_entry.py (1)
ManagedEntry(15-63)key-value/key-value-aio/src/key_value/aio/stores/opensearch/store.py (5)
OpenSearchSerializationAdapter(88-112)OpenSearchV1CollectionSanitizationStrategy(122-129)OpenSearchV1KeySanitizationStrategy(115-119)_get_index_name(271-272)_get_document_id(274-275)key-value/key-value-shared/src/key_value/shared/utils/serialization.py (2)
dump_dict(114-158)load_dict(67-106)
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (1)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (6)
cleanup_elasticsearch(117-121)get_elasticsearch_client(36-37)cleanup_elasticsearch_indices(47-50)test_put_put_two_indices(148-158)store(104-105)test_value_stored_as_flattened_object(160-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-2022, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, windows-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, macos-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-latest, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, macos-14, key-value/key-value-aio)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-sync)
- GitHub Check: test_quick (3.10, ubuntu-22.04, key-value/key-value-aio)
🔇 Additional comments (11)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (3)
117-121: LGTM!The refactor to use a context-managed client instead of a fixture is correct and aligns with the sync version pattern. The cleanup logic before and after the yield is preserved correctly.
148-158: LGTM!The test correctly removes the
es_clientfixture dependency and creates a context-managed client inline for verification. The test logic and assertions are preserved accurately.
160-194: LGTM!The test correctly refactors to use a context-managed client instead of the fixture. All verification logic, including the TTL assertions, is preserved accurately.
key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py (2)
245-246: LGTM: Idempotent resource handling is appropriate.The addition of
resource_already_exists_exceptionhandling mirrors the existing pattern forindex_already_exists_exceptionand correctly treats both as non-fatal conditions during collection setup. This ensures idempotent behavior regardless of which exception type the backend returns.The case-insensitive string matching is consistent with the existing approach on line 243.
1-3: Auto-generated file was properly regenerated from async source.Verification confirms this sync file was correctly regenerated from the async source: the async source file at
key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.pycontains the correspondingresource_already_exists_exceptionhandling at lines 273-274. The changes are not manual edits and follow the established pattern for idempotent error handling. No further action needed.key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py (3)
66-89: LGTM! Comprehensive serialization test.The serialization round-trip test correctly validates the OpenSearchSerializationAdapter behavior, including the "flat" field structure, timestamp handling, and TTL calculation. The previous review concern about snapshot expectations has been properly addressed.
91-149: Well-structured test fixtures with proper scoping.The fixture hierarchy correctly uses session scope for the expensive Docker container setup while maintaining function scope for clients and stores to ensure test isolation. The parameterization across OpenSearch versions provides good coverage, and async context managers ensure proper cleanup.
175-220: Excellent integration test coverage.These tests provide comprehensive validation of OpenSearch store behavior:
- Multi-collection index isolation and naming conventions
- Internal document structure with the "flat" value format
- TTL/expiration metadata handling
- Special character sanitization
The use of snapshot assertions makes the tests maintainable, and the direct OpenSearch client queries verify the actual storage format. The previous review concern about type annotations has been properly addressed with
AsyncOpenSearch.key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (3)
118-122: LGTM!The fixture correctly uses the context manager to acquire the Elasticsearch client and performs cleanup before and after each test. The sync conversion from the async version is accurate.
149-159: LGTM!The test correctly uses the context manager to acquire an Elasticsearch client for verification. The refactoring properly removes the es_client fixture parameter dependency.
161-195: LGTM!The test correctly uses the context manager to acquire an Elasticsearch client and thoroughly verifies that values are stored as flattened objects with proper TTL handling. The refactoring is sound.
| except BadRequestError as e: | ||
| if "index_already_exists_exception" in str(e).lower(): | ||
| return | ||
| if "resource_already_exists_exception" in str(e).lower(): | ||
| return | ||
| raise |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding debug logging for swallowed exceptions.
The exception handling correctly implements idempotent collection setup, but silently swallows exceptions that may be useful for debugging race conditions or understanding cluster behavior.
Consider adding a debug log when these exceptions are caught:
try:
_ = await self._client.options(ignore_status=404).indices.create(index=index_name, mappings=DEFAULT_MAPPING, settings={})
except BadRequestError as e:
error_message = str(e).lower()
if "index_already_exists_exception" in error_message or "resource_already_exists_exception" in error_message:
+ logger.debug(
+ "Index already exists during collection setup",
+ extra={"collection": collection, "index_name": index_name, "exception_type": type(e).__name__}
+ )
return
raiseCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py
around lines 270 to 275, the handler for BadRequestError silently returns for
index_already_exists_exception and resource_already_exists_exception; add a
debug-level log when these exceptions are detected that includes the exception
message, the index/collection name (if available from scope), and any contextual
info (e.g., cluster/host) so race conditions are traceable; ensure the logger
used respects module logging conventions and do not change the idempotent
behavior (still return after logging).
| if "resource_already_exists_exception" in str(e).lower(): | ||
| return |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider combining the duplicate exception checks.
Lines 271-274 perform identical actions for two different exception messages. This can be refactored for clarity and maintainability.
Apply this diff to combine the checks:
try:
_ = await self._client.options(ignore_status=404).indices.create(index=index_name, mappings=DEFAULT_MAPPING, settings={})
except BadRequestError as e:
- if "index_already_exists_exception" in str(e).lower():
- return
- if "resource_already_exists_exception" in str(e).lower():
+ error_message = str(e).lower()
+ if "index_already_exists_exception" in error_message or "resource_already_exists_exception" in error_message:
return
raise🤖 Prompt for AI Agents
In key-value/key-value-aio/src/key_value/aio/stores/elasticsearch/store.py
around lines 271 to 274, two consecutive exception checks perform the identical
action (return) for different substrings of the exception message; combine them
into a single check that tests for both substrings (e.g., using any(...) over a
list/tuple of target substrings or an OR condition) so the duplicate branches
are merged and return once when either message is present.
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py
Outdated
Show resolved
Hide resolved
| OPENSEARCH_URL = f"http://{LOCALHOST}:{HOST_PORT}" | ||
|
|
||
|
|
||
| WAIT_FOR_OPENSEARCH_TIMEOUT = 30 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify the timeout constant's unit of measure.
WAIT_FOR_OPENSEARCH_TIMEOUT is named to suggest seconds but is used as a tries count at line 109 with a 2-second wait per try, resulting in a 60-second total timeout rather than 30 seconds. Consider renaming to WAIT_FOR_OPENSEARCH_TRIES or updating the value to 15 if 30 seconds is the intended total timeout.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py around
line 33, the constant WAIT_FOR_OPENSEARCH_TIMEOUT is misleading because it's
used as a number of tries at line 109 with a 2-second sleep per try (30 tries ×
2s = 60s), so either rename it to WAIT_FOR_OPENSEARCH_TRIES to reflect its role
as a retry count and update its usage sites and tests accordingly, or change its
numeric value to 15 if the intended total wait is 30 seconds (15 tries × 2s),
and update any references/comments to match the chosen meaning.
| async def cleanup_opensearch_indices(opensearch_client: AsyncOpenSearch): | ||
| with contextlib.suppress(Exception): | ||
| indices = await opensearch_client.indices.get(index="opensearch-kv-store-e2e-test-*") | ||
| for index in indices: | ||
| _ = await opensearch_client.indices.delete(index=index) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider narrowing exception suppression in cleanup.
The cleanup function suppresses all exceptions, which may hide unexpected failures during teardown. While broad suppression is sometimes acceptable in test cleanup to prevent cascade failures, consider catching only expected exceptions like NotFoundError or ConnectionError from opensearch-py for better diagnostics.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/opensearch/test_opensearch.py around
lines 55 to 59, the cleanup_opensearch_indices function currently suppresses all
Exceptions; change this to only suppress expected opensearch-py exceptions
(e.g., NotFoundError and ConnectionError) so unexpected errors surface. Import
the specific exceptions from opensearchpy.exceptions (or the correct package
used) and replace contextlib.suppress(Exception) with
contextlib.suppress(NotFoundError, ConnectionError); keep the rest of the
deletion logic the same.
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py
Outdated
Show resolved
Hide resolved
Test Failure AnalysisSummary: Tests are failing due to a Docker port mapping misconfiguration in the Elasticsearch test setup, and an intermittent worker crash in Windows tests. Root Cause: Primary Issue: Elasticsearch Port Mapping (Linux/macOS)In
This attempts to map container port 19200 → host port 9200, but Elasticsearch containers expose port 9200 internally, not 19200. The container never starts listening on port 19200, causing the Docker container to fail health checks. Compare with OpenSearch (working correctly): CONTAINER_PORT = 9200 # Container's internal port
HOST_PORT = 19201 # Host machine port
{str(CONTAINER_PORT): HOST_PORT} # Maps 9200 → 19201Secondary Issue: Windows Test Worker CrashThe test Suggested Solution: Fix Elasticsearch Port MappingIn
After fixing, run Fix Windows Worker CrashThe multi_disk test may need to be marked as non-parallel on Windows or have cleanup improved. This is likely not critical as it's intermittent. Detailed AnalysisFailed Jobs
Elasticsearch Error PatternThe container fails to start because:
Windows Worker CrashThis is a pytest-xdist issue where the worker process terminated unexpectedly, possibly due to file locking or resource cleanup issues on Windows. Related Files
|
Test Failure AnalysisSummary: Tests are failing on Root Cause: The Elasticsearch tests are failing with two types of errors:
The port mapping fix in commit Lines 103-108: The @pytest.fixture
async def es_client(self) -> AsyncGenerator[AsyncElasticsearch, None]:
async with AsyncElasticsearch(hosts=[ES_URL]) as es_client: # ← Closes on exit
try:
yield es_client
finally:
await es_client.close() # ← Tries to close again (redundant)Lines 112-113: The @override
@pytest.fixture
async def store(self) -> ElasticsearchStore:
return ElasticsearchStore(url=ES_URL, index_prefix="kv-store-e2e-test") # ← Creates internal clientLines 124-128: The cleanup fixture uses
Why it only fails on ubuntu-22.04: This is likely a timing issue where ubuntu-22.04's Docker/Elasticsearch startup is slightly slower, causing the race conditions to manifest more consistently. Suggested Solution: Option 1: Remove the @override
@pytest.fixture
async def store(self) -> AsyncGenerator[ElasticsearchStore, None]:
store = ElasticsearchStore(url=ES_URL, index_prefix="kv-store-e2e-test")
async with store:
yield store
@pytest.fixture
async def sanitizing_store(self) -> AsyncGenerator[ElasticsearchStore, None]:
store = ElasticsearchStore(
url=ES_URL,
index_prefix="kv-store-e2e-test",
key_sanitization_strategy=ElasticsearchV1KeySanitizationStrategy(),
collection_sanitization_strategy=ElasticsearchV1CollectionSanitizationStrategy(),
)
async with store:
yield store
@pytest.fixture(autouse=True)
async def cleanup_elasticsearch_indices(self):
# Create a separate cleanup client
async with AsyncElasticsearch(hosts=[ES_URL]) as cleanup_client:
await cleanup_elasticsearch_indices(elasticsearch_client=cleanup_client)
yield
async with AsyncElasticsearch(hosts=[ES_URL]) as cleanup_client:
await cleanup_elasticsearch_indices(elasticsearch_client=cleanup_client)Why this works:
Detailed AnalysisFailed Jobs
Error Pattern from LogsWhy Port Mapping is CorrectThe latest commit changed:
Elasticsearch IS starting successfully and responding to health checks. The issue is purely in the test fixture lifecycle management. Related Files
|
|
@claude let's remove the elasticsearch changes from this PR altogether please and let's do a thorough line by line code review to make sure this is ready for production use |
|
@claude let's remove the elasticsearch changes from this PR altogether please and let's do a thorough line by line code review to make sure this is ready for production use. Please also fix merge conflicts |
|
Claude finished @strawgate's task —— View job Production Readiness Code Review - OpenSearch Store Implementation ✅Task List:
Executive SummaryThe OpenSearch store implementation is largely production-ready with good architecture and follows the project's patterns well. However, there are several critical and moderate issues that must be addressed before merging: Critical Issues (Must Fix)
Moderate Issues (Should Fix)
Minor Issues (Nice to Have)
Detailed Code Review1. ❌ CRITICAL: Missing Type-Ignore CommentsFiles: The optional # Current (WRONG):
from opensearchpy import AsyncOpenSearch
from opensearchpy.exceptions import RequestError
# Should be:
from opensearchpy import AsyncOpenSearch # type: ignore[reportMissingImports]
from opensearchpy.exceptions import RequestError # type: ignore[reportMissingImports]Why this matters: CI will fail on static analysis checks. Fix: Add type-ignore comments to the async version, then run 2. ❌ CRITICAL: No Bulk Write OperationsThe Elasticsearch store has Impact:
Evidence: Commit Recommendation: Either:
Example for Option B: class OpenSearchStore(...):
"""An OpenSearch-based store.
Stores collections in their own indices and stores values in Flattened fields.
Note: This implementation does not include bulk write operations (_put_managed_entries,
_delete_managed_entries). Batch operations will fall back to sequential writes,
which may have performance implications for high-volume use cases.
...
"""3. ❌ CRITICAL: Overly Broad Exception HandlingFile: Multiple methods catch all exceptions, which is dangerous: Line 289-290: try:
opensearch_response = await self._client.get(index=index_name, id=document_id)
except Exception:
return NoneLine 312-315: try:
opensearch_response = await self._client.mget(...)
except Exception:
return [None] * len(keys)Also: Lines 381, 411, 448, 474 have similar patterns. Problem: This catches everything including:
Fix: Import and catch specific exceptions: from opensearchpy.exceptions import NotFoundError, ConnectionError, TransportError
try:
opensearch_response = await self._client.get(index=index_name, id=document_id)
except (NotFoundError, ConnectionError, TransportError):
return None4.
|
These changes were unrelated to OpenSearch support and should be in a separate PR. Co-authored-by: William Easton <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (1)
1-209: These Elasticsearch test changes should be removed from this PR.According to the PR objectives, Elasticsearch-related changes should be removed from this PR, which is focused on adding OpenSearch support. These modifications to the Elasticsearch test suite appear to be unintended.
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (1)
1-3: Auto-generated file should not be manually modified.This file is auto-generated by
build_sync_library.pyfrom the async test file. Changes should be made to the original async file (key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py), and this file should be regenerated.
♻️ Duplicate comments (2)
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (2)
26-27: Same issue as async version: Importing from private module.This mirrors the issue in the async test file. See the review comment on
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.pylines 23-24.
49-54: Same critical issue as async version: Cluster health check incompatible with single-node setup.This mirrors the critical issue in the async test file. The health check will fail for single-node Elasticsearch clusters that remain in "yellow" status. See the review comment on
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.pylines 48-53 for the suggested fix.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py(4 hunks)key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (1)
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (1)
es_client(113-115)
key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (1)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (1)
es_client(112-114)
🔇 Additional comments (2)
key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py (1)
112-114: LGTM: Fixture simplification is correct.The removal of explicit close logic is appropriate since the
async withcontext manager automatically handles cleanup via__aexit__.key-value/key-value-sync/tests/code_gen/stores/elasticsearch/test_elasticsearch.py (1)
113-115: LGTM: Fixture simplification is correct.This change correctly relies on the context manager for cleanup, mirroring the improvement in the async version.
| if TYPE_CHECKING: | ||
| from elastic_transport._response import ObjectApiResponse |
There was a problem hiding this comment.
Importing from a private module.
The import of ObjectApiResponse from elastic_transport._response uses a private module (indicated by the underscore prefix). While this is necessary for type annotations, it may break if the elasticsearch library refactors internal modules.
🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py
around lines 23–24, the test imports ObjectApiResponse from the private module
elastic_transport._response; replace this fragile private import by attempting
to import the type from a public, stable location (the library's public export)
inside the TYPE_CHECKING block and, if that isn’t available, fall back to using
typing.Any (or alias ObjectApiResponse = Any) so tests keep type hints without
relying on internal modules. Ensure the code uses a safe conditional
import/alias pattern under TYPE_CHECKING and update any type annotations
accordingly.
| if not await es_client.ping(): | ||
| return False | ||
|
|
||
| status: ObjectApiResponse[dict[str, Any]] = await es_client.options(ignore_status=404).cluster.health(wait_for_status="green") | ||
|
|
||
| return status.body.get("status") == "green" |
There was a problem hiding this comment.
Cluster health check may fail for single-node Elasticsearch.
The health check waits for status="green", but single-node Elasticsearch clusters typically remain in "yellow" status because they cannot allocate replicas. Since the test configuration uses "discovery.type": "single-node" (line 103), this check will likely timeout or cause tests to fail.
Additionally, ignore_status=404 is unnecessary here as cluster.health does not return 404 status codes.
Apply this diff to accept yellow status for single-node clusters:
- status: ObjectApiResponse[dict[str, Any]] = await es_client.options(ignore_status=404).cluster.health(wait_for_status="green")
+ status: ObjectApiResponse[dict[str, Any]] = await es_client.cluster.health(wait_for_status="yellow", timeout="30s")
- return status.body.get("status") == "green"
+ return status.body.get("status") in ("green", "yellow")🤖 Prompt for AI Agents
In key-value/key-value-aio/tests/stores/elasticsearch/test_elasticsearch.py
around lines 48 to 53, the cluster health check currently waits for "green" and
calls cluster.health with ignore_status=404; change it to accept "yellow" for
single-node clusters by waiting for either "green" or "yellow" (or treat yellow
as acceptable) and remove the unnecessary ignore_status=404 option; update the
return condition to check that status.body.get("status") is in ("green",
"yellow") so single-node tests don't fail.
Test Failure AnalysisSummary: The codegen check is failing because the Elasticsearch store (inherited from main branch) has an incorrect import error message that the codegen script now correctly fixes, producing uncommitted changes. Root Cause: This PR branch merged from main in commit Previously, the codegen script didn't transform string literals, so this bug went undetected. However, commit The codegen check fails because:
Suggested Solution: Since the developer requested to "remove elasticsearch changes from this PR altogether" (comment above), this is actually NOT a bug in this PR. This is a pre-existing bug in the main branch that was exposed by the improved codegen script. Option 1: Fix in this PR (Recommended)Simply run Command: make codegen lint
git add -A
git commit -m "fix: apply codegen to fix Elasticsearch import message from main"
git pushWhy this is acceptable: Even though the developer wanted to remove Elasticsearch changes, this fix is actually:
Option 2: Revert the codegen improvement temporarilyRemove the Option 3: Cherry-pick fix to main separately
Recommendation: Use Option 1. The improved codegen script is valuable and should be kept. Simply commit the auto-generated fix to the Elasticsearch import message. Detailed AnalysisFailed Check Output:diff --git a/key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
index cbd016c..ad3e9d0 100644
--- a/key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
+++ b/key-value/key-value-sync/src/key_value/sync/code_gen/stores/elasticsearch/store.py
@@ -38,7 +38,7 @@ try:
get_source_from_body,
)
except ImportError as e:
- msg = "ElasticsearchStore requires py-key-value-aio[elasticsearch]"
+ msg = "ElasticsearchStore requires py-key-value-sync[elasticsearch]"
raise ImportError(msg) from eCodegen Improvement (commit 3a29eaf):The def visit_Constant(self, node: ast.Constant) -> ast.AST:
# Transform string literals containing package names
if isinstance(node.value, str):
# Replace py-key-value-aio with py-key-value-sync
node.value = node.value.replace("py-key-value-aio", "py-key-value-sync")
# Remove [async] extras from package install instructions
node.value = node.value.replace("opensearch-py[async]", "opensearch-py")
self.generic_visit(node)
return nodeThis is a valuable improvement that ensures all string references to package names are correctly transformed. Timeline:
Related Files
|
Implement OpenSearchStore to support Amazon OpenSearch and open-source OpenSearch deployments. This addresses the incompatibility with the Elasticsearch client which rejects OpenSearch servers.
Changes
The implementation mirrors the Elasticsearch store architecture but uses the opensearch-py client library to avoid product detection errors.
Fixes #209
Generated with Claude Code) | View branch | [View job run](https://github.com/strawgate/py-key-value/actions/runs/19178303821
Summary by CodeRabbit
New Features
Dependencies