New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Confusing nullability of IDatabase.ListRightPop()
(and possibly more?)
#2697
Comments
Short version: yes, I think the comments could be clearer Longer version:
|
I remember when we designed here - we already returned a non-empty array to prevent breaks for a lot of scenarios and definitely didn't want to add any breaks. Agreed, it's likely we want to change the docs here. |
Thanks for the confirmation. I can probably get a PR going for this if it helps? |
@bdach That'd be very welcome :) |
bdach
added a commit
to bdach/StackExchange.Redis
that referenced
this issue
Apr 22, 2024
…ss ambiguous alternatives Closes StackExchange#2697. All of the replacements were tested to be correct via simple programs in combination with a local redis instance. Notably, there is one worrying nit; in testing it turns out that the `IDatabase.List{Left,Right}Pop(RedisKey, long, CommandFlags)` overload _can_ actually return null, contrary to its nullability annotations. This occurs on missing key; in that case redis replies Nil reply: if the key does not exist. as per https://redis.io/docs/latest/commands/lpop/, which then at https://github.com/StackExchange/StackExchange.Redis/blob/cb8b20df0e2975717bde97ce95ac20e8e8353572/src/StackExchange.Redis/ResultProcessor.cs#L1546-L1547 and later at https://github.com/StackExchange/StackExchange.Redis/blob/cb8b20df0e2975717bde97ce95ac20e8e8353572/src/StackExchange.Redis/ExtensionMethods.cs#L339-L341 turns into a `null`. I briefly attempted to rectify this, but the `RedisValueArrayProcessor` poses a problem here, as changing it to derive `ResultProcessor<RedisValue[]?>` causes the solution to light up in red, and I'd rather not mess with that as a first contribution.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The interface signature of the method in question is:
StackExchange.Redis/src/StackExchange.Redis/Interfaces/IDatabase.cs
Lines 1082 to 1091 in cb8b20d
The
<returns>
tag says that the method can returnnil
, but the actual method signature says that it can't. (This came up because there was actually a null-check in our code, and it started displaying a warning with a package bump.)Judging by the implementation of the method I found and looked at for 5 minutes:
StackExchange.Redis/src/StackExchange.Redis/RedisDatabase.cs
Lines 1270 to 1274 in cb8b20d
it appears that the method signature is probably correct and the documented return value should be changed to "empty array" instead? That said because I took 5 minutes to look at this, I'm not gonna attempt to claim correctness of that without confirmation.
It would probably be a good idea to grep through all usages of
nil
in the interface xmldocs and double-check them too because I see some more suspect ones likeStackExchange.Redis/src/StackExchange.Redis/Interfaces/IDatabase.cs
Lines 1492 to 1499 in cb8b20d
RedisValue
is a struct, so going off the type annotations alone this can at worst return adefault(RedisValue)
, which I wouldn't generally interpret to be the same asnil
.The text was updated successfully, but these errors were encountered: