swift-server/RediStack
1
0
Fork 0
mirror of https://github.com/swift-server/RediStack.git synced 2026-06-02 07:37:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Revisit RESPValue and RESPValueConvertible implementations.

Browse Source 
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.
This commit is contained in:
Nathan Harris
2019-07-02 23:19:15 -07:00
parent f445822c23
commit cd9bd04f73
9 changed files with 204 additions and 182 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Package.swift
+2 -1
View File
@@ -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
+1 -1
View File
@@ -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
+68 -141
View File
@@ -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,105 +32,83 @@ 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)
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: 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.
/// 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 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<RESPValue>? {
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<UInt8>(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<UInt8>.init(fromRESP:)`.
public var bytes: [UInt8]? { return [UInt8](fromRESP: self) }
}
Sources/RedisNIO/RESP/RESPValueConvertible.swift
+107 -28
View File
@@ -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 -3
View File
@@ -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
+7 -2
View File
@@ -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
+4 -4
View File
@@ -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
+2 -2
View File
@@ -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))
}
Tests/RedisNIOTests/Commands/BasicCommandsTests.swift
+1
View File
@@ -13,6 +13,7 @@
//===----------------------------------------------------------------------===//
@testable import RedisNIO
import RedisNIOTestUtils
import XCTest
final class BasicCommandsTests: XCTestCase {