Python: fix: apply collection-name prefix to keys in RedisJsonCollection._inner_delete#14060
Open
michaelxer wants to merge 2 commits into
Open
Python: fix: apply collection-name prefix to keys in RedisJsonCollection._inner_delete#14060michaelxer wants to merge 2 commits into
michaelxer wants to merge 2 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes Redis JSON deletion to consistently apply the collection/key prefix and adds a unit test to validate delete behavior when a prefix is configured.
Changes:
- Apply
_get_redis_key(...)to Redis JSON.delete(...)calls in_inner_delete. - Add a unit test that verifies prefixed keys are passed to the underlying Redis delete API.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/semantic_kernel/connectors/redis.py | Ensures JSON deletes use the prefixed Redis key via _get_redis_key. |
| python/tests/unit/connectors/memory/test_redis_store.py | Adds coverage for delete behavior when a collection prefix is present. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+279
to
+282
| @mark.parametrize("type_", ["hashset", "json"]) | ||
| async def test_delete_with_prefix( | ||
| collection_with_prefix_hash, collection_with_prefix_json, mock_delete_hash, mock_delete_json, type_ | ||
| ): |
Comment on lines
+292
to
+296
| # Verify the collection-name prefix was applied to the key | ||
| if type_ == "hashset": | ||
| mock_delete.assert_called_once_with("test:id1") | ||
| else: | ||
| mock_delete.assert_called_once_with("test:id1") |
github-actions
Bot
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Author
|
@microsoft-github-policy-service agree |
…er_delete The _inner_delete method in RedisJsonCollection was passing raw keys to JSON.DEL without applying the collection-name prefix via self._get_redis_key(). This caused deletes to fail when prefix_collection_name_to_key_names=True, because the prefixed keys used in upsert/get wouldn't match the unprefixed keys passed to delete. The sibling RedisHashsetCollection._inner_delete already correctly uses self._get_redis_key(key). This fix applies the same pattern to the JSON collection. Fixes microsoft#13904
Use request.getfixturevalue to resolve only the needed fixture per parametrize case instead of injecting all four. Collapse the redundant if/else assertion into a single assert.
michaelxer
force-pushed
the
fix/redis-json-delete-prefix
branch
from
adad746 to
fee8451
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Fixes #13904
The method passes raw keys to without applying the collection-name prefix via . This causes deletes to fail when , because the prefixed keys used in upsert/get don't match the unprefixed keys passed to delete.
The sibling already correctly uses (line 581 of ).
Description
One-line fix in :
Test added in :
Added that verifies the collection-name prefix is applied to keys during delete for both hashset and JSON collection types.
Contribution Checklist