Python: fix(redis): wrap prefix string in list for IndexDefinition#13911
Python: fix(redis): wrap prefix string in list for IndexDefinition#13911armorbreak001 wants to merge 1 commit intomicrosoft:mainfrom
Conversation
redis-py's IndexDefinition treats the prefix argument as an iterable. Passing a plain string causes each character to become a separate prefix (e.g., 'foo:' becomes ['f','o','o',':']). Wrap in a list to send a single prefix.
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 94%
✓ Correctness
This is a correct bug fix. The redis-py IndexDefinition constructor expects
prefixto be a list of strings. The old code passed a bare string, which caused_append_prefixto iterate over individual characters (since strings are iterable in Python), producing malformed Redis FT.CREATE commands likePREFIX 18 c o l l e c t i o n _ n a m e :instead of the intendedPREFIX 1 collection_name:. Wrapping the string in a list is the right fix.
✓ Security Reliability
This is a correct bug fix. The redis-py
IndexDefinition._append_prefixmethod iterates over theprefixparameter and useslen(prefix)to emit the count. When a bare string was passed,len()returned the character count and the loop iterated character-by-character, producing a malformed RedisFT.CREATEcommand. Wrapping the string in a list is the correct fix. No security or reliability concerns are introduced by this change. A pre-existing instance of the same bug exists in the test file.
✗ Test Coverage
The fix correctly changes the
prefixargument toIndexDefinitionfrom a bare string to a list, which is necessary because_append_prefixiterates over the value and appendslen(prefix)as the count — a bare string would producePREFIX 5 t e s t :instead ofPREFIX 1 test:. However, the existing testtest_create_index(line 294) only asserts no exception is raised; it never verifies thatcreate_indexwas called with anIndexDefinitionwhose prefix is a list. This means the bugfix has no regression test that would catch a revert. Additionally,test_create_index_manual(line 302) still passesprefix="test:"(a bare string) toIndexDefinition, demonstrating the same incorrect usage this PR fixes.
✗ Design Approach
This change fixes the immediate failure for the connector's internally generated index definition, but it does so narrowly. The same Redis
IndexDefinitioncontract is still exposed unchanged through the manualindex_definitionpath, and the repo's own test still models that path withprefix="test:", so the patch addresses one constructor call rather than the broader compatibility boundary around Redis prefix handling.
Flagged Issues
- No test validates the fix:
test_create_index(test_redis_store.py:294) callsensure_collection_exists()but never asserts thatcreate_indexwas invoked with anIndexDefinitionwhose prefix is a list. The mock is available but unchecked — a revert of this fix would not be caught by any test. - The fix is too narrow:
ensure_collection_exists()now wraps the auto-generated prefix in a list, but the public/manualindex_definitionpath is still passed through unchanged (redis.py:272-276), and the repo's own test still constructsIndexDefinition(prefix="test:", ...)at test_redis_store.py:302. Calers using that supported path will still get a malformed character-wise prefix sequence.
Suggestions
- Update
test_create_index_manual(test_redis_store.py:302) to useprefix=["test:"]instead of the bare string"test:", so the test demonstrates correct usage consistent with this fix. - In
test_create_index, assert on thedefinitionargument passed to the mockedcreate_indexto verify the prefix is a single-element list (e.g., check that['PREFIX', 1, 'test:']appears indefinition.args), providing regression coverage for this fix. - The legacy memory store at
python/semantic_kernel/connectors/memory_stores/redis/redis_memory_store.py:132has the same bare-string bug (prefix=f"{collection_name}:"). Consider fixing it there too if the legacy connector is still supported. - Normalize Redis prefix handling at the connector boundary instead of only in the default constructor path. A small helper that converts a string prefix to
[prefix]before callingcreate_index()would make both the built-in and caller-suppliedIndexDefinitionpaths compatible.
Automated review by armorbreak001's agents
Summary
The Redis connector passes a plain string to
IndexDefinition(prefix=...), but redis-py treats theprefixargument as an iterable. This causes each character of the collection name to become a separate key-space prefix.For a collection named
redis_json_list_data_model, the wire output becomes:Instead of the expected:
Fix
Wrap the prefix string in a list so redis-py receives a single-element iterable.
Before:
prefix=f"{self.collection_name}:"After:
prefix=[f"{self.collection_name}:"]Fixes #13894