diff --git a/docs/providers/redis.md b/docs/providers/redis.md index bcea89ec6..eb074bb32 100644 --- a/docs/providers/redis.md +++ b/docs/providers/redis.md @@ -280,7 +280,10 @@ on a leading `{` as §3.4 does, and asks before running any command in its Redis vocabulary - key, expiry, string, hash, list, set, sorted-set and stream writes, the scripting entry points, and the server and access commands, with container commands such as `CONFIG SET` matched on their two-token spelling - while a body it cannot read (broken JSON, a JSON body whose -`command` is not a string) asks rather than staying silent. +`command` is not a string) asks rather than staying silent. The vocabulary names only commands the +provider runs: the blocking list and sorted-set pops are refused before they reach the server +([§5.2b](#52b-commands-that-would-change-the-shared-connection-1107)), so their non-blocking +forms are the ones the gate asks about. ### 3.5 Reply normalisation into the shared grid diff --git a/src/lib/db/destructive-commands.ts b/src/lib/db/destructive-commands.ts index a168ea23f..7c9d8ff22 100644 --- a/src/lib/db/destructive-commands.ts +++ b/src/lib/db/destructive-commands.ts @@ -72,7 +72,14 @@ const MONGODB_DESTRUCTIVE_OPERATIONS: ReadonlySet = new Set([ * `CLUSTER SHARDS` are the reads); every form of it reassigns a slot, which is what * puts it beside `CLUSTER RESET` and `CLUSTER FORGET`. * - * Every name here can reach the server: `runCommand` calls + * Every name here is a command the provider runs: `query()` refuses some commands + * before they reach the server (`sharedConnectionRefusal` in + * `src/lib/db/providers/keyvalue/redis.ts`, documented in section 5.2b of + * `docs/providers/redis.md`), and a confirmation followed by that refusal would be + * the double take this gate exists to avoid. The blocking list and sorted-set pops + * (`BLPOP`, `BZPOPMIN` and the rest of the `B` forms) are refused that way, so only their + * non-blocking forms (`LPOP`, `RPOP`, `LMPOP`, `LMOVE`, `RPOPLPUSH`, `ZPOPMIN`, + * `ZPOPMAX`, `ZMPOP`) are here. `runCommand` itself calls * `client.call(command, ...args)` with no allow-list of any kind, so the vocabulary * is bounded by what Redis accepts rather than by what this provider implements. * Container commands are spelled with their subcommand (`CONFIG SET`), which the @@ -133,16 +140,11 @@ const REDIS_DESTRUCTIVE_COMMANDS: ReadonlySet = new Set([ "LPOP", "RPOP", "LMPOP", - "BLPOP", - "BRPOP", - "BLMPOP", "LSET", "LREM", "LTRIM", "LMOVE", - "BLMOVE", "RPOPLPUSH", - "BRPOPLPUSH", // Sets "SPOP", "SREM", @@ -157,10 +159,7 @@ const REDIS_DESTRUCTIVE_COMMANDS: ReadonlySet = new Set([ "ZREMRANGEBYLEX", "ZPOPMIN", "ZPOPMAX", - "BZPOPMIN", - "BZPOPMAX", "ZMPOP", - "BZMPOP", "ZUNIONSTORE", "ZINTERSTORE", "ZDIFFSTORE", diff --git a/tests/unit/db/destructive-commands.test.ts b/tests/unit/db/destructive-commands.test.ts index 0d6ea0c38..68e3c3f50 100644 --- a/tests/unit/db/destructive-commands.test.ts +++ b/tests/unit/db/destructive-commands.test.ts @@ -126,6 +126,36 @@ describe("isDestructiveNonSqlQuery", () => { expect(isDestructiveNonSqlQuery(query, "redis")).toBe(true); }); + test.each<[string]>([ + ["BLPOP queue 0"], + ["BRPOP queue 0"], + ["BLMPOP 2 2 queue LEFT COUNT 1"], + ["BLMOVE src dst LEFT RIGHT 0"], + ["BRPOPLPUSH src dst 0"], + ["BZPOPMIN z 0"], + ["BZPOPMAX z 0"], + ["BZMPOP 2 1 z MIN COUNT 1"], + ])("does not ask before the Redis command %s, which the provider refuses before it reaches the server", (query) => { + // Since #1121 `RedisProvider.query()` refuses the blocking commands through + // `sharedConnectionRefusal`, so a confirmation followed by a refusal is the + // double take this gate exists to avoid. The dialog is for commands that run. + expect(isDestructiveNonSqlQuery(query, "redis")).toBe(false); + }); + + test.each<[string]>([ + ["LPOP queue"], + ["RPOP queue"], + ["LMPOP 1 queue LEFT COUNT 1"], + ["LMOVE src dst LEFT RIGHT"], + ["RPOPLPUSH src dst"], + ["ZPOPMIN z"], + ["ZPOPMAX z"], + ["ZMPOP 1 z MIN COUNT 1"], + ])("still asks before %s, the non-blocking form of a refused command", (query) => { + // The provider runs these, and each one removes what it returns. + expect(isDestructiveNonSqlQuery(query, "redis")).toBe(true); + }); + test.each<[string]>([ ["GET k"], ["HGETALL user:1"],