Skip to content

feat: add SET IFEQ/IFNE/IFDEQ/IFDNE condition options plus DIGEST and DELEX commands - #52

Open
lkwr wants to merge 2 commits into
vcms-io:mainfrom
lkwr:feat/set-conditional-options
Open

feat: add SET IFEQ/IFNE/IFDEQ/IFDNE condition options plus DIGEST and DELEX commands#52
lkwr wants to merge 2 commits into
vcms-io:mainfrom
lkwr:feat/set-conditional-options

Conversation

@lkwr

@lkwr lkwr commented Aug 3, 2026

Copy link
Copy Markdown

Support the conditional SET subcommands introduced in Redis 8.4, along with
the supporting DIGEST (XXH3 hash digest) and DELEX (conditional delete) commands.

  • CommandSetOptions: add setIfValueEquals/setIfValueNotEquals and setIfDigestEquals/setIfDigestNotEquals, mapped to the IFEQ/IFNE/IFDEQ/IFDNE tokens in buildSetCommand.
  • Add digest(key) returning the XXH3 hex digest of a string value.
  • Add delex(key, options?) returning 0/1 with the same four condition options via the new buildDelexCommand and CommandDelexOptions type.
  • Wire both commands into the command index and SolidisFeaturedClient.
  • Add builder unit tests and capability-probed integration tests in the strings suite; gating probes each option rather than relying on version detection, since distributions differ (Valkey 9.1 supports IFEQ only).

I've migrated a project from @redis/client to @vcms-io/solidis and noticed that the SET ... IFEQ option wasn't available, so I decided to add support for it, along with other related options and commands.

I know I could send the command manually, but having native support is much more convenient.

DELEX commands

Support the conditional SET subcommands introduced in Redis 8.4, along
with
the supporting DIGEST (XXH3 hash digest) and DELEX (conditional delete)
commands.

- CommandSetOptions: add setIfValueEquals/setIfValueNotEquals and
  setIfDigestEquals/setIfDigestNotEquals, mapped to the
  IFEQ/IFNE/IFDEQ/IFDNE
  tokens in buildSetCommand.
- Add digest(key) returning the XXH3 hex digest of a string value.
- Add delex(key, options?) returning 0/1 with the same four condition
  options
  via the new buildDelexCommand and CommandDelexOptions type.
- Wire both commands into the command index and SolidisFeaturedClient.
- Add builder unit tests and capability-probed integration tests in the
  strings suite; gating probes each option rather than relying on
  version
  detection, since distributions differ (Valkey 9.1 supports IFEQ only).
@vercel

vercel Bot commented Aug 3, 2026

Copy link
Copy Markdown

@lkwr is attempting to deploy a commit to the vcms-io Team on Vercel.

A member of the Team first needs to authorize it.

@lkwr
lkwr force-pushed the feat/set-conditional-options branch from d37c28d to dc41d63 Compare August 3, 2026 14:29
Drop the duplicate probeOptionSupported helper in the strings suite and
probe
the Redis 8.4 SET/DELEX condition options through the shared
isCommandSupported
utility, renaming the capability flags to the *Available style used
elsewhere.

isCommandSupported now treats "syntax error" replies as unsupported
alongside
"unknown command", letting it gate subcommand options whose coverage
differs
between Redis and Valkey (e.g. Valkey 9.1 supports IFEQ only).
@lkwr
lkwr force-pushed the feat/set-conditional-options branch from dc41d63 to c5f6664 Compare August 3, 2026 14:38
@jay-l-e-e

Copy link
Copy Markdown
Contributor

@lkwr I apologize for the delay in checking this as I missed the notification. Thank you for your contribution!

Went through this against the Redis 8.4 and Valkey docs. tsc and biome both pass on the branch, and the command mappings are right. DIGEST returns a null bulk string on a missing key so buildKeyStringOrNullExecutor fits, DELEX returns 0|1 so tryReplyNumber fits. Ordering, barrel exports, the !== undefined checks (correct: empty string is a valid value) all match how the rest of the repo does it.

The problem is that almost none of this is actually being tested.

Nothing in the CI matrix is Redis 8.4. I checked all ten images. redis-stack-server:latest is still on 7.4, so the newest Redis we run anywhere is 7.x. Every one of the DIGEST, DELEX, IFNE, IFDEQ, IFDNE integration tests skips on every job. What's left is two unit tests comparing builder output. This needs a redis:8-alpine entry in test.yaml before it's worth merging.

Valkey diverged and the PR doesn't know about it. SET IFEQ landed in Valkey 8.1, before Redis had it - so setIfEqAvailable comes back true on the four Valkey jobs and that test really does run there. Which means CI is now quietly asserting that Valkey's IFEQ behaves like Redis's, including the missing-key case. Nobody decided that. Everything else is Redis-only: no IFNE/IFDEQ/IFDNE, no DIGEST, and in place of DELEX Valkey has DELIFEQ key value (9.0). So the 'requires Redis 8.4+ SET IFEQ' skip messages are wrong, and setIfValueEquals is portable while the other three aren't. Right now the only place that distinction could live is the skip messages and the matrix, and both are wrong about it. Fix the gating and the messages and it's recorded where it's actually enforced. (DELIFEQ support is its own PR, but we run five Valkey images, so it's a gap.)

The isCommandSupported change worries me more than the feature does. Adding syntax error to the regex means a typo'd probe now reads as "unsupported" and skips quietly. That's how you get a green suite that tests nothing - which, with the matrix as it stands, is literally today's outcome: six probes, all false, suite passes. And it's a shared helper: 0020, 0021, 0022, 0029 all use it, across ten images. Widening its contract for one call site without guarding that call site leaves a lot of surface to luck.

We already have the right tool for this. 0002.keys-generic.test.ts:23 does (await detectServerCapabilities(client)).atLeast(7, 0). isCommandSupported is for module presence; version-gating a core command is what atLeast is for. Gate on atLeast(8, 4) plus isValkey. If you still want per-option probing - and the Valkey split means you do - make it a separate function so the call site says what it's actually doing.

IFDEQ/IFDNE + GET is a trap. With GET, SET returns the old value whether or not it set. For NX/XX a null check is enough; for IFEQ/IFNE you can compare the returned value against what you passed. For the digest variants you can't - you'd have to compute XXH3 yourself, and re-reading is useless because on success DIGEST now returns the new value's digest. Either make it unrepresentable (returnOldValue and the digest conditions in the same object), or at minimum pin what actually comes back with an integration test, since the unit test at :407 already blesses the IFEQ ... GET shape.

The two condition blocks are the same code twice: command.ts:213-228 and :239-254 are identical down to token order, and appendExpireOptions / appendGeoResultOptions are sitting right there as the pattern. Having setIfValueEquals on SET but ifValueEquals on DELEX is what stops you sharing the interface outright. I get why - setIfKeyNotExists already set the prefix - so if the names stay, the appender can still take the four values as arguments and the duplication goes away anyway.

The new tests are commented and the suite isn't. There are zero // comments across the 29 files and ~11.5k lines in scripts/tests/commands/. This PR adds 15 to one file, and most of them restate the line underneath:

// IFEQ mismatch is a no-op.
assert.strictEqual(await client.set(key, 'b', { setIfValueEquals: 'x' }), null);
assert.strictEqual(await client.get(key), 'a');

The it() name says it, the assert says it, and then the comment says it a third time. The one I'd keep is // Recompute the digest for the new value before testing IFDNE. that explains why the extra digest() call exists, which you can't get from reading it. That's about the bar: if the code already says it, don't say it again.

Same lens on the isCommandSupported docblock, which grew three lines to explain that "supported" now also means "didn't return a syntax error." When a helper needs a paragraph to cover what its name doesn't, the name is the thing to fix - which is the argument for splitting it above.

Smaller stuff:

  • No test for IFEQ with a TTL, even though IFEQ sets value and expiration and leaves the TTL alone on failure. 'SET keepOriginalTimeToLive preserves the TTL' (:322) is the precedent.
  • No wrong-type paths. Both DIGEST and conditional DELEX error on a non-string key. 'rejects INCR on a non-integer value' (:132) is the precedent.
  • ifValueEquals takes StringOrBuffer but nothing exercises a Buffer.
  • The DIGEST test uses 'Hello world', which is the exact example from the docs - where the answer is b6acb9d84a38ff74. /^[0-9a-fA-F]+$/ is leaving a free assertion on the table. At minimum pin the length at 16, or assert same-value-same-digest.
  • The DELEX test at :596 gates on the four SET flags as well. Different feature; it just thins the coverage.
  • client.ts:210 and :215 are 91 and 100 chars against lineWidth: 80. Biome won't reflow comments so lint passes, but every other comment in that file is under 80.
  • 383 is still hardcoded in README.md:31, README.ko.md:31, website/app/page.tsx:225.
  • The probes in before() are order-dependent - Connecting via URI #2 creates the key and Connecting from URI does not auto provide authentication parameters #6 deletes it, so Solidis official website #3Unit tests #5 depend on the earlier ones landing. Fragile if anyone reorders them. Also six round trips against servers that can't possibly support any of it.

Unrelated, but now that DELEX ... IFEQ exists, the Lua unlock in website/app/tutorials/distributed-locking/page.tsx:132 collapses to a single command. Redis's own docs use that as the headline example for it.

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants