Skip to content

Ensure proxies respect command options#3261

Open
watersRand wants to merge 5 commits into
redis:masterfrom
watersRand:fix/withTypeMappings
Open

Ensure proxies respect command options#3261
watersRand wants to merge 5 commits into
redis:masterfrom
watersRand:fix/withTypeMappings

Conversation

@watersRand

@watersRand watersRand commented May 2, 2026

Copy link
Copy Markdown
Contributor

Description

Closes #3055 ,a bug where commands generated via static factories (Modules and Functions) were incorrectly referencing the root client's options (this._self._commandOptions) instead of the immediate instance's options (this._commandOptions).

The Problem:
When using .withCommandOptions() or .withTypeMapping(), the library creates a proxy object. However, because Modules and Functions were hard-coded to look at the internal _self reference for their configuration, they completely ignored any user-defined type mappings or timeouts set on the proxy. This effectively broke RESP3 type mapping for all Redis Module commands.

The Solution:

  • Updated #createModuleCommand and #createFunctionCommand to reference this._commandOptions.
  • Updated sendCommand to prioritize instance-level options.
  • Updated the NamespaceProxyClient type definition to include _commandOptions to ensure type safety.

Allows developers to seamlessly switch between different configurations—such as string vs. binary (Buffer) outputs—across all available commands, including built-in modules, third-party modules, and user-defined libraries (Redis Functions). It enables this flexibility on the fly without the overhead of re-initializing the entire connection or maintaining multiple client instances for different return types.


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Note

Medium Risk
Touches core command dispatch for client, pool, cluster, and sentinel; behavior change is intentional but affects every proxied command path including RESP3 type mapping.

Overview
Fixes #3055: withCommandOptions / withTypeMapping proxies were ignored for module, function, and some dispatch paths because handlers read this._self._commandOptions or merged options with object spread (which drops keys on the prototype chain).

Command-option layering now uses a prototype chain (Object.assign(Object.create(...), overrides)) when building proxies and when merging per-call options in sendCommand, instead of shallow spreads. Inherited settings (e.g. timeout) stay visible without copying every key onto each layer.

Dispatch for modules, functions, pool/cluster/sentinel commands, and namespace objects wired through commander.ts uses the receiver’s _commandOptions (including a getter on module namespaces tied to the current proxy). duplicate() flattens the option chain via #flattenCommandOptions so duplicated clients keep the effective config.

Cluster routing passes merged options through an execution client proxy so deep proxy chains still apply typeMapping and related flags during MOVED/ASK retries.

Tests cover client, pool, and cluster proxy inheritance, buffer typeMapping on module echo, and duplicate() preserving chained options.

Reviewed by Cursor Bugbot for commit 67744f1. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread packages/client/lib/client/index.ts
Comment thread packages/client/lib/client/index.ts Outdated
Comment thread packages/client/lib/client/index.spec.ts
Comment thread packages/client/lib/commander.ts Outdated
Comment thread packages/client/lib/commander.ts Outdated
@nkaradzhov

Copy link
Copy Markdown
Collaborator

@watersRand thanks, I will have a look!

@nkaradzhov nkaradzhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are more places where this._self._commandOptions or this._self.commandOptions is used:

return this._self._executeCommand(command, parser, this._self._commandOptions, transformReply);

return this._self._executeCommand(fn, parser, this._self._commandOptions, transformReply);

...this._self._commandOptions,

return this._self.execute(client => client._executeCommand(command, parser, this._self._commandOptions, transformReply))

return this._self.execute(client => client._executeCommand(fn, parser, this._self._commandOptions, transformReply)) };

this._self._commandOptions,

this._self._commandOptions,

...this._self._commandOptions,

client => client._executeCommand(fn, parser, this._self.commandOptions, transformReply)

client => client._executeCommand(command, parser, this._self.commandOptions, transformReply)

It would be great if we can fix and test those as well.

});


testUtils.testWithClient('Module TypeMapping Fix', async client => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe change the title to: "proxies respect command options"

Comment thread packages/client/lib/commander.ts Outdated
Comment thread packages/client/lib/sentinel/utils.ts Outdated
Comment thread packages/client/lib/client/pool.ts Outdated
Comment thread packages/client/lib/client/pool.ts Outdated
Comment thread packages/client/lib/cluster/index.ts Outdated

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit fa944a8. Configure here.

Comment thread packages/client/lib/commander.ts Outdated
@nkaradzhov

Copy link
Copy Markdown
Collaborator

@watersRand thanks, i will have a look at this.

@nkaradzhov nkaradzhov self-requested a review June 25, 2026 11:00

@nkaradzhov nkaradzhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things are still open before this can go in:

  1. RESP default regression (commander.ts). const RESP = config?.RESP ?? 2 should stay ?? DEFAULT_RESP (which is 3). The library default is RESP3, and this binds RESP2 transformReply handlers at class-generation time for any client created without an explicit RESP — even though the connection still negotiates RESP3. Bugbot re-flagged this on the latest commit. Please revert to DEFAULT_RESP (and keep the import).
  2. Sentinel getter recurses / duplicate member (sentinel/index.ts). The added
    get _commandOptions() { return this.commandOptions; }
  3. recurses — the existing commandOptions getter already returns this._commandOptions, so commandOptions → _commandOptions → commandOptions → … overflows the stack. It also collides with the existing private _commandOptions? field (duplicate identifier), in both RedisSentinelClient and RedisSentinel. Sentinel already stores options in #commandOptions with a public commandOptions getter, so it needs a different shape than the standalone client. Suggest having the sentinel command funcs read this._self.commandOptions (the existing public getter) rather than inventing _commandOptions, or wiring the namespace _self the way commander.ts does — but not a self-referential getter.
  4. Cluster withCommandOptions not updated (cluster/index.ts). Still proxy._commandOptions = options; (plain replace), unlike client/pool which now do Object.assign(Object.create(this._commandOptions ?? null), options). Since the cluster command paths now read this._commandOptions, chaining withCommandOptions drops parent options like timeout. Please make it consistent.
  5. Duplicate import (cluster/index.spec.ts). import { RESP_TYPES } from '../RESP/decoder' appears twice. Won't compile.
  6. Unrelated formatting churn. Most of the diff is reformatting (noop, if( → if (, reindenting the .on() chain and the _ejectSocket block, type-map indentation) in files this fix doesn't functionally touch. It buries the real change and invites conflicts — please strip it and keep only the functional edits. If formatting is genuinely off, a separate PR is the place for it.

Also minor: the title still says "ensure module commands respect proxy typeMapping" — "proxies respect command options" matches the actual scope better now.

Once 1–4 are sorted and the formatting is reverted, happy to re-review. npm test (with linting) should catch 2 and 4 locally.

@watersRand watersRand changed the title fix: ensure module commands respect proxy typeMapping Ensure proxies respect command options Jun 27, 2026
@watersRand

Copy link
Copy Markdown
Contributor Author

A few things are still open before this can go in:

  1. RESP default regression (commander.ts). const RESP = config?.RESP ?? 2 should stay ?? DEFAULT_RESP (which is 3). The library default is RESP3, and this binds RESP2 transformReply handlers at class-generation time for any client created without an explicit RESP — even though the connection still negotiates RESP3. Bugbot re-flagged this on the latest commit. Please revert to DEFAULT_RESP (and keep the import).
  2. Sentinel getter recurses / duplicate member (sentinel/index.ts). The added
    get _commandOptions() { return this.commandOptions; }
  3. recurses — the existing commandOptions getter already returns this._commandOptions, so commandOptions → _commandOptions → commandOptions → … overflows the stack. It also collides with the existing private _commandOptions? field (duplicate identifier), in both RedisSentinelClient and RedisSentinel. Sentinel already stores options in #commandOptions with a public commandOptions getter, so it needs a different shape than the standalone client. Suggest having the sentinel command funcs read this._self.commandOptions (the existing public getter) rather than inventing _commandOptions, or wiring the namespace _self the way commander.ts does — but not a self-referential getter.
  4. Cluster withCommandOptions not updated (cluster/index.ts). Still proxy._commandOptions = options; (plain replace), unlike client/pool which now do Object.assign(Object.create(this._commandOptions ?? null), options). Since the cluster command paths now read this._commandOptions, chaining withCommandOptions drops parent options like timeout. Please make it consistent.
  5. Duplicate import (cluster/index.spec.ts). import { RESP_TYPES } from '../RESP/decoder' appears twice. Won't compile.
  6. Unrelated formatting churn. Most of the diff is reformatting (noop, if( → if (, reindenting the .on() chain and the _ejectSocket block, type-map indentation) in files this fix doesn't functionally touch. It buries the real change and invites conflicts — please strip it and keep only the functional edits. If formatting is genuinely off, a separate PR is the place for it.

Also minor: the title still says "ensure module commands respect proxy typeMapping" — "proxies respect command options" matches the actual scope better now.

Once 1–4 are sorted and the formatting is reverted, happy to re-review. npm test (with linting) should catch 2 and 4 locally.

Thanks for the review! I acknowledge all the points and have updated the PR accordingly. The regressions, sentinel stack overflow, cluster inheritance logic, and duplicate imports are now fixed, and I've completely stripped out the unrelated formatting noise.

@watersRand watersRand force-pushed the fix/withTypeMappings branch from fa944a8 to c2de21e Compare June 27, 2026 14:51
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.

withTypeMappings not working

2 participants