Back to Stackexchange Redis

SER303: transaction may be replaceable by a single compound command

docs/rules/SER303.md

3.1.134.9 KB
Original Source

SER303: transaction may be replaceable by a single compound command

There is no condition here at all - the transaction exists only to make two commands atomic, and a single command already does both.

c#
// flagged
var tran = db.CreateTransaction();
var value = tran.StringGetAsync(key);
_ = tran.KeyDeleteAsync(key);
await tran.ExecuteAsync();

// suggested
RedisValue value = await db.StringGetDeleteAsync(key);
Queued pairSingle commandServer
StringGet + KeyDeleteStringGetDelete (GETDEL)6.2
StringGet + KeyExpireStringGetSetExpiry (GETEX)6.2
StringGet + KeyPersistStringGetSetExpiry(key, null) (GETEX PERSIST)6.2
StringGet + StringSetStringSetAndGet (SET ... GET)6.2
HashGet + HashDeleteHashFieldGetAndDelete (HGETDEL)8.0
StringSet + KeyExpireStringSet(key, value, expiry) (SET ... EX)any
SetRemove + SetAddSetMove (SMOVE)any

The requirement varies across this family, from "any server" for SMOVE up to 8.0 for HGETDEL, so each message names its own - see declaring your server version to be shown only what your server supports.

One caveat on StringSet + KeyExpire: an absolute expiry works too, because Expiration converts implicitly from DateTime as well as TimeSpan - but the SET ... EXAT that produces does want a 6.2 server, where the relative form has worked since 2.6.12. The version column above is the relative case, which is the common one.

Order matters

Which way round the pair is queued is part of the meaning, for two different reasons.

The reads return a value: SET ... GET hands back the value from before the write, so it matches a queued get followed by a set - and not a set followed by a get, which asks for the value afterwards. That pairing is left alone.

The writes overwrite each other: SET clears any TTL on the key, so StringSet + KeyExpire is one command with a lifetime, while KeyExpire + StringSet ends with no expiry at all. Only the first order is flagged. For the same reason a StringSet that already carries an expiry, followed by a KeyExpire that overrides it, is left alone: which of the two lifetimes the single command should carry is a guess.

SetRemove + SetAdd is the exception: within a transaction both effects happen regardless of order, so either spelling is flagged.

What changes when you apply it

  • tran.Execute() returns whether the transaction ran; the compound command returns its own result - usually the value you were reading anyway.
  • The queued Task<T>s collapse into the single command's result.
  • CommandFlags must be carried over verbatim.

Cases that are deliberately not flagged

  • ListRightPop + ListLeftPush. This looks like LMOVE, and it is not. LMOVE moves the element it popped; inside a transaction the pop's result is an unresolved Task, so the caller cannot pass it to the push - whatever value is being pushed is a different one, and LMOVE would not reproduce it. The same reasoning rules out every read-modify-write pairing.
  • Different keys (or different members, for SetMove) - those are genuinely two operations.
  • Anything with a condition, which is SER300-SER302 territory.
  • A key local reassigned between the two calls - the keys are compared as source text, so a reassignment means identical text can be two different keys, and the rule stays quiet.
  • Three or more queued commands, and anything queued in a loop. Note that the same command repeated - which can be three or more - is SER304 rather than this rule.

Plus everything under when these rules stay quiet, which is where the family-wide cases live - a third queued command, commands in different branches, and arguments the compound command cannot carry.

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

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

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

or locally:

c#
#pragma warning disable SER303