Python: fix(python/redis): wrap IndexDefinition prefix in a list#13898
Python: fix(python/redis): wrap IndexDefinition prefix in a list#13898Doondi-Ashlesh wants to merge 2 commits intomicrosoft:mainfrom
Conversation
redis-py's IndexDefinition expects prefix to be a list or tuple. Passing a bare string causes it to iterate character-by-character, producing a malformed FT.CREATE PREFIX command that fails at runtime. Wrap the prefix string in a list so the correct prefix is passed. Fixes microsoft#13894 Signed-off-by: Doondi-Ashlesh <doondiashlesh@gmail.com>
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 95%
✓ Correctness
This is a correct and important bug fix. The
IndexDefinition._append_prefix()method (redis-py 6.4.0) iterates over itsprefixparameter withfor p in prefixand reportslen(prefix)as the prefix count. When a bare string like"my_collection:"was passed,len()returned the character count (e.g. 14) and the loop iterated over individual characters, producing a malformed index definition. Wrapping the string in a list ensureslen(prefix)is 1 and the loop yields the single correct prefix string.
✓ Security Reliability
This is a correct bug fix. The
IndexDefinition._append_prefix()method expects an iterable of prefix strings — it callslen(prefix)and then iterates over each element. When a bare string like"mycollection:"was passed, Python iterated over individual characters (e.g.,"m","y","c", ..), producing a malformed index prefix definition. Wrapping the string in a list[f"{self.collection_name}:"]matches the expected type (the default parameter isprefix=[]) and ensures the full prefix string is treated as a single entry. No security or reliability issues introduced.
✗ Test Coverage
The change wraps the
prefixparameter from a string to a list inIndexDefinition(prefix=[...]), which is a bug fix aligning with the redis-py API. However, the existing unit testtest_create_index(line 294) uses an autouse mock that patchescreate_indexentirely, so it never actually constructs anIndexDefinitionor validates the prefix format. The test passes identically with both the old and new code, meaning there is no test that verifies this fix. There should be an assertion that theIndexDefinitionis constructed with a list prefix.
✓ Design Approach
The change correctly fixes a genuine bug:
IndexDefinition._append_prefix(redis 7.4.0, index_definition.py:43-49) callslen(prefix)and iterates overprefixwith a for-loop. When passed a plain string like"mycollection:", this iterates over each character, producing a malformedPREFIX 12 m y c o l e c t i o n :Redis command instead of the intendedPREFIX 1 mycollection:. Wrapping the string in a list is the correct fix. The test at line 302 in test_redis_store.py still passes a bare stringprefix="test:"directly toIndexDefinition, which has the same bug, but that is outside the scope of this diff.
Flagged Issues
- No test verifies the actual fix.
test_create_index(test_redis_store.py:294) mockscreate_indexvia the autouse fixture at line 83, soIndexDefinitionis constructed but never inspected. The test passes with bothprefix=stringandprefix=[list]. Add a test (or update the existing one) that assertscreate_indexwas called with anIndexDefinitionwhose prefix is a list.
Suggestions
- test_redis_store.py:302 constructs
IndexDefinition(prefix="test:", ...)with a bare string, which will also iterate character-by-character per the same_append_prefixlogic. Consider fixing that test fixture as a follow-up to avoid a misleading test.
Automated review by Doondi-Ashlesh's agents
| 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] |
There was a problem hiding this comment.
This fix is not covered by any test. test_create_index in test_redis_store.py:294 mocks create_index and never inspects the IndexDefinition argument. Please add a test that asserts create_index was called with a definition whose prefix is a list (e.g., via mock.call_args). Without this, the bug could regress silently.
…reate_index The existing test_create_index called ensure_collection_exists() but never inspected the IndexDefinition passed to create_index, so it passed even when prefix was a bare string (the original bug). - Parametrize test_create_index over hashset and json collection types. - Assert that the IndexDefinition passed to create_index has args matching a reference built with a list prefix. A bare string prefix produces PREFIX <len(string)> <char> ... while a list produces PREFIX 1 <string>, so the args comparison catches the regression exactly. - Fix test_create_index_manual which also used a bare string prefix="test:". Changed to prefix=["test:"] to match correct usage. Signed-off-by: Doondi-Ashlesh <doondiashlesh@gmail.com>
Summary
Fixes #13894
redis-py'sIndexDefinitionexpectsprefixto be alistortuple. Inpython/semantic_kernel/connectors/redis.py:281the prefix was passed as a bare string:Because Python iterates a string character-by-character,
redis-pywas building aFT.CREATE PREFIXcommand with each character as a separate prefix entry instead of the intended single prefix. This causedcreate_collectionto fail with a malformed command.Change
Wrapping the string in a list ensures a single, correctly formed prefix is passed to
FT.CREATE.Testing
python/tests/unit/connectors/memory/test_redis_store.pycontinue to pass.FT.CREATEnow receives the correctPREFIX 1 <collection_name>:argument.Signed-off-by: Doondi-Ashlesh doondiashlesh@gmail.com