Skip to content

StreamEntry (XREAD) does not support binary because it uses Map<String, String> #3566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
raylau1234 opened this issue Oct 3, 2023 · 8 comments · Fixed by #4152
Closed

StreamEntry (XREAD) does not support binary because it uses Map<String, String> #3566

raylau1234 opened this issue Oct 3, 2023 · 8 comments · Fixed by #4152
Labels
good-first-issue

Comments

@raylau1234
Copy link

raylau1234 commented Oct 3, 2023 •
edited
Loading

StreamEntry uses a Map<String, String> for fields.

As a result, binary data which contains illegal utf-8 sequences get mangled.

Reproduce by using XADD with a key or value which contains an illegal utf-8 sequence like 0xc3 0x28.

Retrieve via XREAD.

Expected result -- API to get byte array instead of a String, which is lossy with illegal utf-8 sequences.
@sazzad16
Copy link
Contributor

sazzad16 commented Oct 3, 2023

First of all, what is XGET?

Secondly, there are xrange(byte[] key, ...) methods. Don't these help?

@raylau1234
Copy link
Author

Sorry, I meant XREAD.

You are correct on the alternate XRANGE signature. But I am really using XREAD. Let me update the title and to clarify.... XREAD only is problematic.

@raylau1234 raylau1234 changed the title StreamEntry (XRANGE / XGET) does not support binary because it uses Map<String, String> StreamEntry (XREAD) does not support binary because it uses Map<String, String> Oct 3, 2023
@raylau1234
Copy link
Author

I see there are alternate signatures for xread as well. Let me see of those help. I found the StreamEntry ones first. Give me a few minutes to explore and will udpate.

@raylau1234
Copy link
Author

OK, List<byte[]> xread(XReadParams, Entry<byte[], byte[]>... streams) is a possible workaround, but is cumbersome bec I need to deserialize the byte into the map of key / message id / values. i.e. I need to replicate everything that List<Map, Entry<String, List<StreamEntry>>> xread(XReadParams, Map<String, StreamEntryID>) is doing in terms of decoding with STREAM_READ_RESPONSE, STREAM_ENTRY_LIST, etc. (but skipping the SafeEncoder.encode() and keeping the binary there).

@thachlp thachlp mentioned this issue Apr 6, 2024
Closed
@kmularise
Copy link

Hello, I noticed that there was a draft PR (#3803) opened for this issue quite a while ago, but it hasn’t been updated in some time.

I’m interested in picking up this work and continuing from where it left off — possibly by improving the previous approach or starting fresh while keeping backward compatibility in mind.

Would it be okay if I proceed with a new PR for this? Please let me know if there are any concerns or preferences before I begin

@thachlp
Copy link
Contributor

thachlp commented Apr 7, 2025

@kmularise sure, please open the new pr for this issue, I will close mine 🙇

@kmularise
Copy link

@thachlp Thanks for the clarification and for allowing others to pick this up.
I’ll start working on a new PR for this issue. Appreciate the prior work done on #3803!

YoHanKi added a commit to YoHanKi/jedis that referenced this issue Apr 27, 2025
@YoHanKi
Add binary stream support for XREAD and XREADGROUP (redis#3566)
e5829f1 
- Created StreamEntryBinary class to support binary data with Map<byte[], byte[]>
- Added xreadBinary, xreadBinaryAsMap, xreadGroupBinary, and xreadGroupBinaryAsMap methods to StreamBinaryCommands
- Implemented binary stream builders in BuilderFactory
- Added implementation in Jedis and UnifiedJedis classes
- Created BinaryStreamEntryTest to verify binary stream functionality
YoHanKi added a commit to YoHanKi/jedis that referenced this issue May 8, 2025
@YoHanKi
Add parameterized binary stream tests for RESP2/RESP3 in Jedis, Clust…
c82cca9 
…er, and Pooled clients TEST (redis#3566)

- Introduce `BinaryStreamEntryTest` and `BinaryStreamsCommandsTestBase` following the unified test pattern
- Add `ClusterBinaryStreamsCommandsTest` and `PooledBinaryStreamsCommandsTest` to cover JedisCluster and JedisPooled
- Parameterize tests to run under both RESP2 and RESP3 protocols
- Format BuilderFactory to conform to project formatter rules
YoHanKi added a commit to YoHanKi/jedis that referenced this issue May 10, 2025
@YoHanKi
Switch to StreamEntryID in binary stream tests and add illegal UTF-8 …
88a8ff4 
…sequence test (redis#3566)

- Refactor StreamsBinaryCommandsTest: replace Map.Entry<byte[], byte[]> with Map.Entry<byte[], StreamEntryID> for xreadBinary, xreadBinaryAsMap, xreadGroupBinary and related methods

- Update tests to assert on StreamEntryBinary IDs and byte-array payloads accordingly

- Add a new test case covering an illegal UTF-8 sequence (0xc3 0x28) to validate correct binary handling (issue redis#3566)
YoHanKi added a commit to YoHanKi/jedis that referenced this issue May 10, 2025
@YoHanKi
Add Map<byte[], StreamEntryID> support and binary stream tests (redis…
24c5510 
…#3566)

- Introduce a Map<byte[], StreamEntryID> parameter for binary stream commands

- Duplicate all existing test cases for xread, xreadAsMap, xreadGroup and xreadGroupAsMap from StreamsCommandsTest.java and replace them with xreadBinary, xreadBinaryAsMap, xreadGroupBinary and xreadGroupBinaryAsMap in StreamsBinaryCommandsTest
YoHanKi added a commit to YoHanKi/jedis that referenced this issue May 10, 2025
@YoHanKi
Modify Map<byte[], StreamEntryID> support and binary stream tests (re…
6fdcf85 
…dis#3566)

- Duplicate all existing test cases for xread, xreadAsMap, xreadGroup and xreadGroupAsMap from StreamsCommandsTest.java and replace them with xreadBinary, xreadBinaryAsMap, xreadGroupBinary and xreadGroupBinaryAsMap in StreamsBinaryCommandsTest
YoHanKi added a commit to YoHanKi/jedis that referenced this issue May 17, 2025
@YoHanKi
Add JedisByteMap implementation (redis#3566)
5b54a64 
- Implemented a new JedisByteMap<T> class similar to the existing JedisByteHashMap to properly handle byte array keys in Map<byte[], T> operations
- This implementation ensures that `get(byte[])` operations work correctly by using proper byte array equality comparison instead of reference comparison
- Fixes issues when using byte arrays as keys in stream-related operations
YoHanKi added a commit to YoHanKi/jedis that referenced this issue May 17, 2025
@YoHanKi
Add JedisByteMapTest (redis#3566)
7ddf39c 
- Added tests for JedisByteMap similar to existing JedisByteHashMap tests
YoHanKi added a commit to YoHanKi/jedis that referenced this issue May 20, 2025
@YoHanKi
Changed Map<byte[], byte[]> in STREAM_ENTRY_BINARY_LIST to JedisByteH…
246123d 
…ashMap (redis#3566)

- Changed Map<byte[], byte[]> in STREAM_ENTRY_BINARY_LIST to JedisByteHashMap.
ggivo added a commit that referenced this issue May 27, 2025
@YoHanKi @ggivo
Add binary stream support for XREAD and XREADGROUP (#3566) (#4152)
33dd489 
* Add binary stream support for XREAD and XREADGROUP (#3566)

- Created StreamEntryBinary class to support binary data with Map<byte[], byte[]>
- Added xreadBinary, xreadBinaryAsMap, xreadGroupBinary, and xreadGroupBinaryAsMap methods to StreamBinaryCommands
- Implemented binary stream builders in BuilderFactory
- Added implementation in Jedis and UnifiedJedis classes
- Created BinaryStreamEntryTest to verify binary stream functionality

* Add parameterized binary stream tests for RESP2/RESP3 in Jedis, Cluster, and Pooled clients TEST (#3566)

- Introduce `BinaryStreamEntryTest` and `BinaryStreamsCommandsTestBase` following the unified test pattern
- Add `ClusterBinaryStreamsCommandsTest` and `PooledBinaryStreamsCommandsTest` to cover JedisCluster and JedisPooled
- Parameterize tests to run under both RESP2 and RESP3 protocols
- Format BuilderFactory to conform to project formatter rules

* Switch to StreamEntryID in binary stream tests and add illegal UTF-8 sequence test (#3566)

- Refactor StreamsBinaryCommandsTest: replace Map.Entry<byte[], byte[]> with Map.Entry<byte[], StreamEntryID> for xreadBinary, xreadBinaryAsMap, xreadGroupBinary and related methods

- Update tests to assert on StreamEntryBinary IDs and byte-array payloads accordingly

- Add a new test case covering an illegal UTF-8 sequence (0xc3 0x28) to validate correct binary handling (issue #3566)

* Add Map<byte[], StreamEntryID> support and binary stream tests (#3566)

- Introduce a Map<byte[], StreamEntryID> parameter for binary stream commands

- Duplicate all existing test cases for xread, xreadAsMap, xreadGroup and xreadGroupAsMap from StreamsCommandsTest.java and replace them with xreadBinary, xreadBinaryAsMap, xreadGroupBinary and xreadGroupBinaryAsMap in StreamsBinaryCommandsTest

* Modify Map<byte[], StreamEntryID> support and binary stream tests (#3566)

- Duplicate all existing test cases for xread, xreadAsMap, xreadGroup and xreadGroupAsMap from StreamsCommandsTest.java and replace them with xreadBinary, xreadBinaryAsMap, xreadGroupBinary and xreadGroupBinaryAsMap in StreamsBinaryCommandsTest

* Add JedisByteMap implementation (#3566)

- Implemented a new JedisByteMap<T> class similar to the existing JedisByteHashMap to properly handle byte array keys in Map<byte[], T> operations
- This implementation ensures that `get(byte[])` operations work correctly by using proper byte array equality comparison instead of reference comparison
- Fixes issues when using byte arrays as keys in stream-related operations

* Add JedisByteMapTest (#3566)

- Added tests for JedisByteMap similar to existing JedisByteHashMap tests

* Changed Map<byte[], byte[]> in STREAM_ENTRY_BINARY_LIST to JedisByteHashMap (#3566)

- Changed Map<byte[], byte[]> in STREAM_ENTRY_BINARY_LIST to JedisByteHashMap.

* Remove varargs alternatives

 There is already discrepancy in API between
 `xread` binary variant & `xread` string one.
 Binary one accepts varargs for requested streams, while the string variant expects them to be provided as `Map`.

  Going forward xread binary methods should use Map. This commit also marks existing `List<Object> xread`  one as deprecated in favor of newly introduced methods
  for type safety and better stream entry parsing.

* Add xreadBinary and xreadGroupBinary methods to PipelineBase

   * Deprecate List<Object> xreadXXX in favor of newly introduced typed methods

* Clean up

   * Java docs
   * STREAM_ENTRY_BINARY not used

* Fix BinaryStreamEntryTest test

* Add BinaryStreamsPipeline tests

  Added tests to cover Pipeline command executions.
  UnifiedJedis BinaryStreamsCommand test clean up and attempt to simplify focused on testing xreadBinary and xreadGroupBinary methods .

* Test cleanup & formating

   * Merged & refactored a bit Jedis BinaryStreamEntryTest.java & StreamsBinaryCommandsTest to match UnifiedJedis StreamsBinaryCommandsTestBase.

---------

Co-authored-by: ggivo <[email protected]>
@ggivo ggivo linked a pull request May 27, 2025 that will close this issue
Add binary stream support for XREAD and XREADGROUP (#3566) #4152
Merged
@ggivo
Copy link
Collaborator

ggivo commented May 27, 2025

Resolved with PR #4152

@ggivo ggivo closed this as completed May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good-first-issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Add binary stream support for XREAD and XREADGROUP (#3566) YoHanKi/jedis
5 participants
@raylau1234 @sazzad16 @thachlp @kmularise @ggivo