From cd9bd04f737dbf1b998fc2ac13ae19b3cf3d9e82 Mon Sep 17 00:00:00 2001 From: Nathan Harris Date: Mon, 1 Jul 2019 19:14:33 -0700 Subject: [PATCH] Revisit `RESPValue` and `RESPValueConvertible` implementations. Motivation: Johannes provided a fair code review of the project and summarized his findings in issue #48, and one of the prime offenders was all of the `unsafe*` APIs (pointers, buffers, bytes) that were used with `RESPValue` and `RESPValueConvertible`. He also provided great feedback and pointed out good points of confusion with the API design of `RESPValue` and `RESPValueConvertible`. Modifications: - Return to using `Array` instead of `ContiguousArray` for `RESPValue.array` storage - Update all documentation to be more thorough in explaining how the types should be used and conformed to. - Remove all uses of `unsafe*` APIs where possible - Change implementations to be a lot more type and memory safe, double checking assumptions - Remove conformance to `ExpressibleBy*Literal` as it is too easy for users to shoot themselves in the foot and saves only a few characters over `.init(bulk:)` - Create new `RedisNIOTestUtils` target for common test extensions, making them public - Move most almost all implementations of `RESPValue` computed properties into the `RESPValueConvertible` conformances Result: Users should be more safeguarded by the API against unknowingly getting incorrect `RESPValue` representations, the API design of `RESPValue` and `RESPValueConvertible` should be much clearer, and memory safety should be at a higher bar from these changes. This resolves issues #55 & #48, and contributes to issue #47. --- Package.swift | 3 +- Sources/RedisNIO/RESP/RESPTranslator.swift | 2 +- Sources/RedisNIO/RESP/RESPValue.swift | 209 ++++++------------ .../RedisNIO/RESP/RESPValueConvertible.swift | 135 ++++++++--- .../Extensions/General.swift | 15 +- .../Extensions/RedisNIO.swift | 9 +- .../RedisByteDecoderTests.swift | 8 +- .../RedisMessageEncoderTests.swift | 4 +- .../Commands/BasicCommandsTests.swift | 1 + 9 files changed, 204 insertions(+), 182 deletions(-) rename Tests/RedisNIOTests/Utilities/String.swift => Sources/RedisNIOTestUtils/Extensions/General.swift (54%) rename Tests/RedisNIOTests/Utilities/RedisConnection.swift => Sources/RedisNIOTestUtils/Extensions/RedisNIO.swift (67%) diff --git a/Package.swift b/Package.swift index d8d9201..0164afe 100644 --- a/Package.swift +++ b/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"]) ] ) diff --git a/Sources/RedisNIO/RESP/RESPTranslator.swift b/Sources/RedisNIO/RESP/RESPTranslator.swift index 9e911c9..19af8d4 100644 --- a/Sources/RedisNIO/RESP/RESPTranslator.swift +++ b/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 = [] + var results: [RESPValue] = [] results.reserveCapacity(elementCount) for _ in 0..) + 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) + self.init(bulk: value.description) } - 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)) - } -} - -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: 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. + /// The unwrapped value for `.array` representations. + /// - Note: This is a shorthand for `Array.init(fromRESP:)` + public var array: [RESPValue]? { return [RESPValue](fromRESP: self) } + + /// 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 unwrapped value is `.null`. + public var isNull: Bool { + guard case .null = self else { return false } + return true + } + + /// 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 unwrapped `NIO.ByteBuffer` for `.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? { - guard case .array(let array) = self else { return nil } - return array - } - - /// The storage value for `integer` representations. - public var int: Int? { - switch self { - case let .integer(value): return value - default: return nil - } - } - - /// Returns `true` if the value represents a `null` value from Redis. - 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 } } @@ -149,56 +117,15 @@ extension RESPValue { // 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 raw bytes of the `RESPValue` representation. - /// - Important: This will always return `nil` from `.error` and `.null` cases. - public var bytes: [UInt8]? { - 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) - - 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 - } - } - - // 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(UnsafeRawBufferPointer(ptr).bindMemory(to: UInt8.self)) - } + /// 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.init(fromRESP:)`. + public var bytes: [UInt8]? { return [UInt8](fromRESP: self) } } diff --git a/Sources/RedisNIO/RESP/RESPValueConvertible.swift b/Sources/RedisNIO/RESP/RESPValueConvertible.swift index 31d764e..1a555f5 100644 --- a/Sources/RedisNIO/RESP/RESPValueConvertible.swift +++ b/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(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) } } diff --git a/Tests/RedisNIOTests/Utilities/String.swift b/Sources/RedisNIOTestUtils/Extensions/General.swift similarity index 54% rename from Tests/RedisNIOTests/Utilities/String.swift rename to Sources/RedisNIOTestUtils/Extensions/General.swift index d461490..8d738ea 100644 --- a/Tests/RedisNIOTests/Utilities/String.swift +++ b/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 + } } diff --git a/Tests/RedisNIOTests/Utilities/RedisConnection.swift b/Sources/RedisNIOTestUtils/Extensions/RedisNIO.swift similarity index 67% rename from Tests/RedisNIOTests/Utilities/RedisConnection.swift rename to Sources/RedisNIOTestUtils/Extensions/RedisNIO.swift index 8d999ba..8fa983c 100644 --- a/Tests/RedisNIOTests/Utilities/RedisConnection.swift +++ b/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 { + /// 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 { let env = ProcessInfo.processInfo.environment return Redis.makeConnection( to: try .makeAddressResolvingHost( diff --git a/Tests/RedisNIOTests/ChannelHandlers/RedisByteDecoderTests.swift b/Tests/RedisNIOTests/ChannelHandlers/RedisByteDecoderTests.swift index 25823b8..0e6c8f0 100644 --- a/Tests/RedisNIOTests/ChannelHandlers/RedisByteDecoderTests.swift +++ b/Tests/RedisNIOTests/ChannelHandlers/RedisByteDecoderTests.swift @@ -91,7 +91,7 @@ extension RedisByteDecoderTests { } func testArrays() throws { - func runArrayTest(_ input: String) throws -> ContiguousArray? { + 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?, - expected right: ContiguousArray + _ lhs: [RESPValue]?, + expected right: [RESPValue] ) -> Bool { guard let left = lhs, diff --git a/Tests/RedisNIOTests/ChannelHandlers/RedisMessageEncoderTests.swift b/Tests/RedisNIOTests/ChannelHandlers/RedisMessageEncoderTests.swift index f9855d7..96d5ced 100644 --- a/Tests/RedisNIOTests/ChannelHandlers/RedisMessageEncoderTests.swift +++ b/Tests/RedisNIOTests/ChannelHandlers/RedisMessageEncoderTests.swift @@ -53,11 +53,11 @@ final class RedisMessageEncoderTests: XCTestCase { try runEncodePass(with: bs1) { XCTAssertEqual($0.readableBytes, 11) } XCTAssertNoThrow(try self.channel.writeOutbound(bs1)) - let bs2: RESPValue = "®in§³¾" + let bs2: RESPValue = .init(bulk: "®in§³¾") try runEncodePass(with: bs2) { XCTAssertEqual($0.readableBytes, 17) } XCTAssertNoThrow(try self.channel.writeOutbound(bs2)) - let bs3: RESPValue = "" + let bs3: RESPValue = .init(bulk: "") try runEncodePass(with: bs3) { XCTAssertEqual($0.readableBytes, 6) } XCTAssertNoThrow(try self.channel.writeOutbound(bs3)) } diff --git a/Tests/RedisNIOTests/Commands/BasicCommandsTests.swift b/Tests/RedisNIOTests/Commands/BasicCommandsTests.swift index 10d8003..f663f93 100644 --- a/Tests/RedisNIOTests/Commands/BasicCommandsTests.swift +++ b/Tests/RedisNIOTests/Commands/BasicCommandsTests.swift @@ -13,6 +13,7 @@ //===----------------------------------------------------------------------===// @testable import RedisNIO +import RedisNIOTestUtils import XCTest final class BasicCommandsTests: XCTestCase {