Back to Stackexchange Redis

SER300: transaction may be replaceable by a conditional argument

docs/rules/SER300.md

3.1.133.8 KB
Original Source

SER300: transaction may be replaceable by a conditional argument

A transaction whose only job is to make one command conditional can usually be replaced by that command's own when: argument - a single round-trip that cannot abort under contention.

c#
// flagged
var tran = db.CreateTransaction();
tran.AddCondition(Condition.KeyNotExists(key));
_ = tran.StringSetAsync(key, value);
await tran.ExecuteAsync();

// suggested
await db.StringSetAsync(key, value, when: When.NotExists);

The conditional forms have existed as long as the commands have, so unlike SER301 this needs no particular server version.

Why this is worth changing

AddCondition is implemented with WATCH. The transaction takes two round-trips (watch and check, then MULTI/EXEC), and it can abort: if another client touches the key in between, Execute() returns false and correct code has to retry. The conditional command is one round-trip and the server evaluates the condition atomically, so there is no abort to handle.

What changes when you apply it

Read this before rewriting - the collapsed form is not a drop-in for every caller.

  • The result means something different. tran.Execute() returns "the conditions held and the commands ran". The single command returns its own result, which for this rule usually coincides (StringSet with When.NotExists returns whether it set) but is not the same thing by definition.
  • The queued Task<T> goes away. If you awaited the task from the queued command, await the single command instead; there is no longer a separate "did the transaction commit" answer to check first.
  • CommandFlags must be carried over verbatim. In particular a transaction containing a CommandFlags.FireAndForget command does not behave like a fire-and-forget single command.

Cases that are deliberately not flagged

The rule only fires on one condition guarding one queued command with the same key expression, because those are the cases with an exact equivalent. It stays quiet for cross-key conditions, several conditions or commands, and pairings with no atomic equivalent (HashExists + HashSet, HashEqual, ListIndexEqual, the *Length* conditions).

It also stays quiet where the queued command already passes its own when: argument. That is not an argument to move but a statement to overwrite - and Condition.KeyNotExists guarding a When.Exists write says "only if absent, and only if present", which is not code to be rewriting on a guess. An expiry is fine, on the other hand, and still flagged: SET takes one alongside NX, and that lock-acquire shape is much of what this rule is for.

Plus everything under when these rules stay quiet.

See also Transactions.

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

The flagged code is correct, just not optimal - but this is reported as a warning, so if you build with TreatWarningsAsErrors it will fail your build until you act on it or turn it down. To silence it:

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

or locally:

c#
#pragma warning disable SER300