Skip to content

Cannot scan binary keys with Jedis #2006

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.

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

Closed
n12c opened this issue Mar 17, 2021 · 3 comments
Closed

Cannot scan binary keys with Jedis #2006

n12c opened this issue Mar 17, 2021 · 3 comments
Assignees
Labels
type: enhancement A general enhancement

Comments

@n12c
Copy link

n12c commented Mar 17, 2021

In org.springframework.data:spring-data-redis:2.4.5, in org.springframework.data.redis.connection.jedis.JedisKeyCommands#scan(long, org.springframework.data.redis.core.ScanOptions)[1] we round trip keys through String, corrupting them. This makes it impossible scan top level keys using Jedis.

Specifically, we call redis.clients.jedis.Jedis#scan(java.lang.String, redis.clients.jedis.ScanParams) which decodes keys as strings, interpreting the binary data as UTF-8. On the next line, we encode the strings back into byte arrays using JedisConverters.stringListToByteList(), which renders UTF-8 again. This generally results in any non-UTF-8 sequences in a binary key being replaced with efbfbd (the UTF-8 serialization of the Unicode replacement character).

Instead, we should call redis.clients.jedis.BinaryJedis#scan(byte[], redis.clients.jedis.ScanParams) and remove the second conversion using JedisConverters.stringListToByteList().

[1] https://github.com/spring-projects/spring-data-redis/blob/master/src/main/java/org/springframework/data/redis/connection/jedis/JedisKeyCommands.java#L159

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 17, 2021
@mp911de
Copy link
Member

mp911de commented Mar 18, 2021

SCAN is somewhat contradicting the idea of non-string keys. Any format other than UTF-8 strings can potentially contain an asterisk and that's why we went for accepting String as scan input parameter. We cannot accept a typed value (e.g. ScanOptions<K>) because there are no means to express a prefix/suffix/contains match clause.

We could switch pattern to a byte array internally and call the binary SCAN method to avoid key name to UTF-8 conversion. I think we need to do the same for Lettuce as Lettuce's ScanArgs use a string to express the match clause.

@mp911de mp911de added type: enhancement A general enhancement theme: 4.0 and removed status: waiting-for-triage An issue we've not yet triaged labels Mar 18, 2021
@n12c
Copy link
Author

n12c commented Mar 18, 2021

Interesting, I hadn't considered the globbing implications. How does that comport with the implementation of SSCAN[1] though? It doesn't do the string conversion and handles binary set values without problems; the implementation there looks exactly like what (I thought) the SCAN implementation should. But, the implementation does pass through the MATCH subcommand as the SSCAN command[2] does support it. Is SSCAN matching something that only circumstantially works when the binary data involved is actually UTF-8 data?

[1] https://github.com/spring-projects/spring-data-redis/blob/master/src/main/java/org/springframework/data/redis/connection/jedis/JedisSetCommands.java#L275
[2] https://redis.io/commands/sscan

@mp911de
Copy link
Member

mp911de commented Mar 19, 2021

For SSCAN, we already obtain the key name in its binary form. String matching happens on the set member level so prefixes of binary set members may get encoded improperly. The sscan method we invoke returns its results as byte[].

@mp911de mp911de self-assigned this Apr 21, 2021
@mp911de mp911de added this to the 2.6 M1 (2021.1.0) milestone Apr 21, 2021
mp911de added a commit that referenced this issue Apr 21, 2021
We now accept a byte array as pattern for scan operations in addition to String patterns.

Also, use binary Jedis scan method to avoid bytes to String conversion.

Closes #2006
mp911de added a commit that referenced this issue Apr 21, 2021
We now accept a byte array as pattern for scan operations in addition to String patterns.

Also, use binary Jedis scan method to avoid bytes to String conversion.

Closes #2006
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants