Merge branch 'cleanup-unsafe' into '47-proposal-feedback'

Revisit `RESPValue` and `RESPValueConvertible` implementations.

See merge request Mordil/swift-redis-nio-client!67

Package.swift

@@ -27,6 +27,7 @@ let package = Package(
     ],
     targets: [
         .target(name: "RedisNIO", dependencies: ["NIO", "Logging", "Metrics"]),
-        .testTarget(name: "RedisNIOTests", dependencies: ["RedisNIO", "NIO"])
+        .target(name: "RedisNIOTestUtils", dependencies: ["NIO", "RedisNIO"]),
+        .testTarget(name: "RedisNIOTests", dependencies: ["RedisNIO", "NIO", "RedisNIOTestUtils"])
     ]
 )

Sources/RedisNIO/RESP/RESPTranslator.swift

@@ -224,7 +224,7 @@ extension RESPTranslator {
         guard elementCount > -1 else { return .null } // '*-1\r\n'
         guard elementCount > 0 else { return .array([]) } // '*0\r\n'
         
-        var results: ContiguousArray<RESPValue> = []
+        var results: [RESPValue] = []
         results.reserveCapacity(elementCount)
         
         for _ in 0..<elementCount {

Sources/RedisNIO/RESP/RESPValue.swift

@@ -15,26 +15,16 @@
 import struct Foundation.Data
 import NIO
 
-extension String {
-    @inline(__always)
-    var byteBuffer: ByteBuffer {
-        var buffer = RESPValue.allocator.buffer(capacity: self.count)
-        buffer.writeString(self)
-        return buffer
-    }
-}
-
-extension Data {
-    @inline(__always)
-    var byteBuffer: ByteBuffer {
-        var buffer = RESPValue.allocator.buffer(capacity: self.count)
-        buffer.writeBytes(self)
-        return buffer
-    }
-}
-
 /// A representation of a Redis Serialization Protocol (RESP) primitive value.
 ///
+/// This enum representation should be used only as a temporary intermediate representation of values, and should be sent to a Redis server or converted to Swift
+/// types as soon as possible.
+///
+/// Redis servers expect a single message packed into an `.array`, with all elements being `.bulkString` representations of values. As such, all initializers
+/// convert to `.bulkString` representations, as well as default conformances for `RESPValueConvertible`.
+///
+/// Each case of this type is a different listing in the RESP specification, and several computed properties are available to consistently convert values into Swift types.
+///
 /// See: [https://redis.io/topics/protocol](https://redis.io/topics/protocol)
 public enum RESPValue {
     case null
@@ -42,163 +32,100 @@ public enum RESPValue {
     case bulkString(ByteBuffer?)
     case error(RedisError)
     case integer(Int)
-    case array(ContiguousArray<RESPValue>)
+    case array([RESPValue])
 
-    fileprivate static let allocator = ByteBufferAllocator()
+    /// A `NIO.ByteBufferAllocator` for use in creating `.simpleString` and `.bulkString` representations directly, if needed.
+    static let allocator = ByteBufferAllocator()
 
-    /// Initializes a `bulkString` by converting the provided string input.
-    public init(bulk value: String? = nil) {
-        self = .bulkString(value?.byteBuffer)
+    /// Initializes a `bulkString` value.
+    /// - Parameter value: The `String` to store in a `.bulkString` representation.
+    public init(bulk value: String) {
+        var buffer = RESPValue.allocator.buffer(capacity: value.count)
+        buffer.writeString(value)
+        self = .bulkString(buffer)
     }
 
+    /// Initializes a `bulkString` value.
+    /// - Parameter value: The `Int` value to store in a `.bulkString` representation.
     public init(bulk value: Int) {
-        self = .bulkString(value.description.byteBuffer)
-    }
-
-    public init(_ source: RESPValueConvertible) {
-        self = source.convertedToRESPValue()
-    }
-}
-
-// MARK: Expressible by Literals
-
-extension RESPValue: ExpressibleByStringLiteral {
-    /// Initializes a bulk string from a String literal
-    public init(stringLiteral value: String) {
-        self = .bulkString(value.byteBuffer)
-    }
-}
-
-extension RESPValue: ExpressibleByArrayLiteral {
-    /// Initializes an array from an Array literal
-    public init(arrayLiteral elements: RESPValue...) {
-        self = .array(.init(elements))
+        self.init(bulk: value.description)
     }
-}
 
-extension RESPValue: ExpressibleByNilLiteral {
-    /// Initializes null from a nil literal
-    public init(nilLiteral: ()) {
-        self = .null
-    }
-}
-
-extension RESPValue: ExpressibleByIntegerLiteral {
-    /// Initializes an integer from an integer literal
-    public init(integerLiteral value: Int) {
-        self = .integer(value)
+    /// Stores the representation determined by the `RESPValueConvertible` value.
+    /// - Important: If you are sending this value to a Redis server, the type should be convertible to a `.bulkString`.
+    /// - Parameter value: The value that needs to be converted and stored in `RESPValue` format.
+    public init<Value: RESPValueConvertible>(_ value: Value) {
+        self = value.convertedToRESPValue()
     }
 }
 
 // MARK: Custom String Convertible
 
 extension RESPValue: CustomStringConvertible {
+    /// See `CustomStringConvertible.description`
     public var description: String {
         switch self {
-        case .integer, .simpleString, .bulkString: return self.string!
+        case let .simpleString(buffer),
+             let .bulkString(.some(buffer)):
+            guard let value = String(fromRESP: self) else { return "\(buffer)" } // default to ByteBuffer's representation
+            return value
+
+        // .integer, .error, and .bulkString(.none) conversions to String always succeed
+        case .integer,
+             .bulkString(.none):
+            return String(fromRESP: self)!
+            
         case .null: return "NULL"
-        case let .array(elements): return "[\(elements.map({ $0.description }).joined(separator: ","))]"
         case let .error(e): return e.message
+        case let .array(elements): return "[\(elements.map({ $0.description }).joined(separator: ","))]"
         }
     }
 }
 
-// MARK: Computed Values
+// MARK: Unwrapped Values
 
 extension RESPValue {
-    /// The `ByteBuffer` storage for either `.simpleString` or `.bulkString` representations.
-    public var byteBuffer: ByteBuffer? {
-        switch self {
-        case let .simpleString(buffer),
-             let .bulkString(.some(buffer)): return buffer
-        default: return nil
-        }
-    }
-
-    /// The storage value for `array` representations.
-    public var array: ContiguousArray<RESPValue>? {
-        guard case .array(let array) = self else { return nil }
-        return array
-    }
+    /// The unwrapped value for `.array` representations.
+    /// - Note: This is a shorthand for `Array<RESPValue>.init(fromRESP:)`
+    public var array: [RESPValue]? { return [RESPValue](fromRESP: self) }
 
-    /// The storage value for `integer` representations.
-    public var int: Int? {
-        switch self {
-        case let .integer(value): return value
-        default: return nil
-        }
-    }
+    /// The unwrapped value as an `Int`.
+    /// - Note: This is a shorthand for `Int(fromRESP:)`.
+    public var int: Int? { return Int(fromRESP: self) }
 
-    /// Returns `true` if the value represents a `null` value from Redis.
+    /// Returns `true` if the unwrapped value is `.null`.
     public var isNull: Bool {
-        switch self {
-        case .null: return true
-        default: return false
-        }
-    }
-
-    /// The error returned from Redis.
-    public var error: RedisError? {
-        switch self {
-        case .error(let error): return error
-        default: return nil
-        }
+        guard case .null = self else { return false }
+        return true
     }
-}
-
-// MARK: Conversion Values
-
-extension RESPValue {
-    /// The `RESPValue` converted to a `String`.
-    /// - Important: This will always return `nil` from `.error`, `.null`, and `array` cases.
-    /// - Note: This creates a `String` using UTF-8 encoding.
-    public var string: String? {
-        switch self {
-        case let .integer(value): return value.description
-        case let .simpleString(buffer),
-             let .bulkString(.some(buffer)):
-            return buffer.getString(at: buffer.readerIndex, length: buffer.readableBytes)
 
-        case .bulkString(.none): return ""
-        default: return nil
-        }
-    }
+    /// The unwrapped `RedisError` that was returned from Redis.
+    /// - Note: This is a shorthand for `RedisError(fromRESP:)`.
+    public var error: RedisError? { return RedisError(fromRESP: self) }
 
-    /// The raw bytes of the `RESPValue` representation.
-    /// - Important: This will always return `nil` from `.error` and `.null` cases.
-    public var bytes: [UInt8]? {
+    /// The unwrapped `NIO.ByteBuffer` for `.simpleString` or `.bulkString` representations.
+    public var byteBuffer: ByteBuffer? {
         switch self {
-        case let .integer(value): return withUnsafeBytes(of: value, RESPValue.copyMemory)
-        case let .array(values): return values.withUnsafeBytes(RESPValue.copyMemory)
         case let .simpleString(buffer),
-             let .bulkString(.some(buffer)):
-            return buffer.getBytes(at: buffer.readerIndex, length: buffer.readableBytes)
+             let .bulkString(.some(buffer)): return buffer
 
-        case .bulkString(.none): return []
         default: return nil
         }
     }
+}
 
-    public var data: Data? {
-        switch self {
-        case let .integer(value): return withUnsafeBytes(of: value, RESPValue.copyMemory)
-        case let .array(values): return values.withUnsafeBytes(RESPValue.copyMemory)
-        case let .simpleString(buffer),
-             let .bulkString(.some(buffer)):
-            return buffer.withUnsafeReadableBytes(RESPValue.copyMemory)
-
-        case .bulkString(.none): return Data()
-        default: return nil
-        }
-    }
+// MARK: Conversion Values
 
-    // SR-9604
-    @inline(__always)
-    private static func copyMemory(_ ptr: UnsafeRawBufferPointer) -> Data {
-        return Data(UnsafeRawBufferPointer(ptr).bindMemory(to: UInt8.self))
-    }
-    @inline(__always)
-    private static func copyMemory(_ ptr: UnsafeRawBufferPointer) -> [UInt8]? {
-        return Array<UInt8>(UnsafeRawBufferPointer(ptr).bindMemory(to: UInt8.self))
-    }
+extension RESPValue {
+    /// The value as a UTF-8 `String` representation.
+    /// - Note: This is a shorthand for `String.init(fromRESP:)`.
+    public var string: String? { return String(fromRESP: self) }
+    
+    /// The data stored in either a `.simpleString` or `.bulkString` represented as `Foundation.Data` instead of `NIO.ByteBuffer`.
+    /// - Note: This is a shorthand for `Data.init(fromRESP:)`.
+    public var data: Data? { return Data(fromRESP: self) }
+    
+    /// The raw bytes stored in the `.simpleString` or `.bulkString` representations.
+    /// - Note: This is a shorthand for `Array<UInt8>.init(fromRESP:)`.
+    public var bytes: [UInt8]? { return [UInt8](fromRESP: self) }
 }

Sources/RedisNIO/RESP/RESPValueConvertible.swift

@@ -12,15 +12,25 @@
 //
 //===----------------------------------------------------------------------===//
 
-/// Capable of converting to / from `RESPValue`.
+/// An object that is capable of being converted to and from `RESPValue` representations arbitrarily.
+/// - Important: When conforming your types to be sent to a Redis server, it is expected to always be stored in a `.bulkString` representation. Redis will
+/// reject any other `RESPValue` type sent to it.
+///
+/// Conforming to this protocol only provides convenience methods of translating the Swift type into a `RESPValue` representation within the driver, and references
+/// to a `RESPValueConvertible` instance should be short lived for that purpose.
+///
+/// See `RESPValue`.
 public protocol RESPValueConvertible {
+    /// Attempts to create a new instance of the conforming type based on the value represented by the `RESPValue`.
+    /// - Parameter value: The `RESPValue` representation to attempt to initialize from.
     init?(fromRESP value: RESPValue)
 
-    /// Creates a `RESPValue` representation.
+    /// Creates a `RESPValue` representation of the conforming type's value.
     func convertedToRESPValue() -> RESPValue
 }
 
 extension RESPValue: RESPValueConvertible {
+    /// See `RESPValueConvertible.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
         self = value
     }
@@ -32,9 +42,12 @@ extension RESPValue: RESPValueConvertible {
 }
 
 extension RedisError: RESPValueConvertible {
+    /// Unwraps an `.error` representation directly into a `RedisError` instance.
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        guard let error = value.error else { return nil }
-        self = error
+        guard case let .error(e) = value else { return nil }
+        self = e
     }
 
     /// See `RESPValueConvertible.convertedToRESPValue()`
@@ -44,9 +57,27 @@ extension RedisError: RESPValueConvertible {
 }
 
 extension String: RESPValueConvertible {
+    /// Attempts to provide a UTF-8 representation of the `RESPValue` provided.
+    ///
+    /// - `.simpleString` and `.bulkString` have their bytes interpeted into a UTF-8 `String`.
+    /// - `.integer` displays the ASCII representation (e.g. 30 converts to "30")
+    /// - `.error` uses the `RedisError.message`
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        guard let string = value.string else { return nil }
-        self = string
+        switch value {
+        case let .simpleString(buffer),
+             let .bulkString(.some(buffer)):
+            guard let string = buffer.getString(at: buffer.readerIndex, length: buffer.readableBytes) else {
+                return nil
+            }
+            self = string
+
+        case .bulkString(.none): self = ""
+        case let .integer(value): self = value.description
+        case let .error(e): self = e.message
+        default: return nil
+        }
     }
 
     /// See `RESPValueConvertible.convertedToRESPValue()`
@@ -56,12 +87,19 @@ extension String: RESPValueConvertible {
 }
 
 extension FixedWidthInteger {
+    /// Attempts to pull an Integer value from the `RESPValue` representation.
+    ///
+    /// If the value is not an `.integer`, it will attempt to create a `String` representation to then attempt to create an Integer from.
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)` and `String.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        if let int = value.int {
+        if case let .integer(int) = value {
             self = Self(int)
         } else {
-            guard let string = value.string else { return nil }
-            guard let int = Self(string) else { return nil }
+            guard
+                let string = String(fromRESP: value),
+                let int = Self(string)
+            else { return nil }
             self = Self(int)
         }
     }
@@ -84,10 +122,17 @@ extension UInt32: RESPValueConvertible {}
 extension UInt64: RESPValueConvertible {}
 
 extension Double: RESPValueConvertible {
+    /// Attempts to translate the `RESPValue` as a `Double`.
+    ///
+    /// This will only succeed if the value is a ASCII representation in a `.simpleString` or `.bulkString`, or is an `.integer`.
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)` and `String.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        guard let string = value.string else { return nil }
-        guard let float = Double(string) else { return nil }
-        self = float
+        guard
+            let string = String(fromRESP: value),
+            let double = Double(string)
+        else { return nil }
+        self = double
     }
 
     /// See `RESPValueConvertible.convertedToRESPValue()`
@@ -97,9 +142,16 @@ extension Double: RESPValueConvertible {
 }
 
 extension Float: RESPValueConvertible {
+    /// Attempts to translate the `RESPValue` as a `Float`.
+    ///
+    /// This will only succeed if the value is a ASCII representation in a `.simpleString` or `.bulkString`, or is an `.integer`.
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)` and `String.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        guard let string = value.string else { return nil }
-        guard let float = Float(string) else { return nil }
+        guard
+            let string = String(fromRESP: value),
+            let float = Float(string)
+            else { return nil }
         self = float
     }
 
@@ -110,33 +162,48 @@ extension Float: RESPValueConvertible {
 }
 
 extension Collection where Element: RESPValueConvertible {
+    /// Converts all elements into their `RESPValue` representation, storing all results into a final `.array` representation.
+    ///
     /// See `RESPValueConvertible.convertedToRESPValue()`
     public func convertedToRESPValue() -> RESPValue {
-        let elements = map { $0.convertedToRESPValue() }
-        let value = elements.withUnsafeBufferPointer {
-            ContiguousArray<RESPValue>(UnsafeRawBufferPointer($0).bindMemory(to: RESPValue.self))
-        }
+        var value: [RESPValue] = []
+        value.reserveCapacity(self.count)
+        self.forEach { value.append($0.convertedToRESPValue()) }
         return .array(value)
     }
 }
 
 extension Array: RESPValueConvertible where Element: RESPValueConvertible {
+    /// Converts all elements into their Swift type, compacting non-`nil` results into a new `Array`.
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        guard let array = value.array else { return nil }
-        self = array.compactMap { Element(fromRESP: $0) }
+        guard case let .array(a) = value else { return nil }
+        self = a.compactMap(Element.init)
     }
 }
 
-extension ContiguousArray: RESPValueConvertible where Element: RESPValueConvertible {
+extension Array where Element == UInt8 {
+    /// Converts the data stored in `.simpleString` and `.bulkString` representations into a raw byte array.
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        guard let array = value.array else { return nil }
-        self = array.compactMap(Element.init).withUnsafeBytes {
-            .init(UnsafeRawBufferPointer($0).bindMemory(to: Element.self))
+        switch value {
+        case let .simpleString(buffer),
+             let .bulkString(.some(buffer)):
+            guard let bytes = buffer.getBytes(at: buffer.readerIndex, length: buffer.readableBytes) else { return nil }
+            self = bytes
+            
+        case .bulkString(.none): self = []
+        default: return nil
         }
     }
 }
 
 extension Optional: RESPValueConvertible where Wrapped: RESPValueConvertible {
+    /// Translates `.null` into `nil`, otherwise the result of `Wrapped.init(fromRESP:)`.
+    ///
+    /// See `RESPValueConvertible.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
         guard !value.isNull else { return nil }
         guard let wrapped = Wrapped(fromRESP: value) else { return nil }
@@ -144,7 +211,9 @@ extension Optional: RESPValueConvertible where Wrapped: RESPValueConvertible {
         self = .some(wrapped)
     }
 
-    /// See `RESPValueConvertible.convertedToRESPValue()`.
+    /// Creates a `.null` representation when `nil`, otherwise the result of `Wrapped.convertedToRESPValue()`.
+    ///
+    /// See `RESPValueConvertible.convertedToRESPValue()`
     public func convertedToRESPValue() -> RESPValue {
         switch self {
         case .none: return .null
@@ -156,12 +225,22 @@ extension Optional: RESPValueConvertible where Wrapped: RESPValueConvertible {
 import struct Foundation.Data
 
 extension Data: RESPValueConvertible {
+    /// See `RESPValueConvertible.init(fromRESP:)`
     public init?(fromRESP value: RESPValue) {
-        guard let data = value.data else { return nil }
-        self = data
+        switch value {
+        case let .simpleString(buffer),
+             let .bulkString(.some(buffer)):
+            self = Data(buffer.readableBytesView)
+            
+        case .bulkString(.none): self = Data()
+        default: return nil
+        }
     }
 
+    /// See `RESPValueConvertible.convertedToRESPValue()`
     public func convertedToRESPValue() -> RESPValue {
-        return .bulkString(self.byteBuffer)
+        var buffer = RESPValue.allocator.buffer(capacity: self.count)
+        buffer.writeBytes(self)
+        return .bulkString(buffer)
     }
 }

Tests/RedisNIOTests/Utilities/String.swift → Sources/RedisNIOTestUtils/Extensions/General.swift

@@ -12,9 +12,18 @@
 //
 //===----------------------------------------------------------------------===//
 
-import Foundation
+import NIO
+
+private let allocator = ByteBufferAllocator()
 
 extension String {
-    /// Converts this String to a byte representation.
-    var bytes: [UInt8] { return .init(self.utf8) }
+    /// The UTF-8 byte representation of the string.
+    public var bytes: [UInt8] { return .init(self.utf8) }
+    
+    /// Creates a `NIO.ByteBuffer` with the string's value written into it.
+    public var byteBuffer: ByteBuffer {
+        var buffer = allocator.buffer(capacity: self.count)
+        buffer.writeString(self)
+        return buffer
+    }
 }

Tests/RedisNIOTests/Utilities/RedisConnection.swift → Sources/RedisNIOTestUtils/Extensions/RedisNIO.swift

@@ -14,10 +14,15 @@
 
 import Foundation
 import NIO
-@testable import RedisNIO
+import RedisNIO
 
 extension Redis {
-    static func makeConnection() throws -> EventLoopFuture<RedisConnection> {
+    /// Creates a `RedisConnection` using `REDIS_URL` and `REDIS_PW` environment variables if available.
+    ///
+    /// The default URL is `127.0.0.1` while the default port is `RedisConnection.defaultPort`.
+    ///
+    /// If `REDIS_PW` is not defined, no authentication will happen on the connection.
+    public static func makeConnection() throws -> EventLoopFuture<RedisConnection> {
         let env = ProcessInfo.processInfo.environment
         return Redis.makeConnection(
             to: try .makeAddressResolvingHost(

Tests/RedisNIOTests/ChannelHandlers/RedisByteDecoderTests.swift

@@ -91,7 +91,7 @@ extension RedisByteDecoderTests {
     }
 
     func testArrays() throws {
-        func runArrayTest(_ input: String) throws -> ContiguousArray<RESPValue>? {
+        func runArrayTest(_ input: String) throws -> [RESPValue]? {
             return try runTest(input)?.array
         }
 
@@ -100,7 +100,7 @@ extension RedisByteDecoderTests {
         XCTAssertEqual(try runArrayTest("*0\r\n")?.count, 0)
         XCTAssertTrue(arraysAreEqual(
             try runArrayTest("*1\r\n$3\r\nfoo\r\n"),
-            expected: ["foo"]
+            expected: [.init(bulk: "foo")]
         ))
         XCTAssertTrue(arraysAreEqual(
             try runArrayTest("*3\r\n+foo\r\n$3\r\nbar\r\n:3\r\n"),
@@ -136,8 +136,8 @@ extension RedisByteDecoderTests {
     }
 
     private func arraysAreEqual(
-        _ lhs: ContiguousArray<RESPValue>?,
-        expected right: ContiguousArray<RESPValue>
+        _ lhs: [RESPValue]?,
+        expected right: [RESPValue]
     ) -> Bool {
         guard
             let left = lhs,

Tests/RedisNIOTests/ChannelHandlers/RedisMessageEncoderTests.swift

@@ -53,11 +53,11 @@ final class RedisMessageEncoderTests: XCTestCase {
         try runEncodePass(with: bs1) { XCTAssertEqual($0