Python: fix(python/redis): fix FT.CREATE prefix format and silent delete failure#13907
Python: fix(python/redis): fix FT.CREATE prefix format and silent delete failure#13907EaCognitive wants to merge 2 commits intomicrosoft:mainfrom
Conversation
github-actions
Bot
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 93%
✓ Correctness
Both changes are correct bug fixes. (1) The
prefixparameter toIndexDefinitionis changed from a string to a list, which matches the redis-py API expectation. (2)RedisJsonCollection._inner_deletenow correctly applies_get_redis_keyto prefix keys with the collection name, consistent with how_inner_get(line 699) and the siblingRedisHashsetCollection._inner_delete(line 581) already handle keys. Without this fix, JSON deletes would silently fail to find the correct keys.
✓ Security Reliability
Two bug fixes: (1) IndexDefinition prefix changed from string to list to match the redis-py API expectation, and (2) _inner_delete in the JSON-backed collection now correctly applies _get_redis_key() to prefix keys before deletion, consistent with the hash-backed collection's _inner_delete at line 581. Both changes are correct and improve reliability.
✗ Test Coverage
This PR fixes two bugs: (1) the
prefixparameter inIndexDefinitionis changed from a string to a list, and (2)RedisJsonCollection._inner_deletenow correctly applies_get_redis_keyto prepend the collection name prefix to keys before deletion. Both fixes align the behavior with the rest of the codebase. However, test coverage for these changes is insufficient.test_delete(line 274) only tests withoutprefix_collection_name_to_key_names=Trueand makes no assertions on the keys passed to the delete mock, meaning the JSON delete bug fix is not verified. Similarly, theensure_collection_existstests don't assert theprefixargument format passed tocreate_index. Bothtest_getandtest_upsertalready have prefix-parameterized variants, buttest_deletedoes not.
✓ Design Approach
Both changes are correct, targeted bug fixes. The
prefixchange from a bare string to a single-element list correctly matches the redis-py IndexDefinition API contract. The_get_redis_keyfix in RedisJsonCollection._inner_delete brings it in line with RedisHashsetCollection._inner_delete (line 581), which already applied the prefix transformation; without this fix, deletes silently do nothing when prefix_collection_name_to_key_names=True.
Flagged Issues
- The
_inner_deletefix forRedisJsonCollection(adding_get_redis_key) has no test withprefix_collection_name_to_key_names=True. The existingtest_delete(line 274 of test_redis_store.py) only uses non-prefix collections and doesn't assert on the arguments passed to the mock. Compare withtest_get(line 259) which parameterizes onprefix=True/False—test_deleteshould do the same, and should assert that the mock was called with the correctly prefixed keys.
Suggestions
- Add a prefix-parameterized variant of
test_deletefor both hashset and JSON collections (similar totest_get), asserting that the delete mock receives keys prefixed with the collection name whenprefix_collection_name_to_key_names=True. - Add an assertion in
test_create_indexthat verifies theIndexDefinitionpassed tocreate_indexhasprefixas a list (e.g.,['test:']) rather than a string, to directly test the first fix in this PR.
Automated review by EaCognitive's agents
| @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]) |
There was a problem hiding this comment.
This bug fix is not covered by any test. The existing test_delete at line 274 of test_redis_store.py only uses collections with prefix_collection_name_to_key_names=False, so _get_redis_key returns the key unchanged. Please add a test that uses collection_with_prefix_json and asserts the mock's delete was called with 'test:id1' (the prefixed key).
|
@microsoft-github-policy-service agree |
2 participants
Fixes #13894
Fixes #13904
Problem
The Python Redis connector in Semantic Kernel had two critical bugs:
FT.CREATE PREFIX(one prefix per character of collection name) #13894): TheIndexDefinitionprefix was passed as a string instead of a list, causing Redis to malform the index prefix. This effectively broke vector search (RAG) on Redis.RedisJsonCollection._inner_deletemethod used raw keys instead of calling_get_redis_key(). This caused deletion to silently fail whenprefix_collection_name_to_key_nameswas enabled.Solution
IndexDefinition._inner_deleteto use the_get_redis_key()helper for all keys.Changes
python/semantic_kernel/connectors/redis.py: Fixed prefix formatting and key resolution in deletion logic.Verification
IndexDefinitionnow receives a list of prefixes.