Skip to content

Fix ObjectOutput handling of top-level scalar responses#3861

Open
cfcromn wants to merge 3 commits into
redis:mainfrom
cfcromn:fix/object-output-top-level-bulk-string
Open

Fix ObjectOutput handling of top-level scalar responses#3861
cfcromn wants to merge 3 commits into
redis:mainfrom
cfcromn:fix/object-output-top-level-bulk-string

Conversation

@cfcromn

@cfcromn cfcromn commented Jul 23, 2026

Copy link
Copy Markdown

Fixes #3860

Summary

ObjectOutput initialized its top-level result with Collections.emptyList().
When a top-level scalar response (for example, a bulk string returned from
EVAL with ScriptOutputType.OBJECT) was decoded, setValue(...) attempted
to append to the immutable list, resulting in an UnsupportedOperationException.

This change lazily initializes a mutable list when the first top-level scalar
value is received, preserving the existing behavior for nested collections.

Tests

  • Added regression tests for top-level bulk string responses
  • Added regression tests for top-level boolean responses
  • Added regression tests for top-level double responses

Validation

  • mvn test -Dtest=ObjectOutputUnitTests
  • git diff --check

Note

Low Risk
Small, localized change to RESP3 decoding with new unit tests; nested map/list behavior is unchanged.

Overview
Fixes top-level scalar RESP3 responses (e.g. bulk strings from EVAL with ScriptOutputType.OBJECT) that previously crashed with UnsupportedOperationException because the decoder tried to append the first value to the immutable placeholder list from Collections.emptyList().

ObjectOutput now assigns the first decoded value directly to output and marks initialization complete, instead of creating an ArrayList on integer set() and appending. Nested arrays/maps still use the existing multi / multiMap paths unchanged.

Regression coverage adds unit tests for top-level bulk string, boolean, double, and integer decoding via RedisStateMachine.

Reviewed by Cursor Bugbot for commit eb1b5c0. Bugbot is set up for automated code reviews on this repo. Configure here.

private void setValue(Object value) {

if (!initialized) {
output = new ArrayList<>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we assign the actual value here ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updated accordingly.

Top-level RESP3 scalar values are now assigned directly to output instead of being wrapped in a List.

private void setValue(Object value) {

if (!initialized) {
output = new ArrayList<>();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If value is non-collection type, could we not initialize output as a List? Instead, just assign value to ouput?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion!

Updated the implementation to assign top-level non-collection RESP3 values directly to output.

Nested collection responses are unchanged because multi() / multiMap() still initialize the collection container before nested values are accumulated.

I also added regression tests covering top-level bulk string, boolean, double, and integer responses.

@zhengjiudan zhengjiudan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

UnsupportedOperationException when calling eval

3 participants