fix: Redis hash/list/set/zset/stream views drop non-UTF8 binary values#447
Merged
fix: Redis hash/list/set/zset/stream views drop non-UTF8 binary values#447
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughRedis reply parsing and result construction were changed to treat hash, list, set, zset, stream, and config replies as generic Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Plugins/RedisDriverPlugin/RedisPluginDriver.swift (1)
1234-1266:⚠️ Potential issue | 🟡 Minor
STRINGpreviews still blank out non-UTF8 payloads.While the collection branches now preserve raw replies, the
"string"path still goes throughreply.stringValue, so a binaryGETresult will keep showing an empty preview. Routing this branch throughredisReplyToString(...)makes string previews consistent with the rest of the fix.Suggested fix
case "string": - return truncatePreview(reply.stringValue) + return truncatePreview(redisReplyToString(reply))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift` around lines 1234 - 1266, In formatPreviewReply(_ reply:type:) the "string" case uses reply.stringValue which blanks non-UTF8/binary data; replace that use with redisReplyToString(reply) so string previews preserve raw/binary payloads consistently with the other branches (update the "string" case inside formatPreviewReply to call redisReplyToString instead of reply.stringValue and pass the result into truncatePreview).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@TableProTests/Core/Redis/RedisResultBuildingTests.swift`:
- Around line 12-147: The tests are exercising local replicas
(testRedisReplyToString, buildTestHashResult, buildTestListResult,
buildTestSetResult, buildTestSortedSetResult, buildTestConfigResult) instead of
the real builders (and skip the changed formatPreviewReply and buildStreamResult
paths); extract the reply-stringification/result-building logic into a shared,
testable seam (e.g., move implementations into RedisPluginDriver as
internal/static helpers or a small SharedRedisFormatting type) and update the
tests to call those real functions instead of the local copies; ensure the new
helpers expose the same APIs used here (including formatPreviewReply and
buildStreamResult) and make them `@testable-importable` or placed in a common
module so the test file can reference them directly.
- Around line 163-166: The test dataValidUtf8 uses force unwraps which must be
removed; change the UTF-8 setup to safely unwrap the Data using guard let / if
let (e.g., guard let data = "some text".data(using: .utf8) else { XCTFail(...) ;
return }) and update the assertion to compare the reply's optional properties
directly (e.g., compare reply.stringArrayValue or reply.stringValue instead of
force-unwrapping), and apply the same pattern to the other failing tests
referenced; locate the Data creation and the assertion call to
testRedisReplyToString in dataValidUtf8 (and the similar tests at the other
lines) and replace `!` usage with safe optional binding and proper test failure
handling.
---
Outside diff comments:
In `@Plugins/RedisDriverPlugin/RedisPluginDriver.swift`:
- Around line 1234-1266: In formatPreviewReply(_ reply:type:) the "string" case
uses reply.stringValue which blanks non-UTF8/binary data; replace that use with
redisReplyToString(reply) so string previews preserve raw/binary payloads
consistently with the other branches (update the "string" case inside
formatPreviewReply to call redisReplyToString instead of reply.stringValue and
pass the result into truncatePreview).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9bcb27b-c449-4b8d-bf87-9dc0d34aba56
📒 Files selected for processing (2)
Plugins/RedisDriverPlugin/RedisPluginDriver.swiftTableProTests/Core/Redis/RedisResultBuildingTests.swift
Comment on lines
+12
to
+147
| // Because RedisPluginDriver lives in a plugin bundle and cannot be @testable | ||
| // imported, we replicate the fixed logic here as private helpers. | ||
| // | ||
|
|
||
| import Foundation | ||
| import Testing | ||
|
|
||
| // MARK: - Private Local Helpers (copied from RedisDriverPlugin) | ||
|
|
||
| private enum TestRedisReply { | ||
| case string(String) | ||
| case integer(Int64) | ||
| case array([TestRedisReply]) | ||
| case data(Data) | ||
| case status(String) | ||
| case error(String) | ||
| case null | ||
|
|
||
| var stringValue: String? { | ||
| switch self { | ||
| case .string(let s), .status(let s): return s | ||
| case .data(let d): return String(data: d, encoding: .utf8) | ||
| default: return nil | ||
| } | ||
| } | ||
|
|
||
| var intValue: Int? { | ||
| switch self { | ||
| case .integer(let i): return Int(i) | ||
| case .string(let s): return Int(s) | ||
| default: return nil | ||
| } | ||
| } | ||
|
|
||
| var stringArrayValue: [String]? { | ||
| guard case .array(let items) = self else { return nil } | ||
| return items.compactMap(\.stringValue) | ||
| } | ||
|
|
||
| var arrayValue: [TestRedisReply]? { | ||
| guard case .array(let items) = self else { return nil } | ||
| return items | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Fixed Logic Replicas | ||
|
|
||
| /// Matches the fixed `redisReplyToString` in RedisPluginDriver. | ||
| private func testRedisReplyToString(_ reply: TestRedisReply) -> String { | ||
| switch reply { | ||
| case .string(let s), .status(let s), .error(let s): return s | ||
| case .integer(let i): return String(i) | ||
| case .data(let d): return String(data: d, encoding: .utf8) ?? d.base64EncodedString() | ||
| case .array(let items): return "[\(items.map { testRedisReplyToString($0) }.joined(separator: ", "))]" | ||
| case .null: return "(nil)" | ||
| } | ||
| } | ||
|
|
||
| /// Result type mirroring the relevant fields of PluginQueryResult. | ||
| private struct TestResult { | ||
| let columns: [String] | ||
| let rows: [[String?]] | ||
| } | ||
|
|
||
| private func buildTestHashResult(_ result: TestRedisReply) -> TestResult { | ||
| guard let items = result.arrayValue, !items.isEmpty else { | ||
| return TestResult(columns: ["Field", "Value"], rows: []) | ||
| } | ||
|
|
||
| var rows: [[String?]] = [] | ||
| var i = 0 | ||
| while i + 1 < items.count { | ||
| rows.append([testRedisReplyToString(items[i]), testRedisReplyToString(items[i + 1])]) | ||
| i += 2 | ||
| } | ||
|
|
||
| return TestResult(columns: ["Field", "Value"], rows: rows) | ||
| } | ||
|
|
||
| private func buildTestListResult(_ result: TestRedisReply, startOffset: Int = 0) -> TestResult { | ||
| guard let items = result.arrayValue else { | ||
| return TestResult(columns: ["Index", "Value"], rows: []) | ||
| } | ||
|
|
||
| let rows = items.enumerated().map { index, item -> [String?] in | ||
| [String(startOffset + index), testRedisReplyToString(item)] | ||
| } | ||
|
|
||
| return TestResult(columns: ["Index", "Value"], rows: rows) | ||
| } | ||
|
|
||
| private func buildTestSetResult(_ result: TestRedisReply) -> TestResult { | ||
| guard let items = result.arrayValue else { | ||
| return TestResult(columns: ["Member"], rows: []) | ||
| } | ||
|
|
||
| let rows = items.map { [testRedisReplyToString($0)] as [String?] } | ||
| return TestResult(columns: ["Member"], rows: rows) | ||
| } | ||
|
|
||
| private func buildTestSortedSetResult(_ result: TestRedisReply, withScores: Bool) -> TestResult { | ||
| guard let items = result.arrayValue else { | ||
| return TestResult( | ||
| columns: withScores ? ["Member", "Score"] : ["Member"], | ||
| rows: [] | ||
| ) | ||
| } | ||
|
|
||
| if withScores { | ||
| var rows: [[String?]] = [] | ||
| var i = 0 | ||
| while i + 1 < items.count { | ||
| rows.append([testRedisReplyToString(items[i]), testRedisReplyToString(items[i + 1])]) | ||
| i += 2 | ||
| } | ||
| return TestResult(columns: ["Member", "Score"], rows: rows) | ||
| } else { | ||
| let rows = items.map { [testRedisReplyToString($0)] as [String?] } | ||
| return TestResult(columns: ["Member"], rows: rows) | ||
| } | ||
| } | ||
|
|
||
| private func buildTestConfigResult(_ result: TestRedisReply) -> TestResult { | ||
| guard let items = result.arrayValue, !items.isEmpty else { | ||
| return TestResult(columns: ["Parameter", "Value"], rows: []) | ||
| } | ||
|
|
||
| var rows: [[String?]] = [] | ||
| var i = 0 | ||
| while i + 1 < items.count { | ||
| rows.append([testRedisReplyToString(items[i]), testRedisReplyToString(items[i + 1])]) | ||
| i += 2 | ||
| } | ||
|
|
||
| return TestResult(columns: ["Parameter", "Value"], rows: rows) | ||
| } |
There was a problem hiding this comment.
These suites validate a copy, not RedisPluginDriver.
All assertions in this file run against local replicas of the production builders, so the tests can stay green even if Plugins/RedisDriverPlugin/RedisPluginDriver.swift diverges. That also leaves the changed formatPreviewReply(...) and buildStreamResult(...) paths untested here. Please move the reply-stringification/result-building logic behind a shared testable seam and point these cases at the real implementation.
Also applies to: 151-516
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TableProTests/Core/Redis/RedisResultBuildingTests.swift` around lines 12 -
147, The tests are exercising local replicas (testRedisReplyToString,
buildTestHashResult, buildTestListResult, buildTestSetResult,
buildTestSortedSetResult, buildTestConfigResult) instead of the real builders
(and skip the changed formatPreviewReply and buildStreamResult paths); extract
the reply-stringification/result-building logic into a shared, testable seam
(e.g., move implementations into RedisPluginDriver as internal/static helpers or
a small SharedRedisFormatting type) and update the tests to call those real
functions instead of the local copies; ensure the new helpers expose the same
APIs used here (including formatPreviewReply and buildStreamResult) and make
them `@testable-importable` or placed in a common module so the test file can
reference them directly.
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
build*ResultandformatValuePreviewmethods usedstringArrayValuewhich callscompactMap(\.stringValue)— silently dropping.data(non-UTF8),.null, and.integerentries from Redis repliesarrayValue+redisReplyToString()which handles all reply types (binary → base64 fallback, null → "(nil)", integer → string)Test plan
RedisResultBuildingTests.swift(all passing)test:binary:hash1(has non-UTF8 value) → should show 3 rows with base64 for binary valuetest:bullmq:job:1(JSON values) → should show all 9 fieldsSummary by CodeRabbit
Bug Fixes
Tests