fix(redis): comprehensive Redis plugin overhaul#378
Merged
Conversation
📝 WalkthroughWalkthroughExpanded Redis support: broadened command parsing and flags, added SET EXAT/PXAT, richer CLI completions, username AUTH and SSL verify options, connection/timeout/keepalive and state-lock hardening, SCAN caching and DB discovery, type-aware statement generation, and task cancellation fixes. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Driver as RedisPluginDriver
participant Conn as RedisPluginConnection
participant Redis as Redis Server
rect rgba(100, 150, 255, 0.5)
Note over Client,Redis: Connection, Auth, SSL verify
Client->>Driver: selectRedisDatabaseAndQuery(db)
Driver->>Conn: init(host, port, username?, password, sslConfig)
Conn->>Redis: redisConnectWithTimeout(10s)
Redis-->>Conn: connected
Conn->>Conn: setTimeouts(30s), enableTcpKeepalive()
Conn->>Redis: AUTH username password
Redis-->>Conn: OK
Conn-->>Driver: ready
end
rect rgba(150, 200, 100, 0.5)
Note over Driver,Redis: Database discovery & SCAN caching
Driver->>Redis: CONFIG GET databases
Redis-->>Driver: ["databases","N"]
Driver->>Redis: INFO keyspace
Redis-->>Driver: db0:keys=...,db1:keys=...
Driver->>Driver: build database list
Driver->>Redis: SCAN pattern...
Redis-->>Driver: keys...
Driver->>Driver: cache keys for (db,pattern)
end
rect rgba(200, 100, 150, 0.5)
Note over Driver,Redis: Type-aware statement execution & previews
Driver->>Driver: determine Type column
alt hash/list/set/zset/string
Driver->>Redis: HSET/RPUSH/SADD/ZADD/SET ...
end
Redis-->>Driver: reply
Driver-->>Client: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Plugins/RedisDriverPlugin/RedisPluginDriver.swift (2)
87-98:⚠️ Potential issue | 🟠 MajorRemove the
SELECT 1interception — it breaks database selection from the query editor.
SELECT 1is a legitimate Redis command to switch to database 1. Remapping it toPINGprevents users from selecting that database. The code already usesexecuteCommand(["PING"])for health checks; rely on that exclusively instead of hijackingSELECT 1.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift` around lines 87 - 98, The code intercepts a trimmed query equal to "select 1" and remaps it to a PING, which prevents legitimate Redis "SELECT 1" calls from reaching conn.executeCommand; remove the special-case branch (the if trimmed.lowercased() == "select 1" block) and its PluginQueryResult return so queries like "SELECT 1" are passed through to normal execution via conn.executeCommand, while keeping any dedicated health-check logic that already calls conn.executeCommand(["PING"]) elsewhere.
893-907:⚠️ Potential issue | 🟠 Major
ZADDreply handling does not account forINCRandCHflags.When
INCRis specified, Redis returns the member's updated score (not an integer count). WithCH, the integer return value represents members "changed" (newly added + existing members with updated scores), not just "added" as the current code assumes. This causes misreporting of flaggedZADDresults.🐛 Proposed fix
case .zadd(let key, let flags, let scoreMembers): var args = ["ZADD", key] args += flags for (score, member) in scoreMembers { args += [String(score), member] } let result = try await conn.executeCommand(args) - let added = result.intValue ?? 0 + if flags.contains("INCR") { + return PluginQueryResult( + columns: ["score"], + columnTypeNames: ["Double"], + rows: [[result.stringValue ?? redisReplyToString(result)]], + rowsAffected: 0, + executionTime: Date().timeIntervalSince(startTime) + ) + } + let changedOrAdded = result.intValue ?? 0 + let countLabel = flags.contains("CH") ? "changed" : "added" return PluginQueryResult( - columns: ["added"], + columns: [countLabel], columnTypeNames: ["Int64"], - rows: [[String(added)]], - rowsAffected: added, + rows: [[String(changedOrAdded)]], + rowsAffected: changedOrAdded, executionTime: Date().timeIntervalSince(startTime) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift` around lines 893 - 907, The ZADD branch currently always treats Redis' reply as an integer "added" count; update the .zadd handling to detect flags (check if flags contains "INCR" and/or "CH"), parse the Redis reply appropriately from result (use string/double value when INCR is present to return the updated score, and use intValue as "changed" when CH is present), and construct PluginQueryResult fields to match: when INCR return a column like "score" with a Float64/String of the returned score and set rowsAffected accordingly (e.g., 0 or 1 as appropriate), when CH is present return "changed" (intValue) and set rowsAffected to that value, otherwise keep the original "added" semantics using result.intValue; update columns, columnTypeNames, rows and rowsAffected to reflect each case and continue using conn.executeCommand(result) as before.
🧹 Nitpick comments (3)
TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift (2)
217-219: Consider clarifying the double-optional unwrapping.The
firstRow.first ?? nilpattern correctly flattensString??toString?, but the intent isn't immediately obvious. AflatMapmakes the double-optional handling explicit.♻️ Optional: clearer double-optional handling
if let firstRow = countResult?.rows.first, - let countStr = firstRow.first ?? nil { + let countStr = firstRow.first.flatMap({ $0 }) { count = Int(countStr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+QueryHelpers.swift around lines 217 - 219, The double-optional unwrapping in the block using countResult, firstRow and countStr is unclear; replace the 'firstRow.first ?? nil' pattern with an explicit flatMap to collapse String?? to String? (e.g. use firstRow.first.flatMap { $0 }) so the intent is clear before converting to Int and assigning to count; update the let binding for countStr in the same scope where countResult?.rows.first and count are handled.
295-309: Extract duplicated count-fetching logic into a helper.The count-fetching logic (lines 295-309) is nearly identical to lines 209-223 in
launchPhase2Work. Consider extracting a private helper to reduce duplication and ensure consistency when the logic needs to change.♻️ Proposed helper extraction
/// Fetch row count using appropriate method for connection type. private func fetchRowCount( driver: DatabaseDriver, tableName: String, isNonSQL: Bool ) async -> Int? { if isNonSQL { return try? await driver.fetchApproximateRowCount(table: tableName) } let quotedTable = driver.quoteIdentifier(tableName) let countResult = try? await driver.execute( query: "SELECT COUNT(*) FROM \(quotedTable)" ) guard let firstRow = countResult?.rows.first, let countStr = firstRow.first.flatMap({ $0 }) else { return nil } return Int(countStr) }Then both
launchPhase2WorkandlaunchPhase2Countcan call:let count = await fetchRowCount(driver: mainDriver, tableName: tableName, isNonSQL: isNonSQL)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+QueryHelpers.swift around lines 295 - 309, The duplicated logic that computes a table row count in launchPhase2Work and launchPhase2Count should be extracted into a single private helper (e.g., fetchRowCount(driver:tableName:isNonSQL:)) that encapsulates the isNonSQL branch, quoting via driver.quoteIdentifier, executing "SELECT COUNT(*) FROM …" for SQL drivers, and converting the first row/column to Int; replace the duplicated blocks in both launchPhase2Work and launchPhase2Count with an awaited call to this new helper to return Int?.Plugins/RedisDriverPlugin/RedisStatementGenerator.swift (1)
137-147: Hardcoded fallback field name may cause confusion.When the value cannot be parsed as JSON for a hash type, the fallback uses a hardcoded field name
"field". This could be surprising to users and may conflict with existing hash fields. Consider using a more descriptive name or documenting this behavior.💡 Consider a more descriptive fallback or warning
if let data = value.data(using: .utf8), let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] { var args = "HSET \(escapeArgument(key))" for (field, val) in json { args += " \(escapeArgument(field)) \(escapeArgument(String(describing: val)))" } return args } - return "HSET \(escapeArgument(key)) field \(escapeArgument(value))" + // Non-JSON value: use "value" as the single field name + return "HSET \(escapeArgument(key)) value \(escapeArgument(value))"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RedisDriverPlugin/RedisStatementGenerator.swift` around lines 137 - 147, In the "hash" branch of RedisStatementGenerator (inside the switch handling HSET), the fallback uses a hardcoded field name "field" when JSON parsing fails which can be confusing or collide with existing fields; update the logic in the case "hash" block (where HSET is constructed, referencing escapeArgument and the returned args string) to use a clearer strategy—either generate a descriptive or unique field name (e.g., include the key or a timestamp/UUID), or emit/log a warning/error and require callers to supply a proper field name—in short, replace the hardcoded "field" fallback with a deterministic, descriptive alternative or an explicit error/warning path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/RedisDriverPlugin/RedisCommandParser.swift`:
- Around line 980-988: The EX option currently parses Int(args[i + 1]) into
seconds but doesn't enforce positivity; update the parsing in RedisCommandParser
so after obtaining seconds from Int(...) you verify seconds > 0 and if not throw
RedisParseError.invalidArgument("EX value must be a positive integer"); keep the
existing missingArgument check, continue to set options.ex = seconds, hasOption
= true and increment i only when the value is valid.
- Around line 954-961: The trailing backslash inside an unclosed quote is
dropped because the code only appends a backslash when escapeNext &&
!escapedInsideQuote; update the logic in RedisCommandParser (around variables
escapeNext, escapedInsideQuote, current, hadQuote, tokens) to also append the
backslash when escapeNext is true and we were inside an (possibly unclosed)
quote — e.g., treat escapeNext uniformly whether escapedInsideQuote is true or,
at minimum, when hadQuote is true — so the trailing backslash is preserved in
current before pushing to tokens.
In `@Plugins/RedisDriverPlugin/RedisPluginConnection.swift`:
- Around line 562-567: markDisconnected() currently nulls the redis context
pointer without freeing it, leaking the allocated redisContext; modify
markDisconnected() to, while holding stateLock, check if context is non-nil and
call redisFree(context) (or the appropriate free function) before setting
context = nil and _isConnected = false, ensuring the free is only invoked once
and safe for repeated calls; update any related cleanup paths (e.g.,
disconnect()/deinit) to avoid double-free by clearing or nil-ing the context
after freeing.
- Around line 576-587: The current args.map block builds C strings but
force-unwraps buf.baseAddress (in the closure inside
utf8.withUnsafeBufferPointer), which crashes for empty strings; update the
closure in RedisPluginConnection where cStrings is created to safely handle
empty utf8 buffers: inside utf8.withUnsafeBufferPointer use if let base =
buf.baseAddress { base.withMemoryRebound(to: CChar.self, capacity: buf.count) {
src in ptr.initialize(from: src, count: buf.count) } } else { /* no bytes to
copy */ } so you still allocate ptr with capacity utf8.count + 1, set
ptr[utf8.count] = 0, and keep the existing defer deallocation. This removes the
force-unwrap and preserves correct behavior for empty-string arguments.
In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift`:
- Around line 147-156: The SCAN cache is only keyed by pattern so it is reused
across Redis databases; include the current connection's selected database
identifier in the cache key so the cache is DB-aware. Specifically, when
building cacheKey (currently `pattern ?? "*"`) incorporate the connection's
database index (e.g., `db` or `selectedDB`) into that string (e.g.,
`"\(db):\(pattern ?? "*")`), and use that combined key for the comparisons and
assignments of `cachedScanPattern` and `cachedScanKeys` around
`scanAllKeys(connection:conn, pattern:..., maxKeys: Self.maxScanKeys)`, ensuring
`cachedScanPattern` is set to the DB-aware key so scans from different databases
do not share cached results.
- Around line 410-412: The dropObjectStatement implementation returns nil which
causes PluginDriverAdapter.dropObjectStatement(...) to synthesize a default SQL
DROP statement; instead return an explicit unsupported marker so the adapter can
disable the action. Change func dropObjectStatement(name: String, objectType:
String, schema: String?, cascade: Bool) -> String? (in RedisPluginDriver) to
return a clear sentinel (e.g. a constant like "REDIS_UNSUPPORTED_DROP" or a
descriptive comment string) or otherwise signal "unsupported" (and ensure
PluginDriverAdapter recognizes that sentinel) so Redis drops are not translated
into SQL-like DROP <type> <name>.
- Around line 79-82: cancelCurrentQuery() flips the connection-level
cancellation flag but only execute(query:) currently clears it, which can cause
subsequent metadata/browse operations to be treated as canceled; update the code
to call redisConnection?.resetCancellation() at the start (or just before
issuing commands) of the browse/metadata entry points—specifically add a reset
call in fetchRows(...), fetchTables(...), and switchDatabase(to:) so that
executeCommand/executePipeline/selectDatabase won't see a stale cancellation
flag from a prior cancelCurrentQuery().
---
Outside diff comments:
In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift`:
- Around line 87-98: The code intercepts a trimmed query equal to "select 1" and
remaps it to a PING, which prevents legitimate Redis "SELECT 1" calls from
reaching conn.executeCommand; remove the special-case branch (the if
trimmed.lowercased() == "select 1" block) and its PluginQueryResult return so
queries like "SELECT 1" are passed through to normal execution via
conn.executeCommand, while keeping any dedicated health-check logic that already
calls conn.executeCommand(["PING"]) elsewhere.
- Around line 893-907: The ZADD branch currently always treats Redis' reply as
an integer "added" count; update the .zadd handling to detect flags (check if
flags contains "INCR" and/or "CH"), parse the Redis reply appropriately from
result (use string/double value when INCR is present to return the updated
score, and use intValue as "changed" when CH is present), and construct
PluginQueryResult fields to match: when INCR return a column like "score" with a
Float64/String of the returned score and set rowsAffected accordingly (e.g., 0
or 1 as appropriate), when CH is present return "changed" (intValue) and set
rowsAffected to that value, otherwise keep the original "added" semantics using
result.intValue; update columns, columnTypeNames, rows and rowsAffected to
reflect each case and continue using conn.executeCommand(result) as before.
---
Nitpick comments:
In `@Plugins/RedisDriverPlugin/RedisStatementGenerator.swift`:
- Around line 137-147: In the "hash" branch of RedisStatementGenerator (inside
the switch handling HSET), the fallback uses a hardcoded field name "field" when
JSON parsing fails which can be confusing or collide with existing fields;
update the logic in the case "hash" block (where HSET is constructed,
referencing escapeArgument and the returned args string) to use a clearer
strategy—either generate a descriptive or unique field name (e.g., include the
key or a timestamp/UUID), or emit/log a warning/error and require callers to
supply a proper field name—in short, replace the hardcoded "field" fallback with
a deterministic, descriptive alternative or an explicit error/warning path.
In `@TablePro/Views/Main/Extensions/MainContentCoordinator`+QueryHelpers.swift:
- Around line 217-219: The double-optional unwrapping in the block using
countResult, firstRow and countStr is unclear; replace the 'firstRow.first ??
nil' pattern with an explicit flatMap to collapse String?? to String? (e.g. use
firstRow.first.flatMap { $0 }) so the intent is clear before converting to Int
and assigning to count; update the let binding for countStr in the same scope
where countResult?.rows.first and count are handled.
- Around line 295-309: The duplicated logic that computes a table row count in
launchPhase2Work and launchPhase2Count should be extracted into a single private
helper (e.g., fetchRowCount(driver:tableName:isNonSQL:)) that encapsulates the
isNonSQL branch, quoting via driver.quoteIdentifier, executing "SELECT COUNT(*)
FROM …" for SQL drivers, and converting the first row/column to Int; replace the
duplicated blocks in both launchPhase2Work and launchPhase2Count with an awaited
call to this new helper to return Int?.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9919c0da-c0ad-4230-a906-62b8c1c53f80
📒 Files selected for processing (12)
Plugins/RedisDriverPlugin/RedisCommandParser.swiftPlugins/RedisDriverPlugin/RedisPlugin.swiftPlugins/RedisDriverPlugin/RedisPluginConnection.swiftPlugins/RedisDriverPlugin/RedisPluginDriver.swiftPlugins/RedisDriverPlugin/RedisQueryBuilder.swiftPlugins/RedisDriverPlugin/RedisStatementGenerator.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+Navigation.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+QueryAnalysis.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+Redis.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+SaveChanges.swiftTablePro/Views/Main/MainContentCoordinator.swift
- 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
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Plugins/RedisDriverPlugin/RedisPluginDriver.swift (1)
382-394: Potential SCAN cache staleness after database switch.When
switchDatabase(to:)is called, it changes the current database but does not clearcachedScanPatternandcachedScanKeys. The cache key includesdbIndex, so subsequentfetchRowscalls will correctly miss the cache due to key mismatch. However, the old cache entries remain in memory until the nextexecute()ordisconnect().This is a minor memory concern rather than a correctness issue, since the DB-aware cache key prevents returning wrong data.
🧹 Optional: Clear cache on database switch for memory efficiency
func switchDatabase(to database: String) async throws { redisConnection?.resetCancellation() guard let conn = redisConnection else { throw RedisPluginError.notConnected } + cachedScanPattern = nil + cachedScanKeys = nil let dbIndex: Int🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift` around lines 382 - 394, The switchDatabase(to:) method currently changes the selected Redis DB but leaves cachedScanPattern and cachedScanKeys populated, causing stale cache entries to remain in memory; update switchDatabase(to:) (the function that calls redisConnection?.resetCancellation() and conn.selectDatabase(dbIndex)) to clear or reset cachedScanPattern and cachedScanKeys immediately after successfully selecting the new database so stale scan caches are released (similar to how caches are cleaned on execute() or disconnect()).Plugins/RedisDriverPlugin/RedisStatementGenerator.swift (1)
134-157: Hash field insertion order is non-deterministic.When parsing JSON for hash inserts (lines 142-145), iterating over a
[String: Any]dictionary produces fields in an undefined order. While RedisHSETaccepts fields in any order and the result is semantically equivalent, this could make debugging or testing harder since the generated command varies between runs.This is a minor concern since correctness is preserved, but worth noting for test reproducibility.
🔧 Optional: Sort fields for deterministic output
if let data = value.data(using: .utf8), let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] { var args = "HSET \(escapeArgument(key))" - for (field, val) in json { + for (field, val) in json.sorted(by: { $0.key < $1.key }) { args += " \(escapeArgument(field)) \(escapeArgument(String(describing: val)))" } return args }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RedisDriverPlugin/RedisStatementGenerator.swift` around lines 134 - 157, The JSON-to-HSET path in generateInsertCommand produces non-deterministic field order because it iterates the [String: Any] dictionary directly; make the output deterministic by collecting json.keys, sorting them (e.g., lexicographically), and then iterating the sorted keys to append " \(escapeArgument(field)) \(escapeArgument(String(describing: val)))" to the HSET args (use the existing escapeArgument and json lookup to get val). This preserves behavior but ensures the HSET command string is stable for testing and debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift`:
- Around line 382-394: The switchDatabase(to:) method currently changes the
selected Redis DB but leaves cachedScanPattern and cachedScanKeys populated,
causing stale cache entries to remain in memory; update switchDatabase(to:) (the
function that calls redisConnection?.resetCancellation() and
conn.selectDatabase(dbIndex)) to clear or reset cachedScanPattern and
cachedScanKeys immediately after successfully selecting the new database so
stale scan caches are released (similar to how caches are cleaned on execute()
or disconnect()).
In `@Plugins/RedisDriverPlugin/RedisStatementGenerator.swift`:
- Around line 134-157: The JSON-to-HSET path in generateInsertCommand produces
non-deterministic field order because it iterates the [String: Any] dictionary
directly; make the output deterministic by collecting json.keys, sorting them
(e.g., lexicographically), and then iterating the sorted keys to append "
\(escapeArgument(field)) \(escapeArgument(String(describing: val)))" to the HSET
args (use the existing escapeArgument and json lookup to get val). This
preserves behavior but ensures the HSET command string is stable for testing and
debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b693976b-a2b6-4115-aca7-984b17671148
📒 Files selected for processing (5)
Plugins/RedisDriverPlugin/RedisCommandParser.swiftPlugins/RedisDriverPlugin/RedisPluginConnection.swiftPlugins/RedisDriverPlugin/RedisPluginDriver.swiftPlugins/RedisDriverPlugin/RedisStatementGenerator.swiftTablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- TablePro/Views/Main/Extensions/MainContentCoordinator+QueryHelpers.swift
- Plugins/RedisDriverPlugin/RedisCommandParser.swift
…caping - Restore async dispatch in deinit to prevent use-after-free when commands are in-flight on the serial queue - Use conn.selectDatabase() for SELECT command in query editor so _currentDatabase is updated and SCAN cache key stays correct - Quote keys with spaces in buildExplainQuery DEBUG OBJECT command - Escape ] in escapeGlobChars for correct SCAN MATCH patterns
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift`:
- Around line 443-444: The quoting logic for the Redis key (the local variable
quoted used in the return "DEBUG OBJECT \(quoted)" line) only escapes double
quotes but not backslashes, so first escape backslashes in the key (replace "\"
with "\\") and then escape double quotes (replace """ with "\"") before wrapping
in quotes; update the transformation applied to key in the function/method
containing the quoted variable so keys like a\b become "a\\b" and combined
escapes are correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cf1cba76-51f8-4664-8ddf-0c5ac8fff082
📒 Files selected for processing (3)
Plugins/RedisDriverPlugin/RedisPluginConnection.swiftPlugins/RedisDriverPlugin/RedisPluginDriver.swiftPlugins/RedisDriverPlugin/RedisQueryBuilder.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/RedisDriverPlugin/RedisQueryBuilder.swift
Comment on lines
+443
to
+444
| let quoted = key.contains(" ") || key.contains("\"") ? "\"\(key.replacingOccurrences(of: "\"", with: "\\\""))\"" : key | ||
| return "DEBUG OBJECT \(quoted)" |
There was a problem hiding this comment.
Backslash escaping missing in key quoting.
The quoting logic escapes double quotes but not backslashes. A key containing a\b would produce "a\b" which Redis might interpret as an escape sequence.
🛠️ Proposed fix
- let quoted = key.contains(" ") || key.contains("\"") ? "\"\(key.replacingOccurrences(of: "\"", with: "\\\""))\"" : key
+ let quoted = key.contains(" ") || key.contains("\"") || key.contains("\\")
+ ? "\"\(key.replacingOccurrences(of: "\\", with: "\\\\").replacingOccurrences(of: "\"", with: "\\\""))\""
+ : key🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift` around lines 443 - 444,
The quoting logic for the Redis key (the local variable quoted used in the
return "DEBUG OBJECT \(quoted)" line) only escapes double quotes but not
backslashes, so first escape backslashes in the key (replace "\" with "\\") and
then escape double quotes (replace """ with "\"") before wrapping in quotes;
update the transformation applied to key in the function/method containing the
quoted variable so keys like a\b become "a\\b" and combined escapes are correct.
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
1 participant
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.
Summary
Comprehensive fix for the Redis plugin addressing 60+ issues discovered during deep analysis of GitHub issue #374 (Redis queries load forever).
fetchValuePreview(200-600 round-trips) with pipelined batch (2 pipeline calls). Add scan result cache for pagination.\n/\t/\r), empty string tokens, ZADD flag parsing (NX/XX/GT/LT/CH/INCR), ZRANGE modern syntax (BYSCORE/BYLEX/REV/LIMIT). Make statement generator type-aware (HSET for hash, RPUSH for list, etc.).strdup, fix thread-safety races incontextpointer and cancellation flag.SELECT COUNT(*)and enum fetching for Redis, add Redis write/dangerous query detection, skip MULTI/EXEC wrapping for saves, add race guard on database switching, returnFLUSHDBfor truncate, show empty databases in sidebar.Closes #374
Test plan
SET key "hello\nworld"stores actual newlineSET key ""stores empty stringZADD myset NX 1.0 memberparses successfullyFLUSHDBtriggers dangerous-query warningSET key valuedetected as write in read-only modeSummary by CodeRabbit
New Features
Improvements
Bug Fixes