Skip to content

Commit 8b894d1

Browse files
datlechindatlechin
committed
fix(redis): address PR review findings
- Fix trailing backslash dropped inside unclosed quotes - Enforce positive integers for EX/PX values - Free redisContext in markDisconnected() to prevent leak - Guard against empty string crash in withArgvPointers - Remove SELECT 1 interception that blocked database switching - Add resetCancellation to fetchRows/fetchTables/switchDatabase - Make SCAN cache database-aware to prevent cross-db stale results - Return empty string from dropObjectStatement to prevent SQL fallback - Handle ZADD INCR/CH flags in reply formatting - Rename hash insert fallback field from "field" to "value" - Use flatMap for double-optional unwrapping in QueryHelpers
1 parent a60565f commit 8b894d1

5 files changed

Lines changed: 39 additions & 30 deletions

File tree

‎Plugins/RedisDriverPlugin/RedisCommandParser.swift‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -951,8 +951,8 @@ struct RedisCommandParser {
951951
current.append(char)
952952
}
953953

954-
// Handle trailing backslash outside quotes
955-
if escapeNext, !escapedInsideQuote {
954+
// Handle trailing backslash
955+
if escapeNext {
956956
current.append("\\")
957957
}
958958

@@ -980,7 +980,7 @@ struct RedisCommandParser {
980980
guard i + 1 < args.count else {
981981
throw RedisParseError.missingArgument("EX requires a value")
982982
}
983-
guard let seconds = Int(args[i + 1]) else {
983+
guard let seconds = Int(args[i + 1]), seconds > 0 else {
984984
throw RedisParseError.invalidArgument("EX value must be a positive integer")
985985
}
986986
options.ex = seconds
@@ -990,7 +990,7 @@ struct RedisCommandParser {
990990
guard i + 1 < args.count else {
991991
throw RedisParseError.missingArgument("PX requires a value")
992992
}
993-
guard let millis = Int(args[i + 1]) else {
993+
guard let millis = Int(args[i + 1]), millis > 0 else {
994994
throw RedisParseError.invalidArgument("PX value must be a positive integer")
995995
}
996996
options.px = millis

‎Plugins/RedisDriverPlugin/RedisPluginConnection.swift‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -561,9 +561,13 @@ private extension RedisPluginConnection {
561561

562562
func markDisconnected() {
563563
stateLock.lock()
564+
let handle = context
564565
context = nil
565566
_isConnected = false
566567
stateLock.unlock()
568+
#if canImport(CRedis)
569+
if let handle { redisFree(handle) }
570+
#endif
567571
}
568572

569573
func withArgvPointers<T>(
@@ -574,11 +578,11 @@ private extension RedisPluginConnection {
574578
let count = args.count
575579

576580
let cStrings: [UnsafeMutablePointer<CChar>] = args.map { arg in
577-
var utf8 = Array(arg.utf8)
581+
let utf8 = Array(arg.utf8)
578582
let ptr = UnsafeMutablePointer<CChar>.allocate(capacity: utf8.count + 1)
579-
utf8.withUnsafeBufferPointer { buf in
580-
buf.baseAddress!.withMemoryRebound(to: CChar.self, capacity: buf.count) { src in
581-
ptr.initialize(from: src, count: buf.count)
583+
if let base = utf8.withUnsafeBufferPointer({ $0.baseAddress }) {
584+
base.withMemoryRebound(to: CChar.self, capacity: utf8.count) { src in
585+
ptr.initialize(from: src, count: utf8.count)
582586
}
583587
}
584588
ptr[utf8.count] = 0

‎Plugins/RedisDriverPlugin/RedisPluginDriver.swift‎

Lines changed: 24 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -86,18 +86,6 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable {
8686

8787
let trimmed = query.trimmingCharacters(in: .whitespacesAndNewlines)
8888

89-
// Health monitor sends "SELECT 1" as a ping — intercept and remap to PING.
90-
if trimmed.lowercased() == "select 1" {
91-
_ = try await conn.executeCommand(["PING"])
92-
return PluginQueryResult(
93-
columns: ["ok"],
94-
columnTypeNames: ["Int32"],
95-
rows: [["1"]],
96-
rowsAffected: 0,
97-
executionTime: Date().timeIntervalSince(startTime)
98-
)
99-
}
100-
10189
let operation = try RedisCommandParser.parse(trimmed)
10290
return try await executeOperation(operation, connection: conn, startTime: startTime)
10391
}
@@ -134,6 +122,7 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable {
134122

135123
func fetchRows(query: String, offset: Int, limit: Int) async throws -> PluginQueryResult {
136124
let startTime = Date()
125+
redisConnection?.resetCancellation()
137126

138127
guard let conn = redisConnection else {
139128
throw RedisPluginError.notConnected
@@ -144,7 +133,8 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable {
144133

145134
switch operation {
146135
case .scan(_, let pattern, _):
147-
let cacheKey = pattern ?? "*"
136+
let dbIndex = conn.currentDatabase()
137+
let cacheKey = "\(dbIndex):\(pattern ?? "*")"
148138
let allKeys: [String]
149139
if cachedScanPattern == cacheKey, let cached = cachedScanKeys {
150140
allKeys = cached
@@ -180,6 +170,7 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable {
180170
// MARK: - Schema Operations
181171

182172
func fetchTables(schema: String?) async throws -> [PluginTableInfo] {
173+
redisConnection?.resetCancellation()
183174
guard let conn = redisConnection else {
184175
throw RedisPluginError.notConnected
185176
}
@@ -389,6 +380,7 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable {
389380
// MARK: - Database Switching
390381

391382
func switchDatabase(to database: String) async throws {
383+
redisConnection?.resetCancellation()
392384
guard let conn = redisConnection else { throw RedisPluginError.notConnected }
393385
let dbIndex: Int
394386
if let idx = Int(database) {
@@ -408,8 +400,9 @@ final class RedisPluginDriver: PluginDatabaseDriver, @unchecked Sendable {
408400
}
409401

410402
func dropObjectStatement(name: String, objectType: String, schema: String?, cascade: Bool) -> String? {
411-
// Redis databases are pre-allocated and cannot be dropped
412-
nil
403+
// Redis databases are pre-allocated and cannot be dropped.
404+
// Return empty string to prevent adapter from synthesizing SQL DROP.
405+
""
413406
}
414407

415408
// MARK: - EXPLAIN
@@ -897,12 +890,24 @@ private extension RedisPluginDriver {
897890
args += [String(score), member]
898891
}
899892
let result = try await conn.executeCommand(args)
900-
let added = result.intValue ?? 0
893+
if flags.contains("INCR") {
894+
// INCR mode returns the new score (or nil for NX miss)
895+
let scoreStr = result.stringValue ?? "nil"
896+
return PluginQueryResult(
897+
columns: ["score"],
898+
columnTypeNames: ["String"],
899+
rows: [[scoreStr]],
900+
rowsAffected: 0,
901+
executionTime: Date().timeIntervalSince(startTime)
902+
)
903+
}
904+
let count = result.intValue ?? 0
905+
let columnName = flags.contains("CH") ? "changed" : "added"
901906
return PluginQueryResult(
902-
columns: ["added"],
907+
columns: [columnName],
903908
columnTypeNames: ["Int64"],
904-
rows: [[String(added)]],
905-
rowsAffected: added,
909+
rows: [[String(count)]],
910+
rowsAffected: count,
906911
executionTime: Date().timeIntervalSince(startTime)
907912
)
908913

‎Plugins/RedisDriverPlugin/RedisStatementGenerator.swift‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ struct RedisStatementGenerator {
144144
}
145145
return args
146146
}
147-
return "HSET \(escapeArgument(key)) field \(escapeArgument(value))"
147+
return "HSET \(escapeArgument(key)) value \(escapeArgument(value))"
148148
case "list":
149149
return "RPUSH \(escapeArgument(key)) \(escapeArgument(value))"
150150
case "set":

‎TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ extension MainContentCoordinator {
215215
query: "SELECT COUNT(*) FROM \(quotedTable)"
216216
)
217217
if let firstRow = countResult?.rows.first,
218-
let countStr = firstRow.first ?? nil {
218+
let countStr = firstRow.first.flatMap({ $0 }) {
219219
count = Int(countStr)
220220
} else {
221221
count = nil
@@ -301,7 +301,7 @@ extension MainContentCoordinator {
301301
query: "SELECT COUNT(*) FROM \(quotedTable)"
302302
)
303303
if let firstRow = countResult?.rows.first,
304-
let countStr = firstRow.first ?? nil {
304+
let countStr = firstRow.first.flatMap({ $0 }) {
305305
count = Int(countStr)
306306
} else {
307307
count = nil

0 commit comments

Comments
 (0)