feat(search): add WITHSCORES support to FT.SEARCH (#2143) - #3432
feat(search): add WITHSCORES support to FT.SEARCH (#2143)#3432watersRand wants to merge 3 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7741133c79
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if(typeof reply[i] === 'number' || (typeof reply[i] === 'string' && !isNaN(Number(reply[i])) && Array.isArray(reply[i + 1]))){ | ||
| score = Number(reply[i++]); |
There was a problem hiding this comment.
For RESP2, FT.SEARCH ... WITHSCORES returns each row as id, score, fields, but this checks for and consumes a score before consuming the ID. A reply such as [1, 'doc', '1', ['field', 'value']] is therefore parsed with '1' as the document value and then produces a spurious row. Moreover, without WITHSCORES, a numeric document ID in [1, '1', ['field', 'value']] is mistaken for a score, corrupting ordinary RESP2 searches as well; consume the ID first and determine score presence from the requested reply layout rather than the ID's numeric shape.
Useful? React with 👍 / 👎.
| VERBATIM?: boolean; | ||
| NOSTOPWORDS?: boolean; | ||
| INKEYS?: RedisVariadicArgument; | ||
| WITHSCORES?: boolean; |
There was a problem hiding this comment.
SEARCH_NOCONTENT.parseCommand reuses Parameters<typeof SEARCH.parseCommand>, so adding WITHSCORES here also exposes and serializes it for ft.searchNoContent. That command's separate RESP2 transformer returns reply.slice(1), treating every score as another document ID, while its RESP3 transformer maps the main result back to IDs and silently discards every score. Callers can therefore request scores through the public type but cannot receive a valid no-content result; either exclude this option from that variant or preserve ID/score pairs in its reply contract.
Useful? React with 👍 / 👎.
| const rawScore = getMapValue(resultMap,['score']); | ||
| const score = rawScore !== undefined ? Number(rawScore) : undefined; |
There was a problem hiding this comment.
When a caller uses withTypeMapping({ [RESP_TYPES.DOUBLE]: String }), the RESP3 decoder supplies the score as a string, but this unconditional Number conversion changes it back to a number, so the newly exposed field ignores the client's requested reply mapping. Scored commands normally pass RESP3 doubles through and use transformDoubleReply[2] for RESP2; this field should follow the same pattern and use a DoubleReply-compatible type so the runtime value and inferred type remain consistent.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d226bdb672
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| _preserve?: any, | ||
| _typeMapping?: TypeMapping | ||
| _typeMapping?: TypeMapping, | ||
| options?: FtSearchOptions |
There was a problem hiding this comment.
Although the prior parser-order issue is addressed, the new option-aware branch still cannot execute through the client: command transformers are called with only (reply, parser.preserve, typeMapping), parseCommand never assigns the search options to parser.preserve, and this fourth parameter is therefore always undefined. On RESP2, requests using WITHSCORES, NOCONTENT, WITHPAYLOADS, or WITHSORTKEYS consequently parse every optional element as document content or another ID; preserve the relevant options during command parsing and consume them as the transformer's second argument.
Useful? React with 👍 / 👎.
| id: String((id as { toString?(): string })?.toString?.() ?? id ?? ''), | ||
| ...(score !== undefined && !isNaN(score) ? {score} : {}), | ||
| ...(scoreExplain !== undefined ? {scoreExplain} : {}), | ||
| ...(rawPayload !== undefined ? {payload: String(rawPayload)} : {}), |
There was a problem hiding this comment.
When WITHPAYLOADS is used for an ordinary document with no payload, Redis returns null and this expression exposes the string "null"; with a blob-string-to-Buffer type mapping, it also decodes arbitrary payload bytes through UTF-8 instead of preserving the Buffer. This makes RESP3 behavior inconsistent with RESP2 and corrupts binary payloads, so the raw null/blob value should be retained and the reply type should include those possibilities.
Useful? React with 👍 / 👎.
| if (options?.EXPLAINSCORE) { | ||
| parser.push('EXPLAINSCORE'); |
There was a problem hiding this comment.
When a caller supplies { EXPLAINSCORE: true }, which the new options interface permits independently, the parser emits EXPLAINSCORE without WITHSCORES. Redis requires WITHSCORES for this modifier, so this validly typed invocation fails at the server instead of returning a search reply; either add WITHSCORES automatically or reject/encode the invalid combination in the public API.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit d226bdb. Configure here.
| ...(score !== undefined && !isNaN(score) ? {score} : {}), | ||
| ...(scoreExplain !== undefined ? {scoreExplain} : {}), | ||
| ...(rawPayload !== undefined ? {payload: String(rawPayload)} : {}), | ||
| ...(rawSortKey !== undefined ? {sortKey: String(rawSortKey)} : {}), |
There was a problem hiding this comment.
Null payload coerced to string null
Medium Severity
RESP3 WITHPAYLOADS (and WITHSORTKEYS) replies include a null field when the document has no payload or sort key. String(null) turns that into 'null', so the document gets a real string instead of omitting the field. Callers that check payload for presence then treat missing payloads as present.
Reviewed by Cursor Bugbot for commit d226bdb. Configure here.
| assert.strictEqual(res.total, 1); | ||
| assert.strictEqual(res.documents.length, 1); | ||
| assert.strictEqual(res.documents[0].id, '1'); | ||
| console.log(res.documents[0].payload); |
There was a problem hiding this comment.
Debug log left in WITHPAYLOADS test
Low Severity
The WITHPAYLOADS client test still calls console.log on res.documents[0].payload, which will print on every test run.
Reviewed by Cursor Bugbot for commit d226bdb. Configure here.
|
Hi @watersRand, thanks for taking this! It looks like there are some outstanding comments from the bots. If you need any help, let me know! |
2 participants
Description
This PR resolves issue #2143 by adding full support for the
WITHSCORESoption inFT.SEARCHcommands within@redis/search.Previously, the
WITHSCORESflag was either ignored during argument parsing or its returned scores were dropped during reply transformation, causing the relevance score to be missing from search results.This change ensures that:
WITHSCORESis properly serialized into the command arguments when requested.scoreis correctly parsed and extracted in both RESP2 and RESP3 reply transformers.SearchReplytype definition is updated to expose an optionalscore?: numberproperty on each returned document.Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Reply parsing for
FT.SEARCHis reworked and now depends on search options being available duringtransformReply; mis-wiring could mis-parse hits or drop fields.Overview
Expands
@redis/searchFT.SEARCHso callers can pass several RediSearch flags and structured filters throughFtSearchOptions, with matching updates to how replies are turned intoSearchReply.Command building adds serialization for
WITHSCORES,EXPLAINSCORE,NOCONTENT,WITHPAYLOADS,WITHSORTKEYS,PAYLOAD, plusFILTERandGEOFILTER(each accepts one object or an array, emitting repeatedFILTER/GEOFILTERclauses on the wire).Reply handling stops inferring document shape from the raw RESP2 array and instead walks each hit using the request options (
WITHSCORES,EXPLAINSCORE,WITHPAYLOADS,WITHSORTKEYS,NOCONTENT). Documents can now expose optionalscore,scoreExplain,payload, andsortKey; RESP3 result mapping pulls the same fields from per-result maps.Tests add
parseArgscoverage for the new options and live-client checks for scores, explain output, nocontent, payloads, sort keys, numericFILTER, geoGEOFILTER, andPAYLOAD.Reviewed by Cursor Bugbot for commit d226bdb. Bugbot is set up for automated code reviews on this repo. Configure here.