fix(workbench): do not block TimeSeries under ACL module detection failure - #6232
fix(workbench): do not block TimeSeries under ACL module detection failure#6232trippyogi wants to merge 5 commits into
Conversation
…er ACL Skip the module gate when the command succeeded or modules are unknown, and fall back to HELLO/INFO module detection when MODULE LIST and COMMAND INFO are unavailable. Fixes redis#5357
Prefer HELLO for module discovery when MODULE LIST fails, and only suppress ModuleNotLoaded for successful command replies.
![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: 6eed2d8154
ℹ️ 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".
| const isSuccessfulResult = Array.isArray(result) | ||
| ? result.length > 0 && | ||
| result.every( | ||
| (item) => item?.status === CommandExecutionStatus.Success, | ||
| ) | ||
| : result?.status === CommandExecutionStatus.Success |
There was a problem hiding this comment.
When Workbench renders grouped results, QueryCardCliGroupResult calls CommonErrorResponse(item.id, item.command, item.response), so this new success check sees the Redis reply itself rather than the item.status. For a successful module command whose reply is an array, such as TS.RANGE, result.every(item => item?.status === 'success') is false because the array entries are data points, so an ACL connection with an empty/unknown modules list still replaces the successful grouped reply with ModuleNotLoaded. Pass the grouped command status or treat the wrapper result consistently before running checkUnsupportedModuleCommand.
Useful? React with 👍 / 👎.
Grouped results were handing CommonErrorResponse the raw Redis reply, so successful TS.RANGE arrays looked like failures under ACL. Pass the CommandExecutionResult wrapper so item.status is visible.
|
@trippyogi I haven't had a chance to review the PR in detail yet, but I noticed that the |
Apply prettier formatting and assert group-mode fixture types so the PR does not introduce a new tsc baseline error.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6920994277
ℹ️ 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 (entries.some(([key]) => key === 'module')) { | ||
| return []; |
There was a problem hiding this comment.
When HELLO is blocked but INFO is allowed, Redis reports modules as module:name=timeseries,ver=... lines, and this repo's getInfo() parser collapses those into info.modules.module. This new branch discards that data entirely, so restricted ACL users in exactly that fallback path still get [] from determineDatabaseModulesUsingHelloOrInfo and Workbench continues to think TimeSeries is not loaded. Parse the collapsed module string (or fetch raw INFO modules) before giving up.
Useful? React with 👍 / 👎.
2 participants
Summary
Fixes #5357
Test plan
Note
Medium Risk
Changes module discovery fallbacks and Workbench error heuristics for ACL-restricted connections; behavior is covered by new unit tests but affects how module availability is inferred and displayed.
Overview
Fixes Workbench showing Time series unavailable for ACL users who can run
TS.RANGEbut lackMODULE LIST/COMMAND INFO, and improves backend module discovery in the same situations (#5357).Workbench UI:
CommonErrorResponseno longer maps successful commands toModuleNotLoadedwhen execution status is success, even if the connected instance has no detected modules. Group-mode rendering passes{ status, response }into that helper so array replies (e.g.TS.RANGE) are not mistaken for failures.API module detection: After
MODULE LISTfails, the flow still probes viaCOMMAND INFO; if that yields nothing, it adds a HELLO-first fallback (then INFO), with parsing that ignores INFO’s collapsedmodule:line. TimeSeries probes now includets.rangeandts.revrange.Reviewed by Cursor Bugbot for commit 6920994. Bugbot is set up for automated code reviews on this repo. Configure here.