Skip to content

Default Resp3#3215

Merged
nkaradzhov merged 28 commits into
redis:masterfrom
nkaradzhov:resp3
May 20, 2026
Merged

Default Resp3#3215
nkaradzhov merged 28 commits into
redis:masterfrom
nkaradzhov:resp3

Conversation

@nkaradzhov
Copy link
Copy Markdown
Collaborator

@nkaradzhov nkaradzhov commented Mar 31, 2026

Description

Describe your pull request here


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
Switching the library default from RESP2 to RESP3 and normalizing multiple reply shapes (including object prototypes and stream/search/module replies) can cause subtle runtime/type breakages for consumers that depended on previous defaults or null-prototype objects.

Overview
RESP3 becomes the default protocol across createClient/cluster/pool/legacy mode via a new DEFAULT_RESP = 3, and docs add a v5→v6 migration guide covering breaking reply-shape/type changes and the new maintNotifications default behavior.

Unstable RESP3 gating is removed (unstableResp3/unstableResp3Modules and per-command unstableResp3 flags), and several RESP3 transforms are stabilized/normalized (notably XREAD/XREADGROUP now return a v4/v5-compatible stream list shape on RESP3; HOTKEYS GET and MODULE LIST normalize map/array/object variants into stable DTOs).

Reply/object normalization changes: RESP3 map decoding and many command transforms now return plain objects instead of Object.create(null), and geo “WITH” commands now parse doubles as number (distance/coordinates). Bloom module updates include CF.INSERTNX reply typing changing to numeric codes, plus broader test coverage and defaulting Bloom tests to RESP: 3.

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

@nkaradzhov nkaradzhov force-pushed the resp3 branch 5 times, most recently from e4d84b7 to 311595a Compare April 6, 2026 15:52
@nkaradzhov nkaradzhov marked this pull request as ready for review April 16, 2026 15:32
@nkaradzhov nkaradzhov mentioned this pull request Apr 16, 2026
3 tasks
Comment thread packages/client/lib/commands/XREAD.ts Outdated
);
}

object[key] = value;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we use Object.defineProperty here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

why?

default:
return this.#decodeMapAsObject(
Object.create(null),
{},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this cause issues if the server returns a __proto__ key in a map? Related to this comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, and we added this type of change to the v5-to-v6 migration doc

Comment thread packages/test-utils/lib/index.ts Outdated
const RESP = (options.clusterConfiguration?.RESP ?? 3) as RESP;
const {
RESP: _RESP,
minimizeConnections = false,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is minimizeConnections: false intentional?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

no, fixing

parseCfInsertArguments(...args);
},
transformReply: INSERT.transformReply
transformReply: undefined as unknown as () => ArrayReply<NumberReply<-1 | 0 | 1>>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might break the old RESP 2 responses as now it will return numbers instead of booleans for both protocols.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

that is intentional, documented in v5-to-v6

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Sorry, i see what you mean. This actually breaks the RESP 2, but we know that the server is in fact returning number in both resp2 and resp3. so its more of a fix for resp2. So i would keep it like this

Comment on lines +169 to +171
if (Array.isArray(rawReply)) {
return transformAggregateReplyResp2(rawReply as unknown as AggregateRawReply, preserve, typeMapping);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could this break RESP3 clients using RESP_TYPES.MAP: Array, since map replies decode as flat arrays and would be parsed as RESP2 here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

true, has to be fixed

Comment on lines +481 to +488
if (
key === "id" ||
key === "values" ||
key.toLowerCase() === "extra_attributes" ||
key === "extraAttributes"
) {
continue;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this stop working for RESP2, because we'll still need to JSON.parse there?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

JSON.parse still runs — it just moved into parseDocumentValue → assignDocumentField.
Worth adding a JSON test though.

@PavelPashov
Copy link
Copy Markdown
Contributor

Suggestion: Should we add a DEFAULT_RESP const so we don't hardcode 3 everywhere?

Comment thread packages/client/lib/commands/generic-transformers.ts
@nkaradzhov nkaradzhov requested a review from PavelPashov May 14, 2026 10:08
@nkaradzhov
Copy link
Copy Markdown
Collaborator Author

Suggestion: Should we add a DEFAULT_RESP const so we don't hardcode 3 everywhere?

the value depends on which 3s you mean:

  • True defaults (this.#options.RESP ?? 3 at client/index.ts:689,701,751, plus the RESP = 3 fallback in sentinel/test-util.ts:196 and test-utils/lib/index.ts:655): ~5 sites total — a const would help name the intent.
  • TS generic defaults (RESP extends RespVersions = 3 — ~6 sites in client/index.ts, sentinel/index.ts): cant reference a runtime const, so we would end up with a parallel type DefaultResp = 3 — duplication.
  • Per-test/per-package RESP: 3 as const (~10+ sites across */lib/test-utils.ts and the e2e specs): these are statements of intent ("I am pinning RESP3 here"), not deferrals to a default. Replacing with DEFAULT_RESP would obscure that — and silently change behavior if the const
    ever moved.

Comment thread packages/client/lib/commands/HOTKEYS_GET.ts Outdated
@nkaradzhov nkaradzhov force-pushed the resp3 branch 2 times, most recently from 7317258 to 367b658 Compare May 18, 2026 13:55
Copy link
Copy Markdown

@cursor cursor Bot left a comment

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 and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit 367b658. Configure here.

Comment thread packages/client/lib/commands/GEOSEARCH_WITH.ts
preserve?: any,
typeMapping?: TypeMapping
): SearchReply {
if (Array.isArray(rawReply)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

With typeMapping[RESP_TYPES.MAP] = Array, RESP3 maps also arrive as flat arrays like ['total_results', 1, 'results', ...], and this branch would misparse them as RESP2.

Comment thread packages/redis/index.ts
M extends RedisModules = RedisDefaultModules,
F extends RedisFunctions = {},
S extends RedisScripts = {},
RESP extends RespVersions = 2,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we update the RedisClientPoolType as well?

- Add RESP2 vs RESP3 structural assertions across command specs.
- Consolidate duplicate, renamed, and flaky test cases in one pass.
- Keep this commit spec-only to isolate test intent.
- Switch default RESP behavior to RESP3 across client entry points.
- Align cluster and sentinel command paths with RESP3 defaults.
- Keep compatibility normalization and module fixes for later commits.
- Normalize cross-module reply-shape handling for RESP2 and RESP3.
- Apply shared parser and transformer updates for stable compatibility.
- Leave targeted module bugfixes isolated for the next commit.
- Fix GEO float reply handling across geosearch-compatible paths.
- Fix Bloom CF.INSERTNX status handling and Search PROFILE parsing edge cases.
- Fix TimeSeries MRANGE selected-label/groupby compatibility behavior.
generic-transformers.spec.ts was newly uncommented and needed some assertion updates
nkaradzhov and others added 20 commits May 20, 2026 19:07
- refactor(streams): share RESP3 compat transformer across XREAD/XREADGROUP
- test-utils: restore pre-resp3 testWithCluster minimizeConnections default
- fix(search): handle FT.AGGREGATE RESP3 reply under typeMapping[MAP]=Array
- test(search): cover FT.HYBRID JSON-doc reply for RESP2 and RESP3
Replace hardcoded `?? 3` defaults with a named DEFAULT_RESP constant
exported from @redis/client. Covers all runtime fallback sites in
client/, sentinel/, cluster/, commander, legacy-mode, and the test-utils
helpers. TS generic defaults, JSDoc examples, and explicit per-test
RESP pins are intentionally left as literals.
transformStreamsMessagesReplyResp3Compat now matches the TransformReply
contract and propagates typeMapping into the inner Resp3 transform.
transformStreamsMessagesReplyResp3 accepts and forwards typeMapping to
transformStreamMessagesReply in every branch, and the V4-compat path in
transformStreamsMessagesReplyResp2 does the same — so XREAD/XREADGROUP
message bodies now honor RESP_TYPES.MAP like XRANGE does.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
FT.SEARCH, FT.SEARCH NOCONTENT, FT.PROFILE SEARCH/AGGREGATE,
FT.AGGREGATE WITHCURSOR and FT.HYBRID wrap inner transforms but their
signatures previously took only (reply), silently dropping typeMapping
on the way through. Extend each transformReply to the full
TransformReply contract and forward preserve/typeMapping into the inner
SEARCH/AGGREGATE transforms — the same shape as 25b36fb for streams —
so RESP_TYPES.MAP (and other typeMappings) now propagate end-to-end.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Object.defineProperties without writable: true defaults to writable:
false, making search/aggregate result rows silently immutable for
callers that mutate them in-place. Add writable: true so the compat
shape matches a plain object literal.

Drop the matching Object.defineProperties shape from
MRANGE_SELECTED_LABELS.spec.ts since the production code doesn't pin
that descriptor and the assertion only cares about structural equality.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
parseDouble short-circuited on typeof value === 'number' before
consulting typeMapping, so RESP3 callers asking for
RESP_TYPES.DOUBLE: String got raw numbers while RESP2 callers got the
string form. Honor the DOUBLE mapping on the number path too so both
protocols return the same shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Tighten the flat-array auto-unwrap to plain-objects only, so Buffers,
  typed arrays, and Dates no longer trip the single-element heuristic
  and short-circuit into mapLikeEntries.
- Guard map/tuple keys against null/undefined before calling toString().
- Surface non-finite slot values (NaN from unexpected wire shapes) by
  throwing a TypeError with the offending payload instead of silently
  emitting NaN-bearing SlotRange objects.
- Document that this command returns a fixed-schema DTO and does not
  honor RESP_TYPES.MAP / other typeMapping entries.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…casts

The previous transform only captured `name`/`ver` and cast missing values
to BlobStringReply/NumberReply, producing malformed entries when the
server returned modules without those fields. It also discarded extra
metadata (`path`, `args`, ...) that Redis exposes today.

Switch to a key-preserving transform over Array/Map/object replies and
relax the spec's exact-keys assertion accordingly. Type still advertises
the canonical name/ver shape, but the returned objects are open to extra
properties.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…LABELS RESP3 tuple

The middle tuple cell isn't actually `never` — Redis 7.4 leaves it empty
while Redis 8 may populate it with aggregation/reducer metadata. Type it
as `unknown` so the tuple shape stays honest with what the wire returns.
The transform already discards this cell, so behavior is unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous `warnings.map(w => w.toString())` collapsed non-string
warning payloads (Map/Array/plain object) into the useless
"[object Object]" string. Route warnings through a small helper that
keeps strings/Buffers as-is, treats null/undefined as empty, and
JSON-serializes everything else so the caller can read what the server
actually sent.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cument shape

- transformStreamsMessagesReplyResp3Compat's Array branch was packing
  the raw BlobStringReply as `name` without normalizing through
  toString(); callers reading the compat shape would get a Buffer when
  decoding is byte-oriented. Stringify the name in that branch to match
  the Map/Object branches.
- Add a JSDoc explaining that the Compat suffix intentionally returns a
  flat Array<{ name, messages }> regardless of RESP_TYPES.MAP, and that
  `typeMapping` is still forwarded to the inner message-body transform.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Legacy callback consumers inherit the v6 RESP3 default, so reply shapes
can differ from v5 for commands whose transforms diverge between
protocol versions. Document the workaround: pin RESP: 2 on the parent
client before calling .legacy().

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…list

These four stream commands route their message bodies through
transformStreamMessageReply -> transformTuplesReply, the same plain-object
normalization path already documented for XREAD/XREADGROUP. List them
explicitly so readers can identify affected reply shapes without tracing
the transform graph.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both options were v5 opt-in gates for in-development RESP3 transforms.
They were deleted in the v6 RESP3-default switch because the transforms
they gated are now stable. Document the removal so v5 upgraders aren't
surprised by the unknown-property TS error.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous annotation `: RespVersions = 3` widened the constant's
inferred type to `2 | 3`, losing the literal information at every call
site that branches on the value. Using `as const satisfies RespVersions`
keeps the literal `3` type while still validating membership in
`RespVersions`, so consumers that compare against `3` retain narrower
inference and the membership invariant is still checked at compile time.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
If the default RESP version ever changes again, this assertion would
silently diverge from the runtime default. Source the fallback from the
exported constant so the spec stays in sync.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
isPlainObject / mapLikeEntries / mapLikeValues / mapLikeToObject /
mapLikeToFlatArray / getMapValue were independently re-implemented in
@redis/client (HOTKEYS_GET), @redis/search (reply-transformers), and
@redis/time-series (INFO, INFO_DEBUG) with subtly different shapes —
some lacked null-safe key stringification, some over-accepted Buffer/
typed-array values as plain objects, and INFO_DEBUG carried a fourth
slightly divergent variant. Consolidate into a single module in
@redis/client and have all consumers import from it.

The unified versions adopt the strictest behavior already validated by
prior fixes: isPlainObject rejects Buffer/typed-arrays, mapLikeEntries
auto-unwraps single-element wrappers only when the inner value is
Array/Map/plain-object, and keyToString short-circuits null/undefined
keys. search/reply-transformers re-exports the shared symbols so
existing search command imports remain unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `_typeMapping` underscore prefix already exempts the param from
`@typescript-eslint/no-unused-vars`, so the disable directive triggered
"unused eslint-disable" warnings under --max-warnings 0.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@nkaradzhov nkaradzhov merged commit 62f4236 into redis:master May 20, 2026
22 of 23 checks passed
@nkaradzhov nkaradzhov deleted the resp3 branch May 20, 2026 16:42
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