Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Confusing nullability of IDatabase.ListRightPop() (and possibly more?) #2697

Closed
bdach opened this issue Apr 15, 2024 · 4 comments · Fixed by #2702
Closed

Confusing nullability of IDatabase.ListRightPop() (and possibly more?) #2697

bdach opened this issue Apr 15, 2024 · 4 comments · Fixed by #2702

Comments

@bdach
Copy link
Contributor

bdach commented Apr 15, 2024

The interface signature of the method in question is:

StackExchange.Redis/src/StackExchange.Redis/Interfaces/IDatabase.cs

Lines 1082 to 1091 in cb8b20d

/// <summary>
/// Removes and returns count elements from the end the list stored at key.
/// If the list contains less than count elements, removes and returns the number of elements in the list.
/// </summary>
/// <param name="key">The key of the list.</param>
/// <param name="count">The number of elements to pop</param>
/// <param name="flags">The flags to use for this operation.</param>
/// <returns>Array of values that were popped, or nil if the key doesn't exist.</returns>
/// <remarks><seealso href="https://redis.io/commands/rpop"/></remarks>
RedisValue[] ListRightPop(RedisKey key, long count, CommandFlags flags = CommandFlags.None);

The <returns> tag says that the method can return nil, 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

public RedisValue[] ListRightPop(RedisKey key, long count, CommandFlags flags = CommandFlags.None)
{
var msg = Message.Create(Database, flags, RedisCommand.RPOP, key, count);
return ExecuteSync(msg, ResultProcessor.RedisValueArray, defaultValue: Array.Empty<RedisValue>());
}

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 like

StackExchange.Redis/src/StackExchange.Redis/Interfaces/IDatabase.cs

Lines 1492 to 1499 in cb8b20d

/// <summary>
/// Removes and returns a random element from the set value stored at key.
/// </summary>
/// <param name="key">The key of the set.</param>
/// <param name="flags">The flags to use for this operation.</param>
/// <returns>The removed element, or nil when key does not exist.</returns>
/// <remarks><seealso href="https://redis.io/commands/spop"/></remarks>
RedisValue SetPop(RedisKey key, CommandFlags flags = CommandFlags.None);

RedisValue is a struct, so going off the type annotations alone this can at worst return a default(RedisValue), which I wouldn't generally interpret to be the same as nil.
@bdach bdach mentioned this issue Apr 15, 2024
Merged
@mgravell
Copy link
Collaborator

Short version: yes, I think the comments could be clearer

Longer version:

  • for the single value version, there is an instance property that allows "null" (not C# null, but null nonetheless) to be conveyed
  • for arrays, I suspect you're right and we always return a non-empty array

@NickCraver
Copy link
Collaborator

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.

@bdach
Copy link
Contributor Author

bdach commented Apr 16, 2024

Thanks for the confirmation. I can probably get a PR going for this if it helps?

@NickCraver
Copy link
Collaborator

@bdach That'd be very welcome :)

bdach added a commit to bdach/StackExchange.Redis that referenced this issue Apr 22, 2024
@bdach
Replace all occurrences of "nil" in IDatabase(Async) xmldoc with le…
f4647e0 
…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.
@bdach bdach mentioned this issue Apr 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mgravell @NickCraver @bdach