ExpireTime and PExpireTime should return times instead of durations

[costela]

Expected Behavior

redis.ExpireTime and redis.PExpireTime should return redis.TimeCmd.

Current Behavior

redis.ExpireTime and redis.PExpireTime should return redis.DurationCmd.

Possible Solution

Ideally both functions (intruduced only recently in #2426) would return redis.TimeCmd to avoid the necessary conversion and possible confusion of returning a duration that actually represents an absolute unix timestamp.

This would however break the v9 API 😞

Alternatively, the discrepancy should be clearly explained in the API, warning about the need for conversion.

[SoulPancake]

Good note.
Redis protocol returns integers and doesn't differentiate between timestamps and durations I'm guessing
Do you have any ideas to have a backwards compatible solution? @costela

[costela]

Unfortunately no good ones.

Adding new methods and deprecating old ones is a possibility, but I suspect that would just cause more confusion for the consumers of the lib.

Maybe just create a v10 Milestone and start tracking these sort of issues?
And meanwhile adding an explicit note to the docs about the need to interpret the results with time.Unix() and time.UnixMilli().

[costela]

can someone please add the v10 label to this issue? 😇 🙏

[martin-sucha]

You can't use redis.TimeCmd directly for EXPIRETIME/PEXPIRETIME because redis.TimeCmd expects a two item list (seconds, microseconds) instead of a single integer that EXPIRETIME/PEXPIRETIME return.

There is one more issue with the current methods — the redis.DurationCmd multiplies the value by time.Second for ExpireTime and time.Millisecond for PExpireTime, so you can't just pass it to time.Unix or time.UnixMilli, you have to divide by those values first. Note that this reduces the possible values that can be represented.

And you can't directly compare the result to -1 or -2 values that have special meaning.

The v10 API should also account for the -1 and -2 integer replies. Whatever type the new methods return, there should probably be some way to check for these. Maybe a completely new type ExpireResult should be introduced? Or should it return some special errors?

Is v10 planned anytime soon? If not, I still suggest adding ExpireTimeV2 and PExpireTimeV2 methods, since the current methods are not really usable at all.

In the mean time, it seems to me the best way to call PEXPIRETIME command is with a generic Do call:

val, err := client.Do(ctx, "PEXPIRETIME", key).Int64()
if err != nil {
	return time.Time{}, err
}
switch val {
	case -1:
		return time.Time{}, ErrNoExpiration
	case -2:
		return time.Time{}, ErrKeyDoesNotExist
	default:
		return time.UnixMilli(val), nil
}

[mathewTH]

For those wanting to simply workaround this issue.
It seems that time.Unix(0, int64(client.ExpireTime(...).Val())) will provide the correct time.

Regarding the limitation of values that can be represented. time.Duration is limited to approx 290 years. So expiry time up until around the year 2260 should be represented ok.

Keep in mind I'm currently using this only in test code, in other code I may make a safer choice (particularly given the special error values).

This issue still needs a v10 label, and to be documented on the methods.

[ndyakov]

Added v10 label and assigned to myself.