Refactor script content argument types to String and byte[] instead of V (value type)

[danielsomekh]

Bug Report

Script load should take in a String, not an argument of type T.

Current Behavior

For the sync, async, and reactive apis command interfaces, the scriptLoad method takes in a script of type V, instead of a string. As redis can only eval with a lua script that is of type String, it does not make sense to use a generic.

Input Code

https://github.com/lettuce-io/lettuce-core/blob/c98108dd7fbe9bee8702e468efff4b085a976374/src/main/java/io/lettuce/core/api/sync/RedisScriptingCommands.java#L109

should be changed to String scriptLoad(String script);

[mp911de]

Thanks for the report. I have been always torn on this one since a script isn't exactly a String but rather a sequence of bytes that has a character-like representation.

The issue with String is that we need to convert it into a binary form to send it to Redis. With the growth and inclusion of various languages and charsets, we face an encoding issue and this boils down to: What is the correct encoding for a given script? Is it ASCII? UTF-8? ISO-8559-1? JIS?

When representing the script as V (value type), we give at least users the possibility to influence the encoding.

[danielsomekh]

Thanks for the quick response!

While I agree the script can be encoded in various ways, forcing it to the value type V severly limits the functionality of using eval. Ex. if you have a <String, Integer> encoding, the client can't use the connection for scripts because a script can't be of type Integer. In my opinion, the type of the script is completely independent of the keys and values it is operating on and returning. Additionally, this behavior is inconsistent with eval, as eval takes in a String.
https://github.com/lettuce-io/lettuce-core/blob/master/src/main/java/io/lettuce/core/api/sync/RedisScriptingCommands.java#L54

It may make more sense to make this type a generic for a given set of encodings. I'm happy to take a stab at the implementation.

Additionally (and let me know if I should open a separate issue), I'm not sure using V as the type for values is the best approach here.
https://github.com/lettuce-io/lettuce-core/blob/master/src/main/java/io/lettuce/core/api/sync/RedisScriptingCommands.java#L77
As the only use for values is in the script itself (I think), values should have the same type as the script.

[mp911de]

We could consider using String as script type (and probably provide a byte[] variant) if we keep the encoding of the script configurable (e.g. in ClientOptions).

We can apply such a change with a major release only to not break existing application code and so this ticket could be a candidate for Lettuce 6.

Feel free to open a new ticket for the second issue to not mix topics.

[danielsomekh]

Sounds great, thanks!