fix(plugin-redis): list databases on a server that refuses CONFIG - #3037
Closed
junghyuneun wants to merge 1 commit into
Closed
junghyuneun wants to merge 1 commit into
junghyuneun wants to merge 1 commit into
Conversation
Member
|
Hi @junghyuneun, thanks for your contribution! Unfortunately, this issue has already been fixed in PR #3038. |
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.
Fixes #3036
An AWS ElastiCache node lists no databases at all. The window shows the server's own refusal where the keyspace should be:
Root cause
docs/databases/redis.mdxalready states the intended behaviour:The fallback did not cover a server that answers with an error. In
RedisPluginDriver.databaseCount(on:):That
guardonly catches a successful reply of an unexpected shape. An error reply never reaches it, becauserunis the choke point that turns one into a throw:throwIfError(name)inRedisCommandChannel.run, which is also where theCONFIG:prefix on the message comes from. The throw leftdatabaseCountand tookfetchTablesandfetchDatabaseswith it.ElastiCache restricts
configalongsidebgsave,debug,migrate,replicaof,save,shutdownandsync. It is removed rather than denied, so the answer isunknown commandand notNOPERM. Both now fall back; an ACL that deniesconfig|getwas failing in exactly the same way.CONFIGis also the only command in this driver's vocabulary that ElastiCache blocks.INFO,SCAN,SELECT,DBSIZE,TYPE,TTLand the rest are all permitted, so the browser works once the count is known.What changed
The rule moved into
RedisDatabaseCount, a pure file with no connection behind it, for the reasonRedisReply.swiftgives in its own header: the plugin imports CRedis and the test target cannot, so logic that needs covering lives apart from the connection.RedisPluginDriver.swiftkeeps the two call sites and loses the private helper, and the new file joins the list of plugin sources the test bundle compiles, besideRedisDatabaseIndex.swiftandRedisQueuedCommandPolicy.swift.A cluster node is still answered from the topology rather than probed, which is what the old first line did.
No docs change: the documented behaviour is what this restores.
Tests
RedisDatabaseCountTests, 12 cases over a stub channel modelled on the one inRedisQueuedReplyTests.swift:CONFIG(the ElastiCache shape) lists 16CONFIG(NOPERM) lists 16CONFIG GET databases0and-1, fall backThe error strings are measured, not invented. Redis 8.10.1 started with
--rename-command CONFIG ''reproduces the reported message byte for byte:Verification, and its limits
I do not have Xcode on this machine, only the Command Line Tools, so I have not built the project or run the suite. CI is the real gate here and I will fix whatever it reports.
What I could check, with the CLT Swift 6.4 compiler against faithful stand-ins for
RedisReply,RedisCommandChannelandruncopied from this repo:swiftc -typecheck -swift-version 6 -strict-concurrency=completeover the new file and the test file: cleanSeparately, the fix was confirmed against the real thing before it was written: a loopback proxy answering
CONFIG GET databaseswith*2\r\n$9\r\ndatabases\r\n$2\r\n16\r\n, byte-identical to a real Redis reply, in front of an untouched ElastiCache endpoint makes the sidebar list db0-db15 with correct counts fromINFO keyspace. The count probe is the only thing standing between the driver and that keyspace.SwiftLint and SwiftFormat were not run for the same reason; the new files follow the spacing, access control and doc-comment style of the files beside them.
Checklist
CHANGELOG.mdupdated under[Unreleased], credit left offdocs/databases/redis.mdxalready documents