Repository navigation
fix(output): forward ByteBuffer error to queued command output (#3960) - #3963
a-TODO-rov merged 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: a38943b64e
ℹ️ 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".
| MultiOutput<String, String> output = new MultiOutput<>(StringCodec.UTF8); | ||
| ErrorTolerantBooleanValueListOutput<String, String> errorTolerantOutput = new ErrorTolerantBooleanValueListOutput<>( | ||
| StringCodec.UTF8); | ||
| Command<String, String, List<io.lettuce.core.Value<Boolean>>> command = new Command<>(CommandType.BF_MADD, |
There was a problem hiding this comment.
In the newly added value-output test, io.lettuce.core.Value is written inline here and again in the assertions despite there being no same-named type to disambiguate. Add an io.lettuce.core.Value import and use the simple Value name so the test follows the repository's required Java import convention.
AGENTS.md reference: AGENTS.md:L154-L156
Useful? React with 👍 / 👎.
![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 using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit dfd59eb. Configure here.
|
|
||
| assertThat(result.wasDiscarded()).isFalse(); | ||
| assertThat(result.<List<Boolean>> get(0)).containsExactly(true, null); | ||
| } |
There was a problem hiding this comment.
Transaction test breaks inherited overloads
Medium Severity
The new bfMAddWithRejectedItemInTransaction test is inherited by cluster and reactive overloads that cannot run it as written. Cluster already disables MULTI tests because MULTI is unavailable, and reactive bfMAdd uses ErrorTolerantBooleanValueListOutput, so exec() returns List<Value<Boolean>> rather than List<Boolean> with raw nulls.
Reviewed by Cursor Bugbot for commit dfd59eb. Configure here.
…#3960) MultiOutput.setError(ByteBuffer) previously decoded the server error to a String before passing it to the currently queued command output via setError(String). Commands returning per-element errors (e.g. BF.MADD, BF.INSERT, TOPK.ADD) use error-tolerant outputs (such as ErrorTolerantBooleanListOutput) which only override setError(ByteBuffer) to convert per-element failures into null entries. Because setError(String) was called instead, the default CommandOutput implementation was invoked, failing the entire command and turning the TransactionResult entry into a RedisCommandExecutionException. Pass the raw ByteBuffer directly to the queued command output via setError(ByteBuffer). Standard CommandOutput instances that do not override setError(ByteBuffer) retain their existing behavior by decoding the buffer in the base class. Signed-off-by: Ronit Gupta <[email protected]>
Signed-off-by: Ronit Gupta <[email protected]>
…putUnitTests Signed-off-by: Ronit Gupta <[email protected]>
dfd59eb to
0b2b8c2
Compare
Problem
In transactions (
MULTI/EXEC),MultiOutput.setError(ByteBuffer)previously decoded server errors toStringbefore passing them to the currently decoded command output viasetError(String).Commands returning per-element errors (e.g.
BF.MADD,BF.INSERT,CF.INSERTNX,TOPK.ADD,TOPK.INCRBY) use error-tolerant outputs (such asErrorTolerantBooleanListOutput) which overridesetError(ByteBuffer)to record individual item failures asnullin the result list. BecausesetError(String)was invoked byMultiOutput, the baseCommandOutputimplementation was triggered, erroneously failing the entire command with aRedisCommandExecutionExceptioneven though Redis applied the other elements.Solution
Forward the raw
ByteBufferdirectly viaqueue.peek().getOutput().setError(error). StandardCommandOutputsubclasses that do not overridesetError(ByteBuffer)preserve identical behavior by decoding the buffer in the base class.Added unit test
shouldForwardByteBufferErrorToErrorTolerantOutputinMultiOutputUnitTests.Fixes #3960.
Note
Medium Risk
Touches transaction reply decoding in a core protocol path; behavior change is narrow but affects how errors propagate for commands inside EXEC.
Overview
Fixes EXEC transaction decoding so per-element Redis errors inside a command reply are handled by the command’s own output instead of failing the whole command.
MultiOutput.setError(ByteBuffer)now passes the raw buffer to the queued command output (setError(error)), rather than decoding it to aStringfirst. That lets error-tolerant list outputs (e.g. forBF.MADD-style mixed success/error arrays) record failures asnullor emptyValueentries while the transaction and command still complete successfully.Adds unit tests that simulate a
MULTI/EXECreply with one success and oneByteBuffererror for bothErrorTolerantBooleanListOutputandErrorTolerantBooleanValueListOutput.Reviewed by Cursor Bugbot for commit 0b2b8c2. Bugbot is set up for automated code reviews on this repo. Configure here.