Breaking changes for `StringSet` from 2.5.43 -> 2.5.61

[WeihanLi]

The following code breaks when upgrade 2.5.43 -> 2.5.61

database.StringSet(_keyName, Base, _expiresIn, When.NotExists);

Error message:

Argument 4: cannot conv
ert from 'StackExchange.Redis.When' to 'bool'

Seemed it's introduced from #2029

[mgravell]

Hmmm; indeed, that will be a code-breaking (but thankfully not binary-breaking) change - we must have overlooked that one, sorry. I think we should change CommandFlags flags to CommandFlags flags = CommandFlags.None (but leave the others as-is), which would allow this previous usage to continue working. Thoughts?

[mgravell]

(as a workaround, you can also just add , CommandFlags.None, which should restore the old usage)

[WeihanLi]

Hi @mgravell , thanks for your quick response.

I think maybe it's more simple to adjust the order of the parameters,

For example:

- bool StringSet(RedisKey key, RedisValue value, TimeSpan? expiry = null, When when = When.Always, CommandFlags flags = CommandFlags.None)
+ bool StringSet(RedisKey key, RedisValue value, TimeSpan? expiry = null, When when = When.Always, CommandFlags flags = CommandFlags.None, bool keepTtl = false)

Another thought is to add a new method such as StringSetAndKeepTtl(maybe a better name)

[mgravell]

Reordering is a binary break, and we've always tried to maintain a convention of flags as the last param.

[slorello89]

Probably needs several smaller overloads, with each ordered sequence of optional parameters. Then in the final version the new parameter should be just prior to the flags.

[erick2red]

This caused some pain for me today. While it works perfectly fine in my development setup in Visual Studio, when I deploy to Azure it crashes with the exception below

Method not found: 'System.Threading.Tasks.Task`1<Boolean> StackExchange.Redis.IDatabaseAsync.StringSetAsync(StackExchange.Redis.RedisKey, StackExchange.Redis.RedisValue, System.Nullable`1<System.TimeSpan>, Boolean, StackExchange.Redis.When, StackExchange.Redis.CommandFlags)'

[NickCraver]

@erick2red Interesting - that's backwards - looking for a new method but using an older assembly? It sounds like you're silently (or with a warning) downgrading somewhere vs. what you're compiling against.

[erick2red]

I'm compiling against the latest version, and I'm deploying a package to Azure with the binaries in it. I think what's happening is that the runtimes are different, the one in my machine versus the one in the cloud service and the method resolution is tripping there. I fixed it by adding the extra parameters to the call site and making sure it wasn't calling the method with the bool keepTtl in it. Somehow, the runtime in Azure can't find that method in the Nuget being uploaded.

[erick2red]

Interestingly enough, it doesn't happen with .NET Core 5. IT happens when running .NET Framework 4.7.2.

[NickCraver]

I pushed a PR to remedy this in #2098. @mgravell good for eyes when you have a chance - I did try going with the simple CommandFlags overload but wanted to add a test suite to be sure and that revealed problems there with ambiguous overloads so...went the adding another overload route. I think this covers all possible source breaks from this.

Sorry about the break here, and thanks for the report!