ZADD with INCR option incurs two different reply types in one command. (Broken contract)

[HeartSaVioR]

I didn't get an answer from Redis Google Groups, so I post to Github issue.

I couldn't follow Redis releases for a while, and see new Redis releases which extends ZADD with various parameters. It is great to add it, without INCR option.

Normally Redis replies ZADD operation to INTEGER reply, which is Integer, but with INCR option Redis replies to Bulk String reply, which is String. (Redis doc says ZADD's return value is INTEGER reply. http://redis.io/commands/zadd)

Now we're having two different return "type" of same operation, which I don't quite understand why you allow it. It is not small issue for driver with static type language, cause we should implement same operation twice (with INCR, without INCR) with different semantic (for example, with INCR, we should convert String to Double to let users enjoy with actual data type).

I strongly think it should be fixed, and there's a way to keep same functionality with safe way.

How about removing INCR option from ZADD and adding NX / XX option to ZINCRBY?

It should be same without making a headache, cause it doesn't change return type whatever combination of option users provide. Since it is already released feature, so deprecating (not removing right now) ZADD with INCR also makes sense for me.

For a reference, Jedis is struggling with this issue: https://github.com/xetorthio/jedis/issues/1067

[HeartSaVioR]

I really don't want to take workaround for client side, since server-client contract is far more better than adding new feature. It should be considered before adopting something new.

Wish to get positive feedback.

[jpe42]

+1

[marcosnils]

:+1:

[chester89]

:+1:

[antirez]

Hello, there is no contract in Redis about the same command returning the same type. Actually even errors are reported as a data type, the error data type in the form "-ERRCODE Some String\n" specifically. Similarly EXEC returns NIL on failed transactions, or an error, or the actual reply of the commands executed as an array. For the sake of it, EVAL and EVALSHA can return every conceivable type.

So I don't thing this is a problem in the Redis side. It is just not the way Java APIs are normally designed that creates a problem here. Proof is that C language bindings such as hiredis don't suffer from the same problem: they return a reply object that you can test for type. The same can be done for Java.

But I don't want to convince you about the API you should use, I never programmed anything in Java so I've really no idea. In this case a wrong design would be, IMHO, to overload with the similar set of options both ZINCRBY and ZADD. Instead I'm trying to centralize the functionalities in ZADD leaving ZINCRBY as an handy alias.

If you want to retain a strongly typed API, I think you should do the following: provide a zadd method as you already do, and provide a zincr method that is implemented in terms of zadd anyway but with a different method fingerprint.

Another alternative you have is to support 95% of the obvious combinations as direct methods, like you do currently, and add a call method which returns a reply object and the user can call when the return value is undetermined.

myreply = redis.call("COMMAND","SOMEARG");

Sorry but I believe that designing the Redis API to accomodate a language which is apparently somewhat too rigid to accomodate for a perfectly valid CS concept of, give me a command, and I reply with a fully specified return value that can be one of A, B, C, D, is not a good idea.

[HeartSaVioR]

@antirez I know we can make additional method to support it, just say that I want to know why it happens.

I didn't use hiredis, but if it returns reply object and if I'm right, it should be interpreted as user side. It means users need to know each commands with combination of options reply type. As you've said, we already did it for EVAL, but IMO it is not a good idea if we extend it to normal operations. It is convenient for Redis (and redis-cli) side, but it is not convenient for user side.

And Redis document for every commands has explaining just one return type. For ZADD, it is not true anymore. Doc says "We described ZADD with INCR option works like ZINCRBY, so just refer ZINCRBY." But now we can achieve ZADD with NX / XX + INCR option, which isn't fully same to ZINCRBY.

I don't know why we make long lasted command to just be alias. Each command has its well defined role, and IMO by role it is more natural to add NX / XX option to ZINCRBY. Now we should check ZADD's field count when with INCR, which doesn't need for ZINCRBY.

But I respect that you're leader of Redis and I believe you're concerning Redis ecosystem. So if you still think it is up to client, I'll follow you.

Just have a feature request as a client developer : Could you add NX / XX option to ZINCRBY? If you're OK to provide it, I will just support that combination and forget about ZADD with INCR option.

ps. I'm curious that you're planning to centralize other commands, for example, SET and INCR, HSET and INCR, etc.

[marcosnils]

I didn't use hiredis, but if it returns reply object and if I'm right, it should be interpreted as user side. It means users need to know each commands with combination of options reply type. As you've said, we already did it for EVAL, but IMO it is not a good idea if we extend it to normal operations. It is convenient for Redis (and redis-cli) side, but it is not convenient for user side.

I do have to agree with @HeartSaVioR comments here. Don't miss understand me, I'm not defending Java or taking any sides in this discussion (in fact I'm not working with Java anymore), but if we take the approach of returning a reply object as you suggested then we need to delegate to the user the responsibility of casting or using Java generics to infer which type the command will return.

One of the design features of Java is that it's a strongly typed language, and since the conception of Jedis we tried to use this advantage to provide our users an API that they can safely use in compile time. @antirez, maybe it's ok for hiredis to make this abstraction because it's mainly used for hi level libraries that are well tested and specifically know which type will every command return.

We "could" provide a generic API which returns a response object (actually we do this with EVAL and EVALSHA as @HeartSaVioR mentioned) but by doing this we force our users to manually handle each possible return type and increasing the probability of having ClassCastExceptions in the users' code.

Again, this is not a "Java vs Redis vs C vs Rust vs Ruby vs Whatever" thing. Quoting @HeartSaVioR one last time, we have the impression that ZADD + INCR + NX / XX it's pretty much the same as adding NX / XX to ZINCRBY which will not require a different return type based on the arguments sent to the command.

I think that this provides a much cleaner API from the Redis perspective as well...

[badboy]

Then why not extend the existing ZINCRBY, implement it on top of ZADD + INCR and be fine with it? This keeps the implementation of zadd as is, thus keeping the return value for it safe, and just extends ZINCRBY.

For the end user it doesn't matter if ZADD + INCR or ZINCRBY is called.

[marcosnils]

@badboy what happens if the user actually want's to do something like ZADD [NX|XX] CH INCR?

We don't have NX, XX or CH in zincrby

[badboy]

They have to be implemented on top of zincrby. The same as if NX and XX would be added to the Redis command ZINCRBY anyway.

[marcosnils]

@badboy agree. When you talked about extending I thought you were saying on Jedis side only. Guess that Redis also can benefit from this by adding NX and XX to ZINCRBY.

[HeartSaVioR]

@marcosnils He says that we can provide ZINCR + [NX | XX] by calling ZADD + [NX | XX] + INCR internally.

@badboy Sorry but again, I know that. But why it should happen as client side? Why client should have semantics?

In order to support multiple versions of Redis we have been trying to avoid implementation has semantic. If Redis changes to break compatibility, Jedis API becomes useless. For now Jedis just calls matched Redis command, that way is that we have been safe to stick.

To tell the truth, I don't understand that there is no contract in Redis about the same command returning the same type. Doc says about return value of command, and ZADD has just one return value, INTEGER. ERROR reply is common so we don't need to describe it so it can't be a example of same command returning two or more type. Then where's the spec for each command? What doc client developer can be safe to refer?

[marcosnils]

@HeartSaVioR I asume that they forgot to update the docs when they added the ZADD options :smile:

[badboy]

@marcosnils @HeartSaVioR Yip, that was somehow forgotten, I will add that in a minute.

[jmorales4]

Found I needed XX today, so THANKS! for putting it in. The code implies that the implementation is supposed to return null when the entry is not present:

jedis-2.8.1-sources.jar!/redis/clients/jedis/Jedis.java line 1416:
    // with nx / xx options it could return null now
    if (newscore == null) return null;

But it throws instead:

redis.clients.jedis.exceptions.JedisDataException: ERR value is not a valid float
    at redis.clients.jedis.Protocol.processError(Protocol.java:117)
    at redis.clients.jedis.Protocol.process(Protocol.java:151)
    at redis.clients.jedis.Protocol.read(Protocol.java:205)
    at redis.clients.jedis.Connection.readProtocolWithCheckingBroken(Connection.java:297)
    at redis.clients.jedis.Connection.getBinaryBulkReply(Connection.java:216)
    at redis.clients.jedis.Connection.getBulkReply(Connection.java:205)
    at redis.clients.jedis.Jedis.zincrby(Jedis.java:1414)

No big deal to me, but I thought you might want to know

[badboy]

@jmorales4 Looks like a misuse or bug in Jedis, not Redis.

[marcosnils]

@jmorales4 can you provide an example where you're getting this error?

[jmorales4]

Ooooops, my bad (he says shyly). Turns out my test Redis wasn't upgraded to 3.0.x. Works great now. Thanks again!

[marcosnils]

@jmorales4 :+1: