Back to Stackexchange Redis

SER304: repeated queued operations may suit the variadic overload

docs/rules/SER304.md

3.1.134.8 KB
Original Source

SER304: repeated queued operations may suit the variadic overload

The same command is queued several times over, and one variadic call does the lot - one round-trip, atomic on the server, no transaction needed.

c#
// flagged
var tran = db.CreateTransaction();
_ = tran.SetAddAsync(key, "a");
_ = tran.SetAddAsync(key, "b");
await tran.ExecuteAsync();

// suggested
long added = await db.SetAddAsync(key, new RedisValue[] { "a", "b" });

What it covers

One key, many values - every call must be on the same key:

RepeatedSingle callServer
SetAdd / SetRemoveSetAdd(key, values) / SetRemove(key, values)any
SortedSetAdd / SortedSetRemoveSortedSetAdd(key, entries) / SortedSetRemove(key, members)any
HashSet / HashDeleteHashSet(key, entries) / HashDelete(key, fields)any
ListLeftPush / ListRightPushListLeftPush(key, values) / ListRightPush(key, values)any
SetContainsSetContains(key, values) (SMISMEMBER)6.2

Many keys - the calls must be on different keys:

RepeatedSingle callServer
StringSetStringSet(KeyValuePair<RedisKey, RedisValue>[]) (MSET)any
StringGetStringGet(keys) (MGET)any
KeyDeleteKeyDelete(keys) (DEL)any
KeyExistsKeyExists(keys) (EXISTS)any

Which direction applies is the whole distinction: SADD takes one key and many values, so calls across different keys have no single-command form; MSET takes many keys, so calls on one key are not what this is about. Neither is flagged in the wrong direction.

Most of these variadic forms arrived in Redis 2.4, which predates anything realistically in service, so no version is mentioned. SMISMEMBER at 6.2 is recent enough to say so - see declaring your server version.

What changes when you apply it

This is why it has its own ID rather than sharing SER303: the result changes shape, not just meaning.

  • N calls each returning bool become one returning a long count. You learn how many were added or removed, not which ones.
  • N calls each returning a value become one returning an array (StringGet, or bool[] for SetContains).
  • The individual queued Task<T>s disappear, so anything awaiting them individually needs rewiring.
  • CommandFlags must be carried over verbatim.

If your code genuinely needs to know which of the members was new, the per-call form is the right one and this suggestion is not for you - suppress it.

Cases that are deliberately not flagged

  • Commands queued in a loop. This is the most common way the shape arises in practice, and it stays quiet on purpose: a loop body is one call site, and we cannot show that the key expression is loop-invariant, so we cannot tell a same-key collapse from a per-key one. Guessing would be worse than silence.
  • A key local reassigned between the calls. The keys are compared as source text, which is only sound while the locals hold the same value throughout; a reassignment anywhere in the method means identical text can be two different keys, so the rule stays quiet. This applies to every rule in this family.
  • N x ListLeftPop across keys is not LMPOP. LMPOP pops from the first non-empty key of those given, not from each of them - a different operation, however similar the argument list looks. Same for ZMPOP.
  • Anything with a condition, which is SER300-SER302 territory.
  • Calls carrying an argument the variadic form has no room for. MSET takes one expiry for the whole batch rather than one per key, and the variadic HashSet has no When, so calls that pass those are left alone - collapsing them would silently drop the argument, and in the MSET case leave your keys with no expiry at all.

Plus everything under when these rules stay quiet.

Guidance, not a verdict

This rule is a heuristic. It reads your source text - it cannot see your keys, your server, or what you know about the code - so it is deliberately conservative and stays quiet wherever it is unsure. Everything it flags still works, and will keep working: this is a suggestion, not a defect report.

That conservatism is meant to make a false positive rare, not impossible. If you think the rule has flagged something it should not have, please report it, including the transaction as written. A rule that fires on correct code is a bug in the rule - and one that reaches every consumer of the package - so it is worth fixing rather than quietly suppressing.

Suppressing

Reported as a warning, so TreatWarningsAsErrors builds fail until you act on it or turn it down.

xml
<NoWarn>$(NoWarn);SER304</NoWarn>

or locally:

c#
#pragma warning disable SER304