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 7 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 👍 / 👎.
Thanks @pawelangelow. fixed the eslint / typecheck failures in 6920994. |
Cast the nested group-mode mock through unknown so CI type-check does not treat the Props result assertion as an unsafe conversion.
Avoid indexing SUPPORTED_REDIS_MODULES with any (TS7053) and refresh the API tsc baseline after removing those errors.
|
@pawelangelow Follow-up: a couple more type-check issues showed up after 6920994. those are fixed in 75eae6c, and checks are green now. Ready whenever you have time to review. |
2 participants
Summary
Fixes #5357
Test plan
Note
Medium Risk
Touches database module discovery fallbacks and Workbench error rendering for module commands; behavior changes for ACL-restricted connections but is guarded by success status and new unit tests.
Overview
Fixes Workbench showing Time series unavailable when ACL users can run
TS.RANGEbut Redis Insight cannot discover loaded modules (#5357).API: When
MODULE LISTfails, module detection still triesCOMMAND INFO, then a new HELLO-first fallback (with INFO only if HELLO yields nothing). HELLO is preferred because parsed INFO# Modulescan collapse multiple modules into one unusablemodule:string. Shared mapping is refactored intomapToAdditionalRedisModules; TimeSeries probe commands now includets.rangeandts.revrange.UI:
CommonErrorResponseskipsModuleNotLoadedwhen the execution already succeeded, since empty/unknown module lists under ACL should not override a good reply. Group-mode rendering passes execution status into that check (not the raw array reply), so successfulTS.RANGEoutput displays instead of the module warning.Reviewed by Cursor Bugbot for commit 75eae6c. Bugbot is set up for automated code reviews on this repo. Configure here.