Python: fix(redis): pass collection prefix as a list#13903
Python: fix(redis): pass collection prefix as a list#13903MukundaKatta wants to merge 1 commit intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 95%
✗ Correctness
The production code fix (wrapping the
prefixargument in a list) is correct —IndexDefinition._append_prefixcallslen(prefix)and iterates over it, so passing a bare string like"test:"would producePREFIX 5 t e s t :instead of the intendedPREFIX 1 test:. However, the new testtest_create_index_uses_single_collection_prefixhas a bug: it parametrizesexpected_index_typeas"hash"/"json"strings, then passes them toIndexType(expected_index_type). SinceIndexTypeis anEnumwith integer values (HASH = 1,JSON = 2), callingIndexType("hash")raisesValueError: 'hash' is not a valid IndexType. The test will fail at assertion time, not because the code is wrong, but because the expected value cannot be constructed.
✓ Security Reliability
This is a correctness bug fix for the Redis connector. The
IndexDefinitionconstructor'sprefixparameter is iterable, and passing a bare string like"test:"causes redis-py to iterate over each character as a separate prefix (e.g.,"t","e","s","t",":") instead of treating the whole string as one prefix. Wrapping it in a list["test:"]ensures the prefix is correctly interpreted as a single entry. The new test properly validates this behavior by patchingIndexDefinitionand asserting it receives the list form. No security or reliability issues found.
✓ Test Coverage
The diff changes the
prefixargument toIndexDefinitionfrom a bare string to a single-element list, matching the redis-py API expectation. The existingtest_create_index_manualtest is updated to reflect the new list format, and a new parametrized testtest_create_index_uses_single_collection_prefixis added to verify both hash and JSON collection types passprefix=["test:"]toIndexDefinition. The new test properly patchesIndexDefinitionat the correct import path, asserts call arguments, and verifies the returned definition object is forwarded tocreate_index. Test coverage for this change is thorough.
✗ Design Approach
The core fix — changing
prefix=f"{self.collection_name}:"(a string) toprefix=[f"{self.collection_name}:"](a list) — is correct and necessary.IndexDefinition._append_prefixiterates withfor p in prefix, so a bare string would iterate individual characters, producing 5 (or more) nonsense prefix entries instead of one. The accompanying test update totest_create_index_manualis also correct. However, the new parametrized testtest_create_index_uses_single_collection_prefixcontains a bug: it passes string values"hash"and"json"toIndexType(expected_index_type), butIndexTypeis a standardEnumwith integer values (HASH=1,JSON=2).IndexType("hash")will raiseValueError: 'hash' is not a valid IndexTypeat test execution time, meaning the new test always fails and provides no coverage.
Automated review by MukundaKatta's agents
|
|
||
| @mark.parametrize( | ||
| ("fixture_name", "expected_index_type"), | ||
| [ | ||
| ("collection_hash", "hash"), | ||
| ("collection_json", "json"), |
There was a problem hiding this comment.
Bug: IndexType("hash") and IndexType("json") will raise ValueError because IndexType is an Enum with integer values (HASH = 1, JSON = 2), not string values. The parametrize should use the actual IndexType enum members directly.
| @mark.parametrize( | |
| ("fixture_name", "expected_index_type"), | |
| [ | |
| ("collection_hash", "hash"), | |
| ("collection_json", "json"), | |
| @mark.parametrize( | |
| ("fixture_name", "expected_index_type"), | |
| [ | |
| ("collection_hash", IndexType.HASH), | |
| ("collection_json", IndexType.JSON), | |
| ], | |
| ) |
Summary
IndexDefinition.prefixas a single-item list instead of a plain stringredis-pyAPI shapeCloses #13894
Verification
git diff --checkensure_collection_exists()prefix constructionNotes