Implement GETEX command

[benbryant0]

Resolves #1740

Should add support for all current options of GETEX.

Questionable (non-)decisions (opinions wanted!):

• I called the new methods StringGet[Async] as the behaviour is most similar to plain StringGet and I think the added expiry functionality is communicated by the parameters.
• The relative expiry overload takes TimeSpan? while the absolute overload takes non-nullable DateTime. I don't like it, but I did this as I realised I'd have to cast to escape ambiguity: (TimeSpan?)null 🤮. But maybe consistency is more important?
• I looked in the tests folder and cried a little; I have no idea what I'm supposed to update there. I just bumped the Redis version in Dockerfile since Docker is easy and you seem to use it in your GitHub actions.
• I placed the tests in Strings, even though they involve expiry and the tests themselves go against the pattern in that file a bit. Felt better than bloating something catch-all like BasicOps.

[NickCraver]

I see we need async tests here, but main point that I'm hung on is the API - want to sanity check method names. IMO, TimeSpan? feels weird - if we're not setting an expiry shouldn't the user be calling the existing StringGet? I wonder if we should call these StringGetSetExpiry or something that's a clearer usage pattern, looking at consumer code you could easily assume it's somehow related to a timeout or respecting an existing TTL or something, and not setting a value (admittedly, this is confusing on the base Redis API we're hitting as well).

The async test versions are straightforward, but thoughts on overload nullability and general naming? cc @mgravell

[benbryant0]

Calling with null for the TimeSpan? parameter isn't "not setting an expiry", but removing any existing expiry; it maps to using the PERSIST option.

Good point about consuming code being unclear; StringGetSetExpiry sounds reasonable to me. If we go with that then I think it also seems reasonable to remove the nullability from the relative expiry overload and we could have that as a separate method, named something like StringGetRemoveExpiry.

[NickCraver]

I'll work on getting this updated (my fault we haven't gotten to everything) as part of #2055. Current thinking here: slim down to only the RedisValue StringGetSetExpiry(RedisKey key, TimeSpan expiry, CommandFlags flags = CommandFlags.None); overload - instead of leaving DateTime conversion in the lib's hands (or assuming what the Redis server has), it's best to stick with the timespan-only overloads as we do elsewhere. Since TimeSpan.Zero also serves the removal case, we can remove the multiple overloads for a single Redis command here. I'll work on getting main merged in here :)

[benbryant0]

It seems a shame to lose support for the EXAT/PXAT options, but if you think that's best I see no issue with it.

Just to try and clarify some things again though: If I remember correctly my intent was to stay consistent with KeyExpireAsync. The two overloads there map to EXPIRE and EXPIREAT, but the difference between those commands is represented as options for GETEX. Having two optional parameters of the form TimeSpan? relativeExpiry = null, DateTime? absoluteExpiry = null just seemed like a nightmare, so I went with an overload (but in hindsight maybe a simple union value type could've made that clean).

I think there's also still misunderstanding about 'removal' and the TimeSpan? parameter. As I said, removing refers to removing the expiry, not the key (hence the suggested alternative name '...RemoveExpiry'). The TimeSpan? parameter & behaviour was consistent with KeyExpireAsync (the GetStringGetExMessage methods were almost exact copies of GetExpiryMessage).

Maybe some example translations might help clarify my original intent
StringGetAsync("key", TimeSpan.FromSeconds(1)) == GETEX key EX 1
StringGetAsync("key", (TimeSpan?)null) == GETEX key PERSIST
StringGetAsync("key", TimeSpan.MaxValue) == GETEX key PERSIST
StringGetAsync("key", DateTime.Parse("2022-04-01")) == GETEX key EXAT 1648771200
StringGetAsync("key", DateTime.MaxValue) == GETEX key PERSIST
My understanding is that this all matches the use of KeyExpireAsync.

[NickCraver]

@benbryant0 Okay I gotcha - yes good clarifications, and some was lost in merge noise here. Will absolutely re-evaluate with fresh eyes. I really appreciate the follow-up here.

[NickCraver]

@benbryant0 Would you mind eyes on latest when time allows please? I didn't match KeyExpire exactly because both versions being nullable makes Method(key, null) ambiguous - so instead I made the DateTime overload non-nullable.

There's code that can be shared here on converting to milliseconds but will do that in a follow-up PR to keep things tidy.

Thanks for everything here, it's much appreciated!

[slorello89]

Hey @benbryant0 - looks pretty good, just a few things.

1: I think there should be another API specifically for persistence rather than passing in null. I worked backward a bit and read through the code rather than reading your back & forth, I think 3 methods per sync/async would make the most sense here. Fundamentally the command allows 3 things, persist a given amount of time, persist until a time, and persit indefinitely. So this should either be 1 enum Operated method

StringGetSetExpiry(RedisKey key, ExpiryType type, Timespan? timespan = null, Timestamp? timestamp = null, Commandflags flags = CommandFlags.None)

or just split it out into 3 methods, which IMO is cleaner and will result in fewer paths, and fewer need for the library to toss exceptions. Sort of thinking the null timespan setting it to persist forever is confusing (I might expect it to have some reasonable default value e.g. 60 seconds, or something configured).

2: There's some extra logic to pull out seconds vs milliseconds I don't think necessary, just use milliseconds, I believe Redis will just use Milliseconds regardless, and you're going to send a 64 bit integer no matter what.

3: Very minor stylistic comments (casing and Assertions) in the tests. Looks like there is one missing test to exercise the off-nominal ArgumentException we're adding above.

[benbryant0]

LGTM!

Thanks for taking another pass at it @NickCraver and applying all the suggestions. Such an improvement over the original.

And thanks @slorello89 for the good input. I agree that a separate method would be nicer; but I think consistency is more important (and this is already a very big interface).

Good point regarding ms vs seconds; if I had even thought of it I wouldn't have done it due to lack of experience with Redis, now I know!

[slorello89]

Given there's agreement on the API, LGTM 👍