Skip to content

Fix : Get command buffer handling closes #2979#3197

Open
watersRand wants to merge 1 commit intoredis:masterfrom
watersRand:fix/Getcommand
Open

Fix : Get command buffer handling closes #2979#3197
watersRand wants to merge 1 commit intoredis:masterfrom
watersRand:fix/Getcommand

Conversation

@watersRand
Copy link
Contributor

@watersRand watersRand commented Mar 14, 2026

Description

This PR addresses an inconsistency in the GET command's reply transformation where it could occasionally return a raw Buffer instead of a string.

The Problem: In certain environments (high-concurrency or specific containerized setups), the Redis client would return binary data (Buffers) for keys that were expected to be UTF-8 strings. This required manual Buffer.isBuffer() checks in application code.

The Fix: 1. GET.ts: Updated transformReply to use type-guarding (Buffer.isBuffer). It now explicitly converts Buffer replies to UTF-8 strings, ensuring a consistent return type of string | null and satisfying the linter.
2. GET.spec.ts: Updated the test suite to handle Buffer arguments correctly in transformArguments.

Related to the reported "random buffer" behavior during key retrieval in #2979

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

Low Risk
Test-only changes that broaden GET coverage across key types and common value scenarios; no production logic is modified.

Overview
Improves the GET command test suite by expanding transformArguments cases (including Buffer keys) and adding broader transformReply assertions for missing keys, empty strings, overwrites, and values set from Buffer inputs to ensure consistent string returns.

Written by Cursor Bugbot for commit 2d27e7d. This will update automatically on new commits. Configure here.

@nkaradzhov
Copy link
Collaborator

Hi @watersRand, good to see you again! I will try to look at this soon. From first glance, cursor pointed couple of legit issues with the PR. Also, do we have minimal reproducible example of this unwanted Buffer case?

@watersRand
Copy link
Contributor Author

Hi @nkaradzhov ,

Good to see you again as well! I appreciate the feedback regarding the potential redundancy in the transformReply logic. I have been looking into this in more detail to ensure that my approach for the GET command aligns with the library’s architectural standards while still resolving the inconsistency I observed during my testing of #2979.

The wrapper was intended to prevent Buffer leaks where the standard undefined as unknown as pattern seemed to fall short compared to commands like GETDEL. However, I hear your concerns about duplicating the decoder's logic. I shall continue working on this to provide a minimal reproducible example of the "unwanted Buffer" case and will update the PR once I’ve determined if we can achieve the same result more cleanly within the decoder itself.

Copy link

@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

}, {
client: GLOBAL.SERVERS.OPEN,
cluster: GLOBAL.CLUSTERS.OPEN,
});
Copy link

Choose a reason for hiding this comment

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

Implementation fix to GET.ts is missing from PR

High Severity

The PR description explicitly states that GET.ts was updated to add Buffer.isBuffer type-guarding in transformReply, but GET.ts is completely unchanged — transformReply remains undefined as unknown as () => BlobStringReply | NullReply. The new tests (e.g., asserting typeof bufferVal === 'string') pass only because the RESP decoder already returns strings by default, not because of any new conversion logic. These tests are vacuous — they validate pre-existing behavior rather than the described fix, giving a false sense that the buffer handling issue from #2979 has been resolved.

Fix in Cursor Fix in Web

Copy link
Contributor Author

Choose a reason for hiding this comment

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

revert(GET): follow RESP type pattern, retain tests

Applied review feedback by removing custom transformReply logic
and restoring the standard RESP type-based approach.

Test updates retained.

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