Repository navigation
Conversation
|
Hi, thanks for your contribution! Can you rebase onto the latest |
5a4abe7 to
972b789
Compare
|
Rebased onto latest |
vishal-bala
left a comment
There was a problem hiding this comment.
Thanks for your contribution! This looks good to go, just one minor thing to avoid bloating the tests and then we can get this merged.
| @@ -0,0 +1,97 @@ | |||
| """Explicit load keys and data may be single-pass iterables.""" | |||
There was a problem hiding this comment.
All four changed lines live in BaseStorage.write/awrite, so this doesn't need Redis or the hash/json × list/iterator matrix. Could you replace this file with a hermetic test in tests/unit/test_storage.py? Something like:
SCHEMA = IndexSchema.from_dict(
{"index": {"name": "t", "prefix": "t"}, "fields": [{"name": "title", "type": "text"}]}
)
OBJS = [{"title": "a"}, {"title": "b"}]
async def test_write_accepts_single_pass_iterables():
storage = HashStorage(index_schema=SCHEMA)
keys = ["t:1", "t:2"]
assert storage.write(MagicMock(), iter(OBJS), keys=iter(keys)) == keys
client = MagicMock()
client.pipeline.return_value.__aenter__.return_value = AsyncMock()
assert await storage.awrite(client, iter(OBJS), keys=iter(keys)) == keys
@pytest.mark.parametrize("keys", [[], ["t:1"], ["t:1", "t:2", "t:3"]])
async def test_write_rejects_mismatched_keys_before_writing(keys):
storage, client = HashStorage(index_schema=SCHEMA), MagicMock()
with pytest.raises(ValueError, match="Length of keys"):
storage.write(client, iter(OBJS), keys=iter(keys))
with pytest.raises(ValueError, match="Length of keys"):
await storage.awrite(client, iter(OBJS), keys=iter(keys))
client.pipeline.assert_not_called()It's 4 cases instead of 28, needs no Docker, fails on main and catches each line of the fix.
…it tests Per review, cover single-pass iterable support and mismatched-keys validation in BaseStorage.write/awrite with small Redis-free tests in tests/unit/test_storage.py.
SearchIndex.load()andAsyncSearchIndex.load()accept iterable data and keys, but passing an iterator for either argument with explicit keys currently raisesTypeErrorbecause storage callslen()before materializing the inputs. Explicit empty keys also bypass the length check and silently generate different keys for nonempty data.Materialize each iterable once before checking lengths in both storage write paths, and distinguish omitted keys from an explicit empty iterable. This preserves the existing validation error and rejects mismatches before any pipeline writes.
The integration tests use Redis 8.4 via the repository's Testcontainers setup, exercise HASH and JSON storage, sync and async APIs, all list/iterator combinations, and shorter/longer/empty keys with no partial writes.
Validation (Python 3.12.14):
make format,make check-types(120 source files),make check-format check-sort-imports,uv lock --check, and pre-commit on both changed files pass.make testwas attempted with the default locked development dependencies: 1,892 passed, 153 skipped, 2 xfailed; 195 failures and 154 setup errors reference missing optional packages (sentence_transformers,sql_redis,google,voyageai,pydantic_settings). This is not a passing full-suite result; provider/model tests were not run with credentials.pre-commit run --all-filespasses the code-quality hook but Codespell rewrites an existingUnparseablecomment in untouchedredisvl/redis/connection.py. That unrelated rewrite was reverted; the changed-file hooks pass.Suggested release label:
auto:patch.Implemented and tested with OpenAI Codex assistance.
Review record
Reviewed at
972b789by Grok Bot withpre-pr-review.Lenses: correctness (always; 12-line storage fix plus sync/async twin); tests (adds integration coverage for iterators and key-length mismatches). Skipped: security (no query-building, MCP, or destructive paths); design (bugfix, no API shape change); docs (behaviour already documented as Iterable); Redis behaviour (no new Redis commands or reply-shape assumptions).
_get_keysstill consumes a single-pass keys iterable twice (storage.py:185) (dead code — never called; out of scope);_preprocess_and_validate_objectsstill uses truthiness for keys (storage.py:308) (write/awrite already reject empty keys vs nonempty data before calling it); string mistaken for Iterable[str] would split into characters (pre-existing contract, not introduced here).Verified:
uv run black/isort --profile blackon the two changed files (unchanged);uv run mypy redisvl/index/storage.py(no issues).Not checked: integration tests in
tests/integration/test_load_iterables.py(no Docker on this machine); cluster mode; fullmake test.Note
Low Risk
Localized validation-order fix in storage write paths with regression tests; no API or Redis command changes.
Overview
Fixes
writeandawriteso iterables forobjectsandkeyswork withSearchIndex.load()/ async load: inputs are materialized to lists once before length checks, and an explicitly passed (including empty)keysiterable is distinguished from omitted keys.Mismatched object/key counts now raise
ValueErrorbefore any Redis pipeline runs, including when callers pass iterators instead of lists.Unit tests cover iterator inputs for sync/async write and parametrize empty, short, and long key lists to ensure no partial writes occur on mismatch.
Reviewed by Cursor Bugbot for commit 92b6298. Bugbot is set up for automated code reviews on this repo. Configure here.