Detect and throw on invalid integer.

Motivation:

parseInteger did not distinguish between not having enough bytes for an
integer and not being able to parse the integer that was present. This
was a bit tricky for code internally, where some call sites had extra
code looking for spooky action at a distance in order to determine if
the integer failed to parse.

This is unnecessary: parseInteger is sufficiently aware of what's going
on to address this problem itself.

Modifications:

- Added a new parser error (acceptable as we haven't tagged 1.0 yet).
- Throw it from parseInteger if the integer is invalid.

Result:

parseInteger clearly communicates if the integer failed to parse.

Author: Lukasa

Sources/RediStack/RESP/RESPTranslator.swift

@@ -104,13 +104,15 @@ extension RESPTranslator {
         case invalidToken
         case invalidBulkStringSize
         case bulkStringSizeMismatch
+        case invalidIntegerFormat
 
         /// See `LocalizedError.errorDescription`
         public var errorDescription: String? {
             switch self {
             case .invalidToken: return "Cannot parse RESP: Invalid Token"
             case .invalidBulkStringSize: return "Cannot parse RESP Bulk String: Received invalid size."
             case .bulkStringSizeMismatch: return "Cannot parse RESP Bulk String: Declared Size and Content Size do not match."
+            case .invalidIntegerFormat: return "Cannot parse RESP integer: invalid integer format"
             }
         }
     }
@@ -133,7 +135,7 @@ extension RESPTranslator {
             result = .simpleString(value)
 
         case .colon:
-            guard let value = self.parseInteger(from: &copy) else { return nil }
+            guard let value = try self.parseInteger(from: &copy) else { return nil }
             result = .integer(value)
 
         case .dollar:
@@ -182,23 +184,19 @@ extension RESPTranslator {
     }
 
     /// See [https://redis.io/topics/protocol#resp-integers](https://redis.io/topics/protocol#resp-integers)
-    internal func parseInteger(from buffer: inout ByteBuffer) -> Int? {
+    internal func parseInteger(from buffer: inout ByteBuffer) throws -> Int? {
         guard
             var stringBuffer = parseSimpleString(from: &buffer),
             let string = stringBuffer.readString(length: stringBuffer.readableBytes)
         else { return nil }
-        return Int(string)
+
+        guard let result = Int(string) else { throw ParsingError.invalidIntegerFormat }
+        return result
     }
 
     /// See [https://redis.io/topics/protocol#resp-bulk-strings](https://redis.io/topics/protocol#resp-bulk-strings)
     internal func parseBulkString(from buffer: inout ByteBuffer) throws -> RESPValue? {
-        let startingReaderIndex = buffer.readerIndex
-        
-        guard let size = self.parseInteger(from: &buffer) else {
-            // if the reader index changed, that means that a valid string parse happened, but it's not a number.
-            guard startingReaderIndex == buffer.readerIndex else {
-                throw ParsingError.invalidBulkStringSize
-            }
+        guard let size = try self.parseInteger(from: &buffer) else {
             return nil
         }
 
@@ -237,7 +235,7 @@ extension RESPTranslator {
 
     /// See [https://redis.io/topics/protocol#resp-arrays](https://redis.io/topics/protocol#resp-arrays)
     internal func parseArray(from buffer: inout ByteBuffer) throws -> RESPValue? {
-        guard let elementCount = parseInteger(from: &buffer) else { return nil }
+        guard let elementCount = try parseInteger(from: &buffer) else { return nil }
         guard elementCount > -1 else { return .null } // '*-1\r\n'
         guard elementCount > 0 else { return .array([]) } // '*0\r\n'
 

Tests/RediStackTests/RESPTranslatorTests.swift

@@ -317,41 +317,49 @@ extension RESPTranslatorTests {
 
 extension RESPTranslatorTests {
     func testParsing_integer_withAllBytes() {
-        XCTAssertEqual(integerParseTest(":100\r\n"), 100)
-        XCTAssertEqual(integerParseTest(":-100\r\n"), -100)
-        XCTAssertEqual(integerParseTest(":\(Int.min)\r\n"), Int.min)
-        XCTAssertEqual(integerParseTest(":\(Int.max)\r\n"), Int.max)
+        XCTAssertEqual(try integerParseTest(":100\r\n"), 100)
+        XCTAssertEqual(try integerParseTest(":-100\r\n"), -100)
+        XCTAssertEqual(try integerParseTest(":\(Int.min)\r\n"), Int.min)
+        XCTAssertEqual(try integerParseTest(":\(Int.max)\r\n"), Int.max)
     }
 
     func testParsing_integer_missingBytes() {
-        XCTAssertNil(integerParseTest(":\r"))
-        XCTAssertNil(integerParseTest(":"))
-        XCTAssertNil(integerParseTest(":\n"))
-        XCTAssertNil(integerParseTest(":\r\n"))
+        XCTAssertNil(try integerParseTest(":\r"))
+        XCTAssertNil(try integerParseTest(":"))
+        XCTAssertNil(try integerParseTest(":\n"))
+        XCTAssertThrowsError(try integerParseTest(":\r\n")) { error in
+            XCTAssertEqual(error as? RESPTranslator.ParsingError, .invalidIntegerFormat)
+        }
     }
 
-    func testParsing_integer_recursively() {
+    func testParsing_integer_recursively() throws {
         let testString = ":1\r\n:300\r\n"
 
         var buffer = allocator.buffer(capacity: testString.count)
         buffer.writeString(testString)
 
         buffer.mimicTokenParse()
-        let first = parser.parseInteger(from: &buffer)
+        let first = try parser.parseInteger(from: &buffer)
         XCTAssertEqual(buffer.readerIndex, 4) // position of the 2nd ':'
         XCTAssertEqual(first, 1)
 
         buffer.mimicTokenParse()
-        let second = parser.parseInteger(from: &buffer)
+        let second = try parser.parseInteger(from: &buffer)
         XCTAssertEqual(buffer.readerIndex, 10)
         XCTAssertEqual(second, 300)
     }
+
+    func testParsing_integer_nonBase10() throws {
+        XCTAssertThrowsError(try integerParseTest(":0xa\r\n")) { error in
+            XCTAssertEqual(error as? RESPTranslator.ParsingError, .invalidIntegerFormat)
+        }
+    }
 
-    private func integerParseTest(_ inputRESP: String) -> Int? {
+    private func integerParseTest(_ inputRESP: String) throws -> Int? {
         var buffer = allocator.buffer(capacity: inputRESP.count)
         buffer.writeString(inputRESP)
         buffer.mimicTokenParse()
-        return parser.parseInteger(from: &buffer)
+        return try parser.parseInteger(from: &buffer)
     }
 }
 
@@ -381,7 +389,7 @@ extension RESPTranslatorTests {
         buffer.writeString("$FOO\r\nwhat\r\n")
         buffer.mimicTokenParse()
         XCTAssertThrowsError(try self.parser.parseBulkString(from: &buffer)) { error in
-            XCTAssertEqual(error as? RESPTranslator.ParsingError, .invalidBulkStringSize)
+            XCTAssertEqual(error as? RESPTranslator.ParsingError, .invalidIntegerFormat)
         }
     }
 

Tests/RediStackTests/XCTestManifests.swift

@@ -28,6 +28,7 @@ extension RESPTranslatorTests {
         ("testParsing_error", testParsing_error),
         ("testParsing_integer_chunked", testParsing_integer_chunked),
         ("testParsing_integer_missingBytes", testParsing_integer_missingBytes),
+        ("testParsing_integer_nonBase10", testParsing_integer_nonBase10),
         ("testParsing_integer_recursively", testParsing_integer_recursively),
         ("testParsing_integer_withAllBytes", testParsing_integer_withAllBytes),
         ("testParsing_integer", testParsing_integer),