Skip to content

fix(index): return the Redis key as id for whole-document JSON FilterQuery results - #793

Draft
vishal-bala wants to merge 2 commits into
mainfrom
fix/json-result-id-is-redis-key
Draft

vishal-bala wants to merge 2 commits into
mainfrom
fix/json-result-id-is-redis-key

Conversation

@vishal-bala

Copy link
Copy Markdown
Collaborator

Motivation

A search result's id is the record's full Redis key in every case but one. A JSON FilterQuery without return_fields returns the whole document, and process_results built that result as {"id": key, **document}, so a document with its own id field overwrote the key. Measured on Redis 8.4, a document loaded with index.load([{"id": "t1", "status": "open"}], id_field="id") came back as {'id': 't1', 'status': 'open'} from FilterQuery(Tag("status") == "open"), and as {'id': '<prefix>:t1', 'status': 'open'} once return_fields=["status"] was added. Hash storage and every other search query already return the key. Code that passes result["id"] to drop_keys(), fetch() or an update therefore worked on hash and broke on JSON, depending only on whether return_fields was set.

Changes

The key always wins as id

The JSON unpack branch of process_results in redisvl/index/index.py now pops the document's own id before merging it under the key. Both SearchIndex and AsyncSearchIndex route query(), paginate() and batch_query() through that one function, so the sync and async paths change together.

The document's id field is dropped rather than kept under another name, because that is what hash results already do. redis-py's search Result parser runs del fields["id"] on every returned field set, in both its RESP2 and RESP3 paths, so a hash field named id has never appeared in a RedisVL result. Renaming the JSON field to something like _id would make the two storage types differ again, and would claim a field name that a user's document could also be using.

Documentation and tests

  • docs/concepts/queries.md gains a "Result ids" section. It states the guarantee for search queries, notes that aggregation, hybrid and SQL rows do not set id to the key, and gives three ways to read a document's own id back.
  • tests/integration/test_search_results.py adds a sync and an async test that each sweep hash and JSON storage, with and without return_fields.
  • tests/unit/test_query_types.py adds a hermetic test of the JSON unpack branch.

Notes

The guarantee covers FT.SEARCH-based queries only. AggregationQuery, AggregateHybridQuery, HybridQuery and SQLQuery return rows of loaded fields through process_aggregate_results or _convert_and_drop_empty_rows, and a field loaded under the name id keeps the document's value there. Those rows are projections rather than documents, so they are documented as outside the guarantee rather than changed.

Next Steps

The MCP code-tools stack (#758–#761, branches feat/mcp-code-tools/01 to 04) works around the old behaviour in documentation: the IndexHandle.document_id docstring in redisvl/mcp/context.py (#758), and one sentence each in docs/concepts/mcp.md and docs/user_guide/how_to_guides/mcp.md telling users to set return_fields on a JSON FilterQuery (#760). Whichever of this PR and that stack lands second should remove those caveats.

Release Notes

On JSON indexes, a FilterQuery without return_fields now returns each record's full Redis key as id, as hash indexes already did, and no longer includes the document's own id field, which previously replaced the key. To read that field back, add "$.id" to return_fields (it is returned under $.id), index it under another name such as {"name": "doc_id", "type": "tag", "path": "$.id"}, or load the document with index.fetch().

Reviewer Brief

Start here: process_results in redisvl/index/index.py, the unpack_json branch. It is the only behavioural change, and every sync and async search result passes through it.

Where it could fail

  • redisvl/index/index.py:534: JSON users who read result["id"] from a whole-document FilterQuery and expected the document's own field now get the key; check the Release Notes recovery routes cover that case.
  • docs/concepts/queries.md, "Result ids": the guarantee is scoped to search queries; check the wording does not promise anything for aggregation, hybrid or SQL rows.

Not verified: cluster mode, and Redis versions other than 8.4. The change touches no API-key paths.

Beyond CI: both new integration tests fail on main at the ('json', None) case and pass with the fix, and the new unit test fails without the pop; all three recovery routes in the Release Notes were run against Redis 8.4 and returned t1.

Reviewed 08d89812, fixes in c7126d5d · Claude Code (claude-opus-5-5) · lenses: correctness, security, design, tests, docs, Redis behaviour

…Query results

A JSON FilterQuery without return_fields spread the document over the
result after setting id, so a document field named id replaced the key.
The key now always wins and the document's own id field is dropped,
matching hash results and every query that sets return_fields.
Restore the process_aggregate_results docstring, which the previous
commit overwrote with process_results wording that does not apply to it,
and limit the queries.md guarantee to FT.SEARCH-based queries. Add a
hermetic unit test for the JSON unpack path.
@vishal-bala vishal-bala added the auto:minor Increment the minor version when merged label Oct 30, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto:minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant