diff --git a/docs/v5-to-v6.md b/docs/v5-to-v6.md index c428649d4d6..51940c62d0c 100644 --- a/docs/v5-to-v6.md +++ b/docs/v5-to-v6.md @@ -93,6 +93,27 @@ const client = createClient({ RESP: 2 }); ``` +## Sentinel: `commandOptions` moved off `nodeClientOptions` / `sentinelClientOptions` + +In v5, `createSentinel` accepted `commandOptions` on both the top-level options *and* on `nodeClientOptions` / `sentinelClientOptions`. The wrapper-level value silently overrode the per-node value at dispatch time, so the nested location never actually controlled command behavior. In v6, `commandOptions` is removed from `nodeClientOptions` and `sentinelClientOptions` at the type level — set it on the top-level sentinel options instead. + +```javascript +// v5 — silently ignored +const sentinel = createSentinel({ + name: 'mymaster', + sentinelRootNodes: [...], + nodeClientOptions: { commandOptions: { timeout: 1000 } } +}); + +// v6 +const sentinel = createSentinel({ + name: 'mymaster', + sentinelRootNodes: [...], + commandOptions: { timeout: 1000 } +}); +``` + + ## Legacy (callback) mode now uses RESP3 `createClient().legacy()` reads the parent client's RESP version. With the v6 default of RESP3, legacy callback consumers will see RESP3-shaped replies for any command whose transforms differ between protocol versions (for example, doubles arriving as `number` instead of `string`, or hash-like replies arriving as `Map`s). To keep the v5 callback reply shapes, pin `RESP: 2` on the parent client: diff --git a/packages/client/lib/client/index.spec.ts b/packages/client/lib/client/index.spec.ts index eec9409485b..19d0f17a4ad 100644 --- a/packages/client/lib/client/index.spec.ts +++ b/packages/client/lib/client/index.spec.ts @@ -29,6 +29,48 @@ export const SQUARE_SCRIPT = defineScript({ }); describe('Client', () => { + it('chained withCommandOptions(...).withTypeMapping(...) preserves earlier overrides at dispatch', () => { + // Regression: `_commandOptionsProxy` used to layer `_commandOptions` via + // `Object.create(this._commandOptions ?? null)`, which left earlier keys + // (e.g. `asap`) on the prototype. At dispatch, `{...this._commandOptions, ...}` + // only iterates *own* enumerable properties, so those inherited keys + // silently disappeared in the spread. + const client = RedisClient.create({}); + const proxy = client + .withCommandOptions({ asap: true }) + .withTypeMapping({ [RESP_TYPES.SIMPLE_STRING]: Buffer }); + type WithOptions = { _commandOptions?: { asap?: boolean; typeMapping?: unknown } }; + const ownKeys = { ...(proxy as unknown as WithOptions)._commandOptions }; + assert.equal(ownKeys.asap, true); + assert.deepEqual(ownKeys.typeMapping, { [RESP_TYPES.SIMPLE_STRING]: Buffer }); + }); + + it('module/function namespaces resolve to the receiver, not the original', () => { + // Regression: `attachNamespace` cached the namespace as an own property + // on the receiver, leaking via the prototype chain into any + // `withCommandOptions(...)` proxy. The proxy then dispatched module/function + // commands through the original's `_self`, silently ignoring the override. + const fakeModule = { + noop: { + parseCommand: () => {}, + transformReply: undefined as unknown as () => unknown + } + }; + const client = RedisClient.create({ modules: { fakeModule } }); + type WithNamespace = { fakeModule: { _self: unknown } }; + // Force the original to cache its namespace first — pre-fix this is what + // poisoned every subsequent proxy access. + const originalNamespace = (client as unknown as WithNamespace).fakeModule; + assert.equal(originalNamespace._self, client); + const proxy = client.withCommandOptions({}); + const proxyNamespace = (proxy as unknown as WithNamespace).fakeModule; + assert.equal(proxyNamespace._self, proxy); + assert.notEqual(proxyNamespace._self, client); + // Per-receiver cache: subsequent accesses on the same receiver are stable. + assert.equal((client as unknown as WithNamespace).fakeModule, originalNamespace); + assert.equal((proxy as unknown as WithNamespace).fakeModule, proxyNamespace); + }); + describe('initialization', () => { describe('clientSideCache validation', () => { const clientSideCacheConfig = { ttl: 0, maxEntries: 0 }; @@ -1314,6 +1356,38 @@ describe('Client', () => { }, GLOBAL.SERVERS.OPEN); }); + describe('withCommandOptions / withTypeMapping dispatch', () => { + testUtils.testWithClient('withTypeMapping override reaches raw sendCommand', async client => { + // Regression for `client/index.ts:1253` (`this._self._commandOptions` → + // `this._commandOptions`): without this fix, the proxy's `withTypeMapping` + // override was silently ignored at `sendCommand` dispatch. + const typed = client.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + const resp = await typed.sendCommand(['PING']); + assert.deepEqual(resp, Buffer.from('PONG')); + }, GLOBAL.SERVERS.OPEN); + + testUtils.testWithClient('withTypeMapping override reaches typed commands', async client => { + const typed = client.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + const resp = await typed.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + }, GLOBAL.SERVERS.OPEN); + + testUtils.testWithClient('withCommandOptions full override reaches typed commands', async client => { + // The `withCommandOptions` (full replace) path went through the same + // proxy-dispatch fix; covered separately from `withTypeMapping` because + // the two helpers store overrides differently on the proxy. + const proxy = client.withCommandOptions({ + typeMapping: { [RESP_TYPES.SIMPLE_STRING]: Buffer } + }); + const resp = await proxy.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + }, GLOBAL.SERVERS.OPEN); + }); + describe("socket errors during handshake", () => { it("should successfully connect when server accepts connection immediately", async () => { diff --git a/packages/client/lib/client/index.ts b/packages/client/lib/client/index.ts index c87efaf894f..e05a47c9bae 100644 --- a/packages/client/lib/client/index.ts +++ b/packages/client/lib/client/index.ts @@ -1043,8 +1043,7 @@ export default class RedisClient< value: V ) { const proxy = Object.create(this._self); - proxy._commandOptions = Object.create(this._commandOptions ?? null); - proxy._commandOptions[key] = value; + proxy._commandOptions = { ...this._commandOptions, [key]: value }; return proxy as RedisClientType< M, F, @@ -1250,7 +1249,7 @@ export default class RedisClient< // Merge global options with provided options const opts = { - ...this._self._commandOptions, + ...this._commandOptions, ...options, }; diff --git a/packages/client/lib/client/pool.spec.ts b/packages/client/lib/client/pool.spec.ts index 676186f7ce9..fa19504785d 100644 --- a/packages/client/lib/client/pool.spec.ts +++ b/packages/client/lib/client/pool.spec.ts @@ -1,19 +1,48 @@ import { strict as assert } from 'node:assert'; import testUtils, { GLOBAL } from '../test-utils'; +import { RESP_TYPES } from '../RESP/decoder'; import { RedisClientPool } from './pool'; describe('RedisClientPool', () => { + it('chained withCommandOptions(...).withTypeMapping(...) preserves earlier overrides at dispatch', () => { + // Regression: pool's `_commandOptionsProxy` had two related bugs. + // First, it built `_commandOptions` via `Object.create(...)`, leaving earlier + // keys on the prototype where the dispatch-time spread silently dropped them. + // Second, `withTypeMapping`/`withAbortSignal`/`asap` called the helper via + // `this._self.#commandOptionsProxy(...)`, so even the prototype chain was + // discarded — the helper saw the original pool's `_commandOptions`, not the + // prior proxy's. + const pool = RedisClientPool.create({}); + const proxy = pool + .withCommandOptions({ asap: true }) + .withTypeMapping({ [RESP_TYPES.SIMPLE_STRING]: Buffer }); + type WithOptions = { _commandOptions?: { asap?: boolean; typeMapping?: unknown } }; + const ownKeys = { ...(proxy as unknown as WithOptions)._commandOptions }; + assert.equal(ownKeys.asap, true); + assert.deepEqual(ownKeys.typeMapping, { [RESP_TYPES.SIMPLE_STRING]: Buffer }); + }); + + it('initializes _commandOptions from clientOptions.commandOptions', () => { + // Regression: when constructor commandOptions weren't propagated to the pool's own + // _commandOptions, the typeMapping equality check in client._executeCommand + // failed and silently bypassed client-side cache for pools. + const commandOptions = { typeMapping: {} }; + const pool = RedisClientPool.create({ commandOptions }); + const internal = Object.getPrototypeOf(pool) as { _commandOptions?: typeof commandOptions }; + assert.equal(internal._commandOptions, commandOptions); + }); + it('should not have HOTKEYS commands (requires session affinity)', () => { // HOTKEYS commands require session affinity and are only available on standalone clients - const pool = RedisClientPool.create({}); - assert.equal((pool as any).hotkeysStart, undefined); - assert.equal((pool as any).hotkeysStop, undefined); - assert.equal((pool as any).hotkeysGet, undefined); - assert.equal((pool as any).hotkeysReset, undefined); - assert.equal((pool as any).HOTKEYS_START, undefined); - assert.equal((pool as any).HOTKEYS_STOP, undefined); - assert.equal((pool as any).HOTKEYS_GET, undefined); - assert.equal((pool as any).HOTKEYS_RESET, undefined); + const pool = RedisClientPool.create({}) as unknown as Record; + assert.equal(pool.hotkeysStart, undefined); + assert.equal(pool.hotkeysStop, undefined); + assert.equal(pool.hotkeysGet, undefined); + assert.equal(pool.hotkeysReset, undefined); + assert.equal(pool.HOTKEYS_START, undefined); + assert.equal(pool.HOTKEYS_STOP, undefined); + assert.equal(pool.HOTKEYS_GET, undefined); + assert.equal(pool.HOTKEYS_RESET, undefined); }); testUtils.testWithClientPool('sendCommand', async pool => { @@ -23,6 +52,42 @@ describe('RedisClientPool', () => { ); }, GLOBAL.SERVERS.OPEN); + testUtils.testWithClientPool('withTypeMapping override reaches raw sendCommand', async pool => { + // Regression for `pool.ts:534-535`: pool.sendCommand now merges its own + // `_commandOptions` (which a `withCommandOptions` proxy overrides) before + // dispatching to the leased client. + const typed = pool.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + const resp = await typed.sendCommand(['PING']); + assert.deepEqual(resp, Buffer.from('PONG')); + }, GLOBAL.SERVERS.OPEN); + + testUtils.testWithClientPool('withTypeMapping override reaches typed commands', async pool => { + const typed = pool.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + const resp = await typed.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + }, GLOBAL.SERVERS.OPEN); + + testUtils.testWithClientPool('constructor commandOptions reach sendCommand without an explicit proxy', async pool => { + // The stated motivation for storing `_commandOptions` on the pool at + // construction was that the typeMapping needs to reach dispatch — the + // earlier internal-shape test only proved the property is stored. + const resp = await pool.sendCommand(['PING']); + assert.deepEqual(resp, Buffer.from('PONG')); + }, { + ...GLOBAL.SERVERS.OPEN, + clientOptions: { + commandOptions: { + typeMapping: { + [RESP_TYPES.SIMPLE_STRING]: Buffer + } + } + } + }); + testUtils.testWithClientPool('multi sendCommand', async pool => { assert.deepEqual( await pool.multi() @@ -96,7 +161,7 @@ describe('RedisClientPool', () => { testUtils.testWithClientPool('execute rejects when pool is closing', async pool => { // Start a long-running task to keep the pool busy during close - const task1Promise = pool.execute(async client => { + const task1Promise = pool.execute(async _client => { await new Promise(resolve => setTimeout(resolve, 100)); return 'task1'; }); diff --git a/packages/client/lib/client/pool.ts b/packages/client/lib/client/pool.ts index 3b8783c6328..0602187ee33 100644 --- a/packages/client/lib/client/pool.ts +++ b/packages/client/lib/client/pool.ts @@ -325,6 +325,7 @@ export class RedisClientPool< } this.#clientFactory = RedisClient.factory(clientOptions).bind(undefined, clientOptions) as () => RedisClientType; + this._commandOptions = clientOptions?.commandOptions as CommandOptions | undefined; } private _self = this; @@ -345,7 +346,11 @@ export class RedisClientPool< >; } - #commandOptionsProxy< + // Plain (not `#`) method so it can be invoked on prototype-derived proxies + // returned by `withCommandOptions(...)` — JS private (`#`) methods aren't + // accessible through the prototype chain, which would force the helper to + // be called via `this._self`, discarding any prior proxy overrides. + private _commandOptionsProxy< K extends keyof CommandOptions, V extends CommandOptions[K] >( @@ -353,8 +358,7 @@ export class RedisClientPool< value: V ) { const proxy = Object.create(this._self); - proxy._commandOptions = Object.create(this._commandOptions ?? null); - proxy._commandOptions[key] = value; + proxy._commandOptions = { ...this._commandOptions, [key]: value }; return proxy as RedisClientPoolType< M, F, @@ -368,14 +372,14 @@ export class RedisClientPool< * Override the `typeMapping` command option */ withTypeMapping(typeMapping: TYPE_MAPPING) { - return this._self.#commandOptionsProxy('typeMapping', typeMapping); + return this._commandOptionsProxy('typeMapping', typeMapping); } /** * Override the `abortSignal` command option */ withAbortSignal(abortSignal: AbortSignal) { - return this._self.#commandOptionsProxy('abortSignal', abortSignal); + return this._commandOptionsProxy('abortSignal', abortSignal); } /** @@ -383,7 +387,7 @@ export class RedisClientPool< * TODO: remove? */ asap() { - return this._self.#commandOptionsProxy('asap', true); + return this._commandOptionsProxy('asap', true); } async connect() { @@ -530,7 +534,8 @@ export class RedisClientPool< args: Array, options?: CommandOptions ) { - return this.execute(client => client.sendCommand(args, options)); + const mergedOptions = { ...this._commandOptions, ...options }; + return this.execute(client => client.sendCommand(args, mergedOptions)); } diff --git a/packages/client/lib/cluster/index.spec.ts b/packages/client/lib/cluster/index.spec.ts index 5b979491665..7ba94a0d571 100644 --- a/packages/client/lib/cluster/index.spec.ts +++ b/packages/client/lib/cluster/index.spec.ts @@ -5,19 +5,33 @@ import { SQUARE_SCRIPT } from '../client/index.spec'; import { RootNodesUnavailableError } from '../errors'; import { spy } from 'sinon'; import RedisClient from '../client'; +import { RESP_TYPES } from '../RESP/decoder'; describe('Cluster', () => { + it('chained withCommandOptions(...).withTypeMapping(...) preserves earlier overrides at dispatch', () => { + // Regression: cluster's `_commandOptionsProxy` used to layer via `Object.create`, + // leaving earlier keys on the prototype where the dispatch-time spread dropped them. + const cluster = RedisCluster.create({ rootNodes: [] }); + const proxy = cluster + .withCommandOptions({ asap: true }) + .withTypeMapping({ [RESP_TYPES.SIMPLE_STRING]: Buffer }); + type WithOptions = { _commandOptions?: { asap?: boolean; typeMapping?: unknown } }; + const ownKeys = { ...(proxy as unknown as WithOptions)._commandOptions }; + assert.equal(ownKeys.asap, true); + assert.deepEqual(ownKeys.typeMapping, { [RESP_TYPES.SIMPLE_STRING]: Buffer }); + }); + it('should not have HOTKEYS commands (requires session affinity)', () => { // HOTKEYS commands require session affinity and are only available on standalone clients - const cluster = RedisCluster.create({ rootNodes: [] }); - assert.equal((cluster as any).hotkeysStart, undefined); - assert.equal((cluster as any).hotkeysStop, undefined); - assert.equal((cluster as any).hotkeysGet, undefined); - assert.equal((cluster as any).hotkeysReset, undefined); - assert.equal((cluster as any).HOTKEYS_START, undefined); - assert.equal((cluster as any).HOTKEYS_STOP, undefined); - assert.equal((cluster as any).HOTKEYS_GET, undefined); - assert.equal((cluster as any).HOTKEYS_RESET, undefined); + const cluster = RedisCluster.create({ rootNodes: [] }) as unknown as Record; + assert.equal(cluster.hotkeysStart, undefined); + assert.equal(cluster.hotkeysStop, undefined); + assert.equal(cluster.hotkeysGet, undefined); + assert.equal(cluster.hotkeysReset, undefined); + assert.equal(cluster.HOTKEYS_START, undefined); + assert.equal(cluster.HOTKEYS_STOP, undefined); + assert.equal(cluster.HOTKEYS_GET, undefined); + assert.equal(cluster.HOTKEYS_RESET, undefined); }); testUtils.testWithCluster('sendCommand', async cluster => { @@ -27,6 +41,25 @@ describe('Cluster', () => { ); }, GLOBAL.CLUSTERS.OPEN); + testUtils.testWithCluster('withTypeMapping override reaches raw sendCommand', async cluster => { + // Regression for `cluster/index.ts:538` (`this._self._commandOptions` → + // `this._commandOptions`): without this fix, `withTypeMapping`/`withCommandOptions` + // proxies were silently ignored at cluster dispatch. + const typed = cluster.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + const resp = await typed.sendCommand(undefined, true, ['PING']); + assert.deepEqual(resp, Buffer.from('PONG')); + }, GLOBAL.CLUSTERS.OPEN); + + testUtils.testWithCluster('withTypeMapping override reaches typed commands', async cluster => { + const typed = cluster.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + const resp = await typed.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + }, GLOBAL.CLUSTERS.OPEN); + testUtils.testWithCluster('isOpen', async cluster => { assert.equal(cluster.isOpen, true); await cluster.destroy(); diff --git a/packages/client/lib/cluster/index.ts b/packages/client/lib/cluster/index.ts index a8f40cccb45..a0e045711aa 100644 --- a/packages/client/lib/cluster/index.ts +++ b/packages/client/lib/cluster/index.ts @@ -397,8 +397,7 @@ export default class RedisCluster< value: V ) { const proxy = Object.create(this); - proxy._commandOptions = Object.create(this._commandOptions ?? null); - proxy._commandOptions[key] = value; + proxy._commandOptions = { ...this._commandOptions, [key]: value }; return proxy as RedisClusterType< M, F, @@ -535,7 +534,7 @@ export default class RedisCluster< // Merge global options with local options const opts = { - ...this._self._commandOptions, + ...this._commandOptions, ...options } return this._self._execute( diff --git a/packages/client/lib/commander.ts b/packages/client/lib/commander.ts index 3370a0f3cec..b5ab085edaa 100644 --- a/packages/client/lib/commander.ts +++ b/packages/client/lib/commander.ts @@ -75,13 +75,31 @@ export function attachConfig< return Class; } +// Per-receiver namespace cache. Keyed by the receiver (original instance or any +// `withCommandOptions(...)` proxy) so each one gets a namespace bound to itself +// via `_self`. Caching the namespace as an own property on the receiver — which +// is what an earlier version did — leaks across the prototype chain: a proxy +// created via `Object.create(original)` would inherit the original's cached +// namespace and `_self` would point back to the original, silently bypassing +// the proxy's command-options overrides for every module/function command. +// eslint-disable-next-line @typescript-eslint/no-explicit-any -- namespaces are dynamically shaped per module +const namespaceCache = new WeakMap>(); + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- dynamic prototype patching helper function attachNamespace(prototype: any, name: PropertyKey, fns: any) { Object.defineProperty(prototype, name, { get() { - const value = Object.create(fns); - value._self = this; - Object.defineProperty(this, name, { value }); + let perReceiver = namespaceCache.get(this); + if (perReceiver === undefined) { + perReceiver = new Map(); + namespaceCache.set(this, perReceiver); + } + let value = perReceiver.get(name); + if (value === undefined) { + value = Object.create(fns); + value._self = this; + perReceiver.set(name, value); + } return value; } }); diff --git a/packages/client/lib/sentinel/index.spec.ts b/packages/client/lib/sentinel/index.spec.ts index fcaf05ad120..08ad5c626c1 100644 --- a/packages/client/lib/sentinel/index.spec.ts +++ b/packages/client/lib/sentinel/index.spec.ts @@ -14,6 +14,96 @@ import { once } from 'node:events' const execAsync = promisify(exec); describe('RedisSentinel', () => { + it('exposes top-level commandOptions via the commandOptions getter', () => { + // Regression: commandOptions used to be settable on both top-level and + // `nodeClientOptions`/`sentinelClientOptions`; the nested location was + // silently ignored at dispatch time. Top-level is now the only place. + const commandOptions = { typeMapping: {} }; + const sentinel = RedisSentinel.create({ + name: 'mymaster', + sentinelRootNodes: [{ host: 'localhost', port: 26379 }], + commandOptions + }); + assert.equal(sentinel.commandOptions, commandOptions); + }); + + it('withTypeMapping does not mutate the source sentinel commandOptions', () => { + // Regression: `_commandOptionsProxy` used to assign to `proxy._self.#commandOptions`, + // which (because `_self` resolves to the original sentinel) corrupted shared state — + // the original instance and every other proxy observed the typeMapping change. + const initialTypeMapping = { [RESP_TYPES.SIMPLE_STRING]: Buffer }; + const base = { typeMapping: initialTypeMapping }; + const sentinel = RedisSentinel.create({ + name: 'mymaster', + sentinelRootNodes: [{ host: 'localhost', port: 26379 }], + commandOptions: base + }); + sentinel.withTypeMapping({ [RESP_TYPES.SIMPLE_STRING]: String }); + assert.equal(sentinel.commandOptions, base); + assert.equal(sentinel.commandOptions?.typeMapping, initialTypeMapping); + }); + + it('withCommandOptions proxy overrides reach the commandOptions getter', () => { + // Regression: `withCommandOptions(...)` returned a proxy with an own + // `_commandOptions` property, but the getter and every dispatch path read + // only the constructor-set `#commandOptions`, so the override was a no-op. + const baseTypeMapping = {}; + const overrideTimeout = 12345; + const sentinel = RedisSentinel.create({ + name: 'mymaster', + sentinelRootNodes: [{ host: 'localhost', port: 26379 }], + commandOptions: { typeMapping: baseTypeMapping } + }); + const proxy = sentinel.withCommandOptions({ timeout: overrideTimeout }); + assert.equal(proxy.commandOptions?.typeMapping, baseTypeMapping); + assert.equal(proxy.commandOptions?.timeout, overrideTimeout); + }); + + it('chained withCommandOptions(...).withTypeMapping(...) preserves earlier overrides', () => { + // Regression: `_commandOptionsProxy` used to layer over `this._self.#commandOptions` + // (the constructor base) instead of `this.commandOptions` (the effective options), + // so any prior `withCommandOptions` override was silently dropped on the second call. + const sentinel = RedisSentinel.create({ + name: 'mymaster', + sentinelRootNodes: [{ host: 'localhost', port: 26379 }] + }); + const overrideTypeMapping = { [RESP_TYPES.SIMPLE_STRING]: Buffer }; + const proxy = sentinel + .withCommandOptions({ asap: true }) + .withTypeMapping(overrideTypeMapping); + assert.equal(proxy.commandOptions?.asap, true); + assert.equal(proxy.commandOptions?.typeMapping, overrideTypeMapping); + }); + + it('chained withTypeMapping(...).withTypeMapping(...) keeps the latest override', () => { + // Sanity: `_commandOptionsProxy` builds from the prior effective options, so a + // later `withTypeMapping` should still win for the same key. + const initial = { [RESP_TYPES.SIMPLE_STRING]: Buffer }; + const sentinel = RedisSentinel.create({ + name: 'mymaster', + sentinelRootNodes: [{ host: 'localhost', port: 26379 }], + commandOptions: { typeMapping: initial } + }); + const second = { [RESP_TYPES.SIMPLE_STRING]: String }; + const proxy = sentinel + .withTypeMapping({ [RESP_TYPES.SIMPLE_STRING]: Buffer }) + .withTypeMapping(second); + assert.equal(proxy.commandOptions?.typeMapping, second); + }); + + it('duplicate() on a withCommandOptions proxy carries the override into the new sentinel', () => { + // Regression: `duplicate()` used to read `this._self.#commandOptions` directly, + // so any proxy override created via `withCommandOptions(...)` was dropped. + const overrideTimeout = 99999; + const sentinel = RedisSentinel.create({ + name: 'mymaster', + sentinelRootNodes: [{ host: 'localhost', port: 26379 }] + }); + const proxy = sentinel.withCommandOptions({ timeout: overrideTimeout }); + const duplicated = proxy.duplicate(); + assert.equal(duplicated.commandOptions?.timeout, overrideTimeout); + }); + it('should not have HOTKEYS commands (requires session affinity)', () => { // HOTKEYS commands require session affinity and are only available on standalone clients const sentinel = RedisSentinel.create({ @@ -156,6 +246,62 @@ describe('RedisSentinel', () => { assert.deepEqual(resp, Buffer.from('PONG')); }, testOptions); + testUtils.testWithClientSentinel('withTypeMapping override flows through use() to the leased client', async sentinel => { + // Regression: `use()` used to pass `this._self.#commandOptions` (constructor base) + // to `RedisSentinelClient.create`, so any `withTypeMapping`/`withCommandOptions` + // proxy override was dropped before the leased client ever saw it. + const typeMapped = sentinel.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + + await typeMapped.use(async client => { + const resp = await client.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + }); + }, testOptions); + + testUtils.testWithClientSentinel('withTypeMapping override flows through acquire() to the leased client', async sentinel => { + // Regression: same as above, but for `acquire()` which returns the leased + // client to the caller instead of passing it to a callback. + const typeMapped = sentinel.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + + const client = await typeMapped.acquire(); + try { + const resp = await client.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + } finally { + client.release(); + } + }, testOptions); + + testUtils.testWithClientSentinel('RedisSentinelClient.withTypeMapping override reaches dispatch', async sentinel => { + // T2 / parity: every other proxy-options regression test hits top-level + // `RedisSentinel`. The same getter/merge/dispatch fixes live on + // `RedisSentinelClient` and need direct coverage. + await sentinel.use(async client => { + const typeMapped = client.withTypeMapping({ + [RESP_TYPES.SIMPLE_STRING]: Buffer + }); + const resp = await typeMapped.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + }); + }, testOptions); + + testUtils.testWithClientSentinel('RedisSentinelClient chained withCommandOptions(...).withTypeMapping(...) preserves earlier overrides', async sentinel => { + // B2 parity: the leased client's `_commandOptionsProxy` had the same + // chained-override bug as the top-level sentinel. + await sentinel.use(async client => { + const proxy = client + .withCommandOptions({ asap: true }) + .withTypeMapping({ [RESP_TYPES.SIMPLE_STRING]: Buffer }); + assert.equal(proxy.commandOptions?.asap, true); + const resp = await proxy.ping(); + assert.deepEqual(resp, Buffer.from('PONG')); + }); + }, testOptions); + testUtils.testWithClientSentinel('many readers', async sentinel => { await sentinel.set("x", 1); for (let i = 0; i < 10; i++) { diff --git a/packages/client/lib/sentinel/index.ts b/packages/client/lib/sentinel/index.ts index f886ab575a9..2aff2a0efd3 100644 --- a/packages/client/lib/sentinel/index.ts +++ b/packages/client/lib/sentinel/index.ts @@ -54,15 +54,19 @@ export class RedisSentinelClient< } /** - * Gets the command options configured for this client + * Gets the command options configured for this client. Merges the constructor-set + * options with any per-proxy override from `withCommandOptions(...)`. * - * @returns The command options for this client or `undefined` if none were set + * @returns The effective command options or `undefined` if none were set */ get commandOptions() { - return this._self.#commandOptions; + return this._commandOptions !== undefined + ? { ...this._self.#commandOptions, ...this._commandOptions } + : this._self.#commandOptions; } #commandOptions?: CommandOptions; + private _commandOptions?: CommandOptions; constructor( internal: RedisSentinelInternal, @@ -142,8 +146,7 @@ export class RedisSentinelClient< value: V ) { const proxy = Object.create(this); - proxy._commandOptions = Object.create(this._self.#commandOptions ?? null); - proxy._commandOptions[key] = value; + proxy._commandOptions = { ...this.commandOptions, [key]: value }; return proxy as RedisSentinelClientType< M, F, @@ -176,9 +179,10 @@ export class RedisSentinelClient< args: CommandArguments, options?: CommandOptions, ): Promise { + const mergedOptions = { ...this.commandOptions, ...options }; return this._execute( isReadonly, - client => client.sendCommand(args, options) + client => client.sendCommand(args, mergedOptions) ); } @@ -294,7 +298,9 @@ export default class RedisSentinel< } get commandOptions() { - return this._self.#commandOptions; + return this._commandOptions !== undefined + ? { ...this._self.#commandOptions, ...this._commandOptions } + : this._self.#commandOptions; } /** @@ -306,6 +312,7 @@ export default class RedisSentinel< } #commandOptions?: CommandOptions; + private _commandOptions?: CommandOptions; #trace: (msg: string) => unknown = () => { }; @@ -404,11 +411,7 @@ export default class RedisSentinel< value: V ) { const proxy = Object.create(this); - // Create new commandOptions object with the inherited properties - proxy._self.#commandOptions = { - ...(this._self.#commandOptions || {}), - [key]: value - }; + proxy._commandOptions = { ...this.commandOptions, [key]: value }; return proxy as RedisSentinelType< M, F, @@ -434,7 +437,7 @@ export default class RedisSentinel< >(overrides?: Partial>) { return new (Object.getPrototypeOf(this).constructor)({ ...this._self.#options, - commandOptions: this._self.#commandOptions, + commandOptions: this.commandOptions, ...overrides }) as RedisSentinelType<_M, _F, _S, _RESP, _TYPE_MAPPING>; } @@ -484,7 +487,7 @@ export default class RedisSentinel< try { return await fn( - RedisSentinelClient.create(this._self.#options, this._self.#internal, clientInfo, this._self.#commandOptions) + RedisSentinelClient.create(this._self.#options, this._self.#internal, clientInfo, this.commandOptions) ); } finally { const promise = this._self.#internal.releaseClientLease(clientInfo); @@ -497,9 +500,10 @@ export default class RedisSentinel< args: CommandArguments, options?: CommandOptions, ): Promise { + const mergedOptions = { ...this.commandOptions, ...options }; return this._execute( isReadonly, - client => client.sendCommand(args, options) + client => client.sendCommand(args, mergedOptions) ); } @@ -627,7 +631,7 @@ export default class RedisSentinel< */ async acquire(): Promise> { const clientInfo = await this._self.#internal.getClientLease(); - return RedisSentinelClient.create(this._self.#options, this._self.#internal, clientInfo, this._self.#commandOptions); + return RedisSentinelClient.create(this._self.#options, this._self.#internal, clientInfo, this.commandOptions); } getSentinelNode(): RedisNode | undefined { diff --git a/packages/client/lib/sentinel/types.ts b/packages/client/lib/sentinel/types.ts index afb378817e0..e193db68a0c 100644 --- a/packages/client/lib/sentinel/types.ts +++ b/packages/client/lib/sentinel/types.ts @@ -15,6 +15,18 @@ export type NodeAddressMap = { [address: string]: RedisNode; } | ((address: string) => RedisNode | undefined); +/** + * Per-node/per-sentinel client options. Excludes sentinel-level options (e.g. `commandOptions`) + * which must be set on the top-level sentinel options instead. + */ +export type RedisSentinelNodeClientOptions< + RESP extends RespVersions = RespVersions, + TYPE_MAPPING extends TypeMapping = TypeMapping +> = Omit< + RedisClientOptions, + keyof SentinelCommander +>; + export interface RedisSentinelOptions< M extends RedisModules = RedisModules, F extends RedisFunctions = RedisFunctions, @@ -34,16 +46,18 @@ export interface RedisSentinelOptions< * The maximum number of times a command will retry due to topology changes. */ maxCommandRediscovers?: number; - // TODO: omit properties that users shouldn't be able to specify for sentinel at this level /** - * The configuration values for every node in the cluster. Use this for example when specifying an ACL user to connect with + * The configuration values for every node in the cluster. Use this for example when specifying an ACL user to connect with. + * + * Sentinel-level options (e.g. `commandOptions`) cannot be set here — set them on the top-level sentinel options instead. */ - nodeClientOptions?: RedisClientOptions; - // TODO: omit properties that users shouldn't be able to specify for sentinel at this level + nodeClientOptions?: RedisSentinelNodeClientOptions; /** - * The configuration values for every sentinel in the cluster. Use this for example when specifying an ACL user to connect with + * The configuration values for every sentinel in the cluster. Use this for example when specifying an ACL user to connect with. + * + * Sentinel-level options (e.g. `commandOptions`) cannot be set here — set them on the top-level sentinel options instead. */ - sentinelClientOptions?: RedisClientOptions; + sentinelClientOptions?: RedisSentinelNodeClientOptions; /** * The number of clients connected to the master node */ diff --git a/packages/test-utils/lib/index.ts b/packages/test-utils/lib/index.ts index db52269810a..f23178d47ff 100644 --- a/packages/test-utils/lib/index.ts +++ b/packages/test-utils/lib/index.ts @@ -660,8 +660,8 @@ export default class TestUtils { name: 'mymaster', sentinelRootNodes: rootNodes, RESP, + commandOptions: options.clientOptions?.commandOptions, nodeClientOptions: { - commandOptions: options.clientOptions?.commandOptions, password: password || undefined, }, sentinelClientOptions: {