Repository navigation
fix(rueidiscompat): drop the txt: prefix from Info and ClientList - #1051
Open
LindseyZ1205 wants to merge 1 commit into
Open
LindseyZ1205 wants to merge 1 commit into
LindseyZ1205 wants to merge 1 commit into
Conversation
With RESP3, INFO, CLIENT LIST, CLUSTER INFO and CLUSTER NODES reply with a verbatim string, which starts with its "txt:" format. Info, ClientList, ClusterInfo and ClusterNodes returned it as is, so their values started with "txt:" with RESP3 but not with RESP2, and the first line didn't parse. go-redis drops the format, and ClientInfo and InfoMap already trim it here. Trim it in these four commands as well, including in pipelines.
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
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.
With RESP3, which rueidis uses by default,
Info,ClientList,ClusterInfoandClusterNodesreturn values that start withtxt::With RESP3, these four commands reply with a verbatim string, whose first four bytes are its format (
txt:).ToString()returns the whole string, and these methods usednewStringCmd, so the format ended up in the value. The first line ofINFOistxt:# Serverinstead of a section header, the first client inCLIENT LISThas atxt:idfield, and the first node ID inCLUSTER NODESstarts withtxt:. go-redis drops the format, andClientInfoandInfoMapalready trim it in their parsers.This change adds
newTxtStringCmd, which trims thetxt:prefix inStringCmd.from, and uses it for these four commands. The flag is on theStringCmd, so the prefix is also trimmed when a pipeline fills in the result. RESP2 replies don't have the prefix and are unchanged, and so are the other commands that usenewStringCmd.Tests
should Info,should Info cpu,should ClientList,ClusterInfoandClusterNodesspecs only checked that the values weren't empty, so they passed with the prefix. They now check how the values start (# Server,# CPU,id=,cluster_state:, and a node ID on each line ofCLUSTER NODES). A newshould Pipeline Info and ClientListspec does the same through a pipeline. I ran the adapter suite against local Redis 8.2.1 servers on thedocker-compose.ymlports: these specs fail without this change and pass with it, and they are the only difference this change makes in the suite. The other failures there need the modules or the Redis 8.4+ commands that my local servers don't have. Locally the RESP2 clients also negotiated RESP3, because the RESP2 server is Redis 5 in CI, so I checked RESP2 separately (third item).TestTxtStringCmdFromincommand_test.go. The mock package has no verbatim string, so it uses blob strings with and without the prefix.Info,ClientList,ClusterInfoandClusterNodeswith go-redis against a local Redis 8.2.1 and a local cluster, with and withoutAlwaysRESP2, andInfoandClientListthrough a pipeline. Without this change the RESP3 values start withtxt:. With it they start the same way as go-redis's, and the RESP2 values are the same as before.rueidiscompattests that don't need a server (go test -skip '^TestAdapter$') pass, andgofmtandgo vetreport nothing.Note
Low Risk
Localized string normalization for four admin/cluster commands; RESP2 and other
newStringCmdcallers are unchanged.Overview
Under RESP3,
Info,ClientList,ClusterInfo, andClusterNodeswere returning values prefixed withtxt:because verbatim replies include that format tag inToString(). That broke parity with go-redis and RESP2 (e.g.# Servervstxt:# Server).This PR introduces
newTxtStringCmd, which sets a flag onStringCmdsofromstrips a leadingtxt:when present, and wires those four adapter methods to use it instead ofnewStringCmd. Pipelines keep the same behavior because the flag lives on the command object filled when results arrive.Tests now assert realistic value prefixes, add
TestTxtStringCmdFrom, and cover pipelinedInfo/ClientList.Reviewed by Cursor Bugbot for commit 4c001be. Bugbot is set up for automated code reviews on this repo. Configure here.