diff --git a/CHANGELOG.md b/CHANGELOG.md index 6529349111..66b6b7df9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -176,6 +176,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Query in one tab cancelled and rolled back when another tab or window on the connection starts or stops a query. - Stop on MySQL 5.5, 5.6 or MariaDB 5.5 interrupting the next statement on the connection. - `QUEUED` results and hidden command errors when running several Redis commands or saving Redis grid edits. +- Redis databases unlistable on a server that removes or denies `CONFIG`, such as AWS ElastiCache, which showed the server's error in place of the keyspace. - Query timeout ignored on MySQL before 5.7.8 and MariaDB before 10.1.1. - MySQL query run a second time, and left running on the server, after the connection timed out. - Empty error when a parameterized MySQL query timed out. diff --git a/Plugins/RedisDriverPlugin/RedisDatabaseCount.swift b/Plugins/RedisDriverPlugin/RedisDatabaseCount.swift new file mode 100644 index 0000000000..0cb14fc6ec --- /dev/null +++ b/Plugins/RedisDriverPlugin/RedisDatabaseCount.swift @@ -0,0 +1,36 @@ +// +// RedisDatabaseCount.swift +// RedisDriverPlugin +// +// How many logical databases the sidebar lists. +// Kept apart from the driver, which imports CRedis, so the rule can be tested without the +// C library. +// + +import Foundation + +nonisolated enum RedisDatabaseCount { + /// What a Redis server ships with, and what the sidebar draws when the server will not say. + static let fallback = 16 + + /// A cluster node answers `CONFIG GET databases` with 1, and refuses `SELECT` with any other + /// index, so the tree shows the one keyspace that exists rather than fifteen that do not. + static let cluster = 1 + + /// Managed Redis commonly removes `CONFIG` outright rather than denying it: AWS ElastiCache + /// lists it as restricted, so the probe answers `unknown command` and `run` throws. The count + /// only decides how many `db` entries to draw, so a server that will not answer takes the + /// fallback instead of failing the whole listing. + static func resolve(on conn: any RedisCommandChannel) async -> Int { + guard conn.supportsDatabaseSelection else { return cluster } + guard let reply = try? await conn.run(["CONFIG", "GET", "databases"]), + let array = reply.arrayValue, + array.count >= 2, + let count = array[1].intValue, + count > 0 + else { + return fallback + } + return count + } +} diff --git a/Plugins/RedisDriverPlugin/RedisPluginDriver.swift b/Plugins/RedisDriverPlugin/RedisPluginDriver.swift index 9a21020572..9e41daf416 100644 --- a/Plugins/RedisDriverPlugin/RedisPluginDriver.swift +++ b/Plugins/RedisDriverPlugin/RedisPluginDriver.swift @@ -248,7 +248,7 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable { return [PluginTableInfo(name: Self.clusterDatabaseName, type: "TABLE", rowCount: count)] } - let databases = try await databaseCount(on: conn) + let databases = await RedisDatabaseCount.resolve(on: conn) let result = try await conn.run(["INFO", "keyspace"]) let info = result.stringValue ?? "" @@ -261,17 +261,6 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable { static let clusterDatabaseName = "db0" - /// A cluster node answers CONFIG GET databases with 1, and refuses SELECT with any other - /// index, so the tree shows the one keyspace that exists rather than fifteen that do not. - private func databaseCount(on conn: any RedisCommandChannel) async throws -> Int { - guard conn.supportsDatabaseSelection else { return 1 } - let reply = try await conn.run(["CONFIG", "GET", "databases"]) - guard let array = reply.arrayValue, array.count >= 2, let count = array[1].intValue, count > 0 else { - return 16 - } - return count - } - func fetchColumns(table: String, schema: String?) async throws -> [PluginColumnInfo] { [ PluginColumnInfo(name: "Key", dataType: "String", isNullable: false, isPrimaryKey: true), @@ -369,7 +358,8 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable { guard let conn = redisConnection else { throw RedisPluginError.notConnected } - return try await (0 ..< databaseCount(on: conn)).map { "db\($0)" } + let databases = await RedisDatabaseCount.resolve(on: conn) + return (0 ..< databases).map { "db\($0)" } } func fetchDatabaseMetadata(_ database: String) async throws -> PluginDatabaseMetadata { diff --git a/TableProTests/Plugins/RedisDatabaseCountTests.swift b/TableProTests/Plugins/RedisDatabaseCountTests.swift new file mode 100644 index 0000000000..fc3d8c0a0e --- /dev/null +++ b/TableProTests/Plugins/RedisDatabaseCountTests.swift @@ -0,0 +1,103 @@ +// +// RedisDatabaseCountTests.swift +// TableProTests +// +// AWS ElastiCache removes CONFIG rather than denying it, so the probe for how many databases to +// list answers `unknown command` and the run choke point throws. That throw used to escape +// `fetchTables` and `fetchDatabases`, and the sidebar showed the server's error instead of the +// keyspace, on every ElastiCache node. +// + +import Foundation +import TableProPluginKit +import Testing + +/// Driven by one task at a time, so the replies are handed out in order with no synchronisation. +private final class StubRedisChannel: RedisCommandChannel, @unchecked Sendable { + private var queuedReplies: [RedisReply] + private let selectable: Bool + private(set) var sentCommands: [[String]] = [] + + init(_ replies: [RedisReply], supportsDatabaseSelection selectable: Bool = true) { + queuedReplies = replies + self.selectable = selectable + } + + var isConnected: Bool { true } + var supportsDatabaseSelection: Bool { selectable } + + func connect(reportingStage report: @escaping ConnectionStageReporter) async throws {} + func disconnect() {} + func cancelCurrentQuery() {} + func serverVersion() -> String? { "8.10.1" } + func currentDatabase() -> Int { 0 } + func selectDatabase(_ index: Int) async throws {} + + func executeCommand(_ args: [Data]) async throws -> RedisReply { + sentCommands.append(args.map { String(data: $0, encoding: .utf8) ?? "" }) + guard !queuedReplies.isEmpty else { return .null } + return queuedReplies.removeFirst() + } + + func executePipeline(_ commands: [[Data]]) async throws -> [RedisReply] { + var replies: [RedisReply] = [] + for command in commands { + replies.append(try await executeCommand(command)) + } + return replies + } +} + +@Suite("Redis database count") +struct RedisDatabaseCountTests { + /// Measured on Redis 8.10.1 started with `--rename-command CONFIG ''`, which is what + /// ElastiCache does: the server answers with an error naming a command it does not have, + /// not with `NOPERM`. + @Test("A server that removes CONFIG still lists the default 16") + func removedConfigFallsBack() async { + let channel = StubRedisChannel([ + .error("ERR unknown command 'CONFIG', with args beginning with: 'GET' 'databases'"), + ]) + #expect(await RedisDatabaseCount.resolve(on: channel) == 16) + } + + @Test("A server that denies CONFIG through an ACL lists the default 16") + func deniedConfigFallsBack() async { + let channel = StubRedisChannel([ + .error("NOPERM User viewer has no permissions to run the 'config|get' command"), + ]) + #expect(await RedisDatabaseCount.resolve(on: channel) == 16) + } + + @Test("A server that answers is taken at its word") + func serverAnswerWins() async { + let channel = StubRedisChannel([.array([.string("databases"), .string("4")])]) + #expect(await RedisDatabaseCount.resolve(on: channel) == 4) + #expect(channel.sentCommands == [["CONFIG", "GET", "databases"]]) + } + + static let unusableAnswers: [RedisReply] = [ + .array([.string("databases")]), + .array([.string("databases"), .string("none")]), + .array([.string("databases"), .string("0")]), + .array([.string("databases"), .string("-1")]), + .array([]), + .status("OK"), + .null, + ] + + @Test("An answer that is not a count falls back", arguments: unusableAnswers) + func unusableAnswerFallsBack(reply: RedisReply) async { + #expect(await RedisDatabaseCount.resolve(on: StubRedisChannel([reply])) == 16) + } + + /// A cluster node has one keyspace and refuses `SELECT` with any other index, so it is never + /// asked: the probe would cost a round trip to learn a number the topology already fixes. + @Test("A cluster node is one keyspace and is never probed") + func clusterIsNotProbed() async { + let channel = StubRedisChannel([.array([.string("databases"), .string("16")])], + supportsDatabaseSelection: false) + #expect(await RedisDatabaseCount.resolve(on: channel) == 1) + #expect(channel.sentCommands.isEmpty) + } +} diff --git a/project.yml b/project.yml index 75f25b0011..d7c2465bdc 100644 --- a/project.yml +++ b/project.yml @@ -636,6 +636,7 @@ targets: - Plugins/RedisDriverPlugin/RedisCommandRouting.swift - Plugins/RedisDriverPlugin/RedisConnectProbe.swift - Plugins/RedisDriverPlugin/RedisConnectionMode.swift + - Plugins/RedisDriverPlugin/RedisDatabaseCount.swift - Plugins/RedisDriverPlugin/RedisDatabaseIndex.swift - Plugins/RedisDriverPlugin/RedisKeySlot.swift - Plugins/RedisDriverPlugin/RedisKeySummary.swift