[8.6] Resolve PARAMS in FT.SPELLCHECK command - [MOD-10596]#9079
[8.6] Resolve PARAMS in FT.SPELLCHECK command - [MOD-10596]#9079
PARAMS in FT.SPELLCHECK command - [MOD-10596]#9079Conversation
* add failing test * resolve params on spellcheck * properly propagate error code from QAST_EvalParams * gitignore .venv * small test improvement * fix build * improve coverage * remove support for params, align test * Resolve `PARAMS` in `FT.SPELLCHECK` command - [MOD-10596] (#8929) Revert "remove support for params, align test" (cherry picked from commit c29ae2b)
🛡️ Jit Security Scan Results
✅ No security findings were detected in this PR
Security scan by Jit
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d216132. Configure here.
| QueryError_ClearError(&status); | ||
| if (opts.params) { | ||
| Param_DictFree(opts.params); | ||
| } |
There was a problem hiding this comment.
Params freed before QAST destroy, inconsistent cleanup order
Low Severity
Param_DictFree(opts.params) is called before QAST_Destroy(&qast), but in aggregate_request.c the established convention is the reverse: QAST_Destroy at line 1456 runs before Param_DictFree at line 1498. This matters because QueryParam_Resolve for PARAM_VEC stores a direct pointer into the dict's RedisModuleString buffer without copying. Freeing the dict first creates dangling pointers that QAST_Destroy → QueryNode_Free could dereference. While spell check doesn't currently use vector params, the inconsistent ordering is a maintenance hazard.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit d216132. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.6 #9079 +/- ##
==========================================
+ Coverage 76.46% 83.84% +7.38%
==========================================
Files 367 367
Lines 55916 55841 -75
Branches 14322 14322
==========================================
+ Hits 42754 46818 +4064
+ Misses 13000 8861 -4139
Partials 162 162
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
2 participants
Description
Backport of #8901 to
8.6.Describe the changes in the pull request
Fix a bug where attributes/parameters in the
FT.SPELLCHECKcommand could be left unresolved, leading to an invalid read.Adding missing support for
PARAMSinFT.SPELLCHECK.Mark if applicable
Release Notes
Note
Medium Risk
Touches query parsing/parameter evaluation and error propagation for
FT.SPELLCHECK, which can affect request validation and failure modes across dialects; behavior is covered by new regression tests but impacts a core query path.Overview
FT.SPELLCHECKnow supportsPARAMS: the command parses provided param pairs intoRSSearchOptions, resolves them against the parsed query viaQAST_EvalParams, and frees the param dict on exit.QAST_EvalParamsis updated to propagate errors fromQueryNode_EvalParamsinstead of always returning success, and new pytests cover dialect 2/3 parameterized spellcheck, missing/invalidPARAMSerrors, and fuzzy param substitution..gitignoreis tweaked to ignore venv directories via/*venv*/.Reviewed by Cursor Bugbot for commit 34f5540. Bugbot is set up for automated code reviews on this repo. Configure here.