Fix/wrapper command options - #3295
Merged
Merged
Conversation
PavelPashov
approved these changes
May 27, 2026
nkaradzhov
force-pushed
the
fix/wrapper-commandOptions
branch
from
May 27, 2026 11:56
3eb8fd9 to
b3d3ec2
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Pool's `_commandOptions` was never set from `clientOptions.commandOptions` — it stayed `undefined` until `withCommandOptions(...)` was called. The pool's dispatch path passes `this._commandOptions` to `client._executeCommand`, where the underlying client's `defaultTypeMapping` check compares the original constructor `#options.commandOptions` against the passed-in options. With pool passing `undefined`, the check failed for any pool created with `commandOptions.typeMapping`, silently bypassing client-side cache for that pool. Assign `clientOptions.commandOptions` directly (same reference as the underlying client's `#options.commandOptions`) so the equality check succeeds and CSC fires. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`createSentinel` previously accepted `commandOptions` on both the top-level options and on `nodeClientOptions` / `sentinelClientOptions`. At dispatch time the wrapper-level value silently overrode the nested one, so setting `commandOptions` under `nodeClientOptions` (or `sentinelClientOptions`) never actually controlled command behavior. Narrow the per-node option types via a new `RedisSentinelNodeClientOptions` that `Omit`s `keyof SentinelCommander` (mirrors how `RedisClusterClientOptions` already excludes `keyof ClusterCommander`). Set `commandOptions` on the top-level sentinel options instead. BREAKING CHANGE: `nodeClientOptions.commandOptions` and `sentinelClientOptions.commandOptions` are no longer accepted by the type. Move the setting to the top-level `commandOptions` on `createSentinel`. Functional behavior is unchanged — the nested location was already silently ignored. Documented in docs/v5-to-v6.md. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`withCommandOptions(opts)` returns a proxy whose `_commandOptions` should override the wrapper's constructor options for subsequent calls. Several dispatch paths read through `this._self._commandOptions` instead of `this._commandOptions`. Because `_self` is pinned to the original instance in the constructor (`this._self = this`), the proxy's own `_commandOptions` is bypassed — the override silently does nothing. Drop the `_self.` in the affected paths so the read resolves through the prototype chain and picks up the proxy's own property when set: - `RedisClient.sendCommand` - `RedisCluster.sendCommand` - `RedisClientPool.sendCommand` (also now merges wrapper options with per-call options, which it previously skipped entirely) Sentinel's two `sendCommand` methods (`RedisSentinel.sendCommand` and `RedisSentinelClient.sendCommand`) had a separate variant of the same bug: the constructor stored options under a private `#commandOptions` field, but `withCommandOptions(...)` set a different property (`_commandOptions`) on the proxy. The `commandOptions` getter (and `sendCommand`) read only the private field, so proxy overrides were ignored on every command path — typed commands routed through `createCommand` included, since they also use the getter. Update the getter to merge `#commandOptions` with the proxy's `_commandOptions` and have `sendCommand` route through the getter. The other named module/function command paths use `this._self ._commandOptions` correctly because `_self` is the namespace receiver (set via `Object.defineProperty` in `attachNamespace`), so the proxy is preserved through namespace access. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
nkaradzhov
force-pushed
the
fix/wrapper-commandOptions
branch
from
May 27, 2026 12:54
b3d3ec2 to
143e4f9
Compare
`lint:changed` surfaced two pre-existing rule violations in pool.spec.ts because c1 modified the file: - HOTKEYS test used `(pool as any).X` repeatedly; cast once through `Record<string, unknown>` instead. - "execute rejects when pool is closing" had an unused `client` parameter; rename to `_client` to match the unused-args convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…sProxy `RedisSentinel._commandOptionsProxy` (used by `withTypeMapping(...)`) wrote the new options object to `proxy._self.#commandOptions`. Because `_self` resolves to the original sentinel instance, this mutated the shared private field that the new `commandOptions` getter (and `sendCommand`) read as the merge base — every other proxy and the original instance observed the change. Switch to the non-mutating pattern already used by `RedisSentinelClient._commandOptionsProxy`: assign the new override to `proxy._commandOptions` with `#commandOptions` as its prototype. The getter merges constructor-set `#commandOptions` with the proxy's own overrides, so behavior is preserved per proxy without leaking back to the source. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
`attachNamespace` cached the namespace as an own property on the
receiver via `Object.defineProperty(this, name, { value })`. When
`withCommandOptions(...)` later returned a proxy
(`Object.create(original)`) and the original had already had its
namespace materialized, the proxy inherited the original's cached own
property through the prototype chain. The namespace's `_self` then
pointed at the original, so module and function command dispatch
(`this._self._commandOptions`) read the original's options and
silently ignored the proxy's overrides.
Switch to a module-level `WeakMap<receiver, Map<name, namespace>>`.
Each receiver gets its own namespace bound to itself via `_self`,
identity is stable across repeated accesses on the same receiver, and
entries are reclaimed automatically when a receiver is GC'd. No
prototype-chain leak by construction.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ionsProxy Both sentinel `_commandOptionsProxy` methods built the new `_commandOptions` as `Object.create(this._self.#commandOptions ?? null)` then assigned the new key as an own property. Every consumer (the `commandOptions` getter and `sendCommand`) reads via spread, which only copies own enumerable properties — the prototype link is never observed. Replace with a plain spread merge that produces the same result and makes the intent obvious: "the base options plus this override". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 714308d. Configure here.
…and chaining
`withCommandOptions(...)` / `withTypeMapping(...)` proxies were silently
dropping their overrides in three related ways:
- Sentinel `use()`, `acquire()`, and `duplicate()` handed the leased or
duplicated client `this._self.#commandOptions` (the constructor base)
instead of the proxy's effective options.
- `_commandOptionsProxy` in client/pool/cluster built `_commandOptions`
via `Object.create(this._commandOptions ?? null)`, leaving earlier keys
on the prototype where the dispatch-time spread skipped them. Sentinel
had a similar shape — layering over `#commandOptions` instead of the
current effective options, which dropped prior proxy overrides on the
second call. Switched all four to flat `{ ...effective, [key]: value }`.
- Pool's helper was `#commandOptionsProxy` (true JS private), forcing
every caller to use `this._self.#commandOptionsProxy(...)` — which
discarded the proxy's overrides because `_self` resolves to the
original. Demoted to `private _commandOptionsProxy` so it inherits
via the prototype chain and is callable on proxies.
Added regression tests across sentinel, client, pool, and cluster: unit
tests for chained-override preservation and `duplicate()` propagation;
integration tests for `use()`/`acquire()` and behavioral dispatch of
`withTypeMapping`/`withCommandOptions` through both raw `sendCommand`
and typed commands. Cleaned up `as any` HOTKEYS casts in the cluster
spec while it was a changed file.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `withTypeMapping` mutation regression test used `{ initial: true }` /
`{ override: true }` as `typeMapping` values, which aren't valid `TypeMapping`
keys and tripped TS2322/TS2353 in the IDE. Swap to `RESP_TYPES.SIMPLE_STRING`
mapped to `Buffer` (initial) and `String` (override) so the test still proves
the source `commandOptions` is unmutated while satisfying the type.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Touches dispatch paths for all connection wrappers and changes Sentinel typing for where
commandOptionsmay be set; behavior fixes are broad but well covered by new regression tests.Overview
Fixes silent loss of
withCommandOptions/withTypeMappingoverrides on client, pool, cluster, and sentinel wrappers. Proxies now merge options with a plain object spread instead of prototype chaining, dispatch reads the proxy’s_commandOptions, and module/function namespaces are cached per receiver so_selfmatches the proxy.Pool also seeds
_commandOptionsfrom constructor options and merges them insendCommand. Sentinel exposes effective options viacommandOptions, passes them throughuse/acquire/duplicate, and types/docs require top-levelcommandOptions(notnodeClientOptions/sentinelClientOptions). Regression tests and a v5→v6 migration note cover the sentinel API change.Reviewed by Cursor Bugbot for commit 04fdb76. Bugbot is set up for automated code reviews on this repo. Configure here.