Fix : Get command buffer handling closes #2979#3197
Fix : Get command buffer handling closes #2979#3197watersRand wants to merge 1 commit intoredis:masterfrom
Conversation
|
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? |
|
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. |
465a3e2 to
2d27e7d
Compare
| }, { | ||
| client: GLOBAL.SERVERS.OPEN, | ||
| cluster: GLOBAL.CLUSTERS.OPEN, | ||
| }); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.


Description
This PR addresses an inconsistency in the
GETcommand's reply transformation where it could occasionally return a rawBufferinstead of astring.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: UpdatedtransformReplyto use type-guarding (Buffer.isBuffer). It now explicitly convertsBufferreplies to UTF-8 strings, ensuring a consistent return type ofstring | nulland satisfying the linter.2.
GET.spec.ts: Updated the test suite to handleBufferarguments correctly intransformArguments.Related to the reported "random buffer" behavior during key retrieval in #2979
Checklist
npm testpass with this change (including linting)?Note
Low Risk
Test-only changes that broaden
GETcoverage across key types and common value scenarios; no production logic is modified.Overview
Improves the
GETcommand test suite by expandingtransformArgumentscases (includingBufferkeys) and adding broadertransformReplyassertions for missing keys, empty strings, overwrites, and values set fromBufferinputs to ensure consistent string returns.Written by Cursor Bugbot for commit 2d27e7d. This will update automatically on new commits. Configure here.