Skip to content

Python: fix(redis): wrap IndexDefinition prefix in list; use _get_redis_key in JSON delete #13970

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nileshpatil6 wants to merge 3 commits into
base: main
Choose a base branch
from
+32 −4
Open

Python: fix(redis): wrap IndexDefinition prefix in list; use _get_redis_key in JSON delete #13970

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
71226fe
fix(redis): wrap IndexDefinition prefix in list and use _get_redis_ke…
nileshpatil6 May 8, 2026
1f578ac
fix(redis): conditionally apply IndexDefinition prefix based on prefi…
nileshpatil6 May 8, 2026
2013d1f
test(redis): add coverage for prefix-aware IndexDefinition and JSON d…
nileshpatil6 May 8, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions python/semantic_kernel/connectors/redis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ async def ensure_collection_exists(self, **kwargs) -> None:
raise VectorStoreOperationException("Invalid index type supplied.")
fields = _definition_to_redis_fields(self.definition, self.collection_type)
index_definition = IndexDefinition(
prefix=f"{self.collection_name}:", index_type=INDEX_TYPE_MAP[self.collection_type]
prefix=[f"{self.collection_name}:"], index_type=INDEX_TYPE_MAP[self.collection_type]
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design mismatch & missing test coverage. Wrapping the prefix in a list fixes the redis-py API usage, but the index prefix is still unconditionally set to collection_name:. Both RedisHashsetCollection and RedisJsonCollection default prefix_collection_name_to_key_names=False (redis.py:524, redis.py:652), so _get_redis_key returns the raw key (redis.py:250-253). Default upsert writes id1 while the index only matches test:-prefixed keys. The prefix should be derived from prefix_collection_name_to_key_names. Additionally, test_create_index (test_redis_store.py:294) mocks create_index without inspecting its arguments — consider asserting on the mock's call_args to verify the definition was constructed with the correct prefix.

)
await self.redis_database.ft(self.collection_name).create_index(fields, definition=index_definition, **kwargs)
Comment on lines 279 to 286

Expand Down Expand Up @@ -706,7 +706,7 @@ def _add_key(self, key: TKey, record: dict[str, Any]) -> dict[str, Any]:

@override
async def _inner_delete(self, keys: Sequence[str], **kwargs: Any) -> None:
await asyncio.gather(*[self.redis_database.json().delete(key, **kwargs) for key in keys])
await asyncio.gather(*[self.redis_database.json().delete(self._get_redis_key(key), **kwargs) for key in keys])
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot May 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage. This fix is correct — RedisHashsetCollection._inner_delete (line 581) already uses _get_redis_key. However, the existing test_delete (test_redis_store.py:274) only uses non-prefix collections (collection_hash/collection_json), where _get_redis_key returns the key unchanged. The test passes identically with the old and new code. Please add a test with collection_with_prefix_json that asserts json().delete() receives 'test:id1', not 'id1'.


@override
def _serialize_dicts_to_store_models(
Expand Down
Loading