Refactor URIDecoder/URIParser to improve handling of the deepObject s…

…tyle (#127)

### Motivation

As part of apple/swift-openapi-generator#259,
adding deepObject parameter style support, the initial PR wasn't
complete.

And once we dug more into it, turns out the original implementation of
the URIDecoder/URIParser didn't really lend themselves well for handling
deepObject, and the recent additions of supporting arrays within
dictionaries (#120)
further confused the implementation.

### Modifications

Refactored URIParser/URIDecoder with a clearer understanding of the
current requirements. It's now much easier to follow and embraces the
fact that each of the 7 variants of URI coding we support (form
exploded, form unexploded, simple exploded, simple unexploded, form data
exploded, form data unexploded, and now deepObject exploded) are
similar, but still different in subtle ways.

This new implementation doesn't try as hard to share code between the
implementations, so might at first sight appear to duplicate code. The
original implementation had many methods with many configuration
parameters and utility methods with a high cyclomatic complexity, which
made it very hard to reason about. We did away with that.

While there, I also made some minor improvements to the serialization
path, which allows cleaner round-tripping tests.

### Result

A more maintainable and more correct URI decoder/parser implementation.

### Test Plan

Added many more unit tests that test the full matrix of supported styles
and inputs.

---------

Co-authored-by: Si Beaumont <simonjbeaumont@gmail.com>

Sources/OpenAPIRuntime/Conversion/ParameterStyles.swift

@@ -14,7 +14,7 @@
 
 /// The serialization style used by a parameter.
 ///
-/// Details: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.3.md#fixed-fields-10
+/// Details: https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.4.md#fixed-fields-10
 @_spi(Generated) public enum ParameterStyle: Sendable {
 
     /// The form style.
@@ -26,9 +26,10 @@
     ///
     /// Details: https://datatracker.ietf.org/doc/html/rfc6570#section-3.2.2
     case simple
+
     /// The deepObject style.
     ///
-    /// Details: https://spec.openapis.org/oas/v3.1.0.html#style-values
+    /// Details: https://spec.openapis.org/oas/v3.0.4.html#style-values
     case deepObject
 }
 

Sources/OpenAPIRuntime/URICoder/Common/URICoderConfiguration.swift

@@ -17,15 +17,22 @@ import Foundation
 /// A bag of configuration values used by the URI encoder and decoder.
 struct URICoderConfiguration {
 
-    /// A variable expansion style as described by RFC 6570 and OpenAPI 3.0.3.
+    /// A variable expansion style as described by RFC 6570 and OpenAPI 3.0.4.
     enum Style {
 
         /// A style for simple string variable expansion.
+        ///
+        /// The whole string always belongs to the root key.
         case simple
 
         /// A style for form-based URI expansion.
+        ///
+        /// Only some key/value pairs can belong to the root key, rest are ignored.
         case form
+
         /// A style for nested variable expansion
+        ///
+        /// Only some key/value pairs can belong to the root key, rest are ignored.
         case deepObject
     }
 
@@ -43,7 +50,7 @@ struct URICoderConfiguration {
     var style: Style
 
     /// A Boolean value indicating whether the key should be repeated with
-    /// each value, as described by RFC 6570 and OpenAPI 3.0.3.
+    /// each value, as described by RFC 6570 and OpenAPI 3.0.4.
     var explode: Bool
 
     /// The character used to escape the space character.

Sources/OpenAPIRuntime/URICoder/Common/URIEncodedNode.swift

@@ -73,6 +73,10 @@ extension URIEncodedNode {
         /// The encoder appended to a node that wasn't an array.
         case appendingToNonArrayContainer
 
+        /// The encoder is trying to mark a container as array, but it's already
+        /// marked as a container of another type.
+        case markingExistingNonArrayContainerAsArray
+
         /// The encoder inserted a value for key into a node that wasn't
         /// a dictionary.
         case insertingChildValueIntoNonContainer
@@ -128,6 +132,18 @@ extension URIEncodedNode {
         }
     }
 
+    /// Marks the node as an array, starting as empty.
+    /// - Throws: If the node is already set to be anything else but an array.
+    mutating func markAsArray() throws {
+        switch self {
+        case .array:
+            // Already an array.
+            break
+        case .unset: self = .array([])
+        default: throw InsertionError.markingExistingNonArrayContainerAsArray
+        }
+    }
+
     /// Appends a value to the array node.
     /// - Parameter childValue: The node to append to the underlying array.
     /// - Throws: If the node is already set to be anything else but an array.

Sources/OpenAPIRuntime/URICoder/Common/URIParsedTypes.swift

@@ -0,0 +1,63 @@
+//===----------------------------------------------------------------------===//
+//
+// This source file is part of the SwiftOpenAPIGenerator open source project
+//
+// Copyright (c) 2023 Apple Inc. and the SwiftOpenAPIGenerator project authors
+// Licensed under Apache License v2.0
+//
+// See LICENSE.txt for license information
+// See CONTRIBUTORS.txt for the list of SwiftOpenAPIGenerator project authors
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+//===----------------------------------------------------------------------===//
+
+import Foundation
+
+/// A component of a `URIParsedKey`.
+typealias URIParsedKeyComponent = String.SubSequence
+
+/// A parsed key for a parsed value.
+///
+/// For example, `foo=bar` in a `form` string would parse the key as `foo` (single component).
+/// In an unexploded `form` string `root=foo,bar`, the key would be `root/foo` (two components).
+/// In a `simple` string `bar`, the key would be empty (0 components).
+struct URIParsedKey: Hashable {
+
+    /// The individual string components.
+    let components: [URIParsedKeyComponent]
+
+    /// Creates a new parsed key.
+    /// - Parameter components: The key components.
+    init(_ components: [URIParsedKeyComponent]) { self.components = components }
+
+    /// A new empty key.
+    static var empty: Self { .init([]) }
+}
+
+/// A primitive value produced by `URIParser`.
+typealias URIParsedValue = String.SubSequence
+
+/// A key-value produced by `URIParser`.
+struct URIParsedPair: Equatable {
+
+    /// The key of the pair.
+    ///
+    /// In `foo=bar`, `foo` is the key.
+    var key: URIParsedKey
+
+    /// The value of the pair.
+    ///
+    /// In `foo=bar`, `bar` is the value.
+    var value: URIParsedValue
+}
+
+// MARK: - Extensions
+
+extension URIParsedKey: CustomStringConvertible {
+    // swift-format-ignore: AllPublicDeclarationsHaveDocumentation
+    var description: String {
+        if components.isEmpty { return "<empty>" }
+        return components.joined(separator: "/")
+    }
+}

Sources/OpenAPIRuntime/URICoder/Decoding/URIDecoder.swift

@@ -15,7 +15,7 @@
 import Foundation
 
 /// A type that decodes a `Decodable` value from an URI-encoded string
-/// using the rules from RFC 6570, RFC 1866, and OpenAPI 3.0.3, depending on
+/// using the rules from RFC 6570, RFC 1866, and OpenAPI 3.0.4, depending on
 /// the configuration.
 ///
 /// [RFC 6570 - Form-style query expansion.](https://datatracker.ietf.org/doc/html/rfc6570#section-3.2.8)
@@ -45,6 +45,13 @@ import Foundation
 /// | `{list\*}`       | `red,green,blue`                  |
 /// | `{keys}`         | `semi,%3B,dot,.,comma,%2C`        |
 /// | `{keys\*}`       | `semi=%3B,dot=.,comma=%2C`        |
+///
+/// [OpenAPI 3.0.4 - Deep object expansion.](https://github.com/OAI/OpenAPI-Specification/blob/main/versions/3.0.4.md#style-examples)
+///
+/// | Example Template |   Expansion                                               |
+/// | ---------------- | ----------------------------------------------------------|
+/// | `{?keys\*}`      | `?keys%5Bsemi%5D=%3B&keys%5Bdot%5D=.&keys%5Bcomma%5D=%2C` |
+///
 struct URIDecoder: Sendable {
 
     /// The configuration instructing the decoder how to interpret the raw
@@ -60,10 +67,6 @@ extension URIDecoder {
 
     /// Attempt to decode an object from an URI string.
     ///
-    /// Under the hood, `URIDecoder` first parses the string into a
-    /// `URIParsedNode` using `URIParser`, and then uses
-    /// `URIValueFromNodeDecoder` to decode the `Decodable` value.
-    ///
     /// - Parameters:
     ///   - type: The type to decode.
     ///   - key: The key of the decoded value. Only used with certain styles
@@ -72,15 +75,12 @@ extension URIDecoder {
     /// - Returns: The decoded value.
     /// - Throws: An error if decoding fails, for example, due to incompatible data or key.
     func decode<T: Decodable>(_ type: T.Type = T.self, forKey key: String = "", from data: Substring) throws -> T {
-        try withCachedParser(from: data) { decoder in try decoder.decode(type, forKey: key) }
+        let decoder = URIValueFromNodeDecoder(data: data, rootKey: key[...], configuration: configuration)
+        return try decoder.decodeRoot(type)
     }
 
     /// Attempt to decode an object from an URI string, if present.
     ///
-    /// Under the hood, `URIDecoder` first parses the string into a
-    /// `URIParsedNode` using `URIParser`, and then uses
-    /// `URIValueFromNodeDecoder` to decode the `Decodable` value.
-    ///
     /// - Parameters:
     ///   - type: The type to decode.
     ///   - key: The key of the decoded value. Only used with certain styles
@@ -90,76 +90,8 @@ extension URIDecoder {
     /// - Throws: An error if decoding fails, for example, due to incompatible data or key.
     func decodeIfPresent<T: Decodable>(_ type: T.Type = T.self, forKey key: String = "", from data: Substring) throws
         -> T?
-    { try withCachedParser(from: data) { decoder in try decoder.decodeIfPresent(type, forKey: key) } }
-
-    /// Make multiple decode calls on the parsed URI.
-    ///
-    /// Use to avoid repeatedly reparsing the raw string.
-    /// - Parameters:
-    ///   - data: The URI-encoded string.
-    ///   - calls: The closure that contains 0 or more calls to
-    ///     the `decode` method on `URICachedDecoder`.
-    /// - Returns: The result of the closure invocation.
-    /// - Throws: An error if parsing or decoding fails.
-    func withCachedParser<R>(from data: Substring, calls: (URICachedDecoder) throws -> R) throws -> R {
-        var parser = URIParser(configuration: configuration, data: data)
-        let parsedNode = try parser.parseRoot()
-        let decoder = URICachedDecoder(configuration: configuration, node: parsedNode)
-        return try calls(decoder)
-    }
-}
-
-struct URICachedDecoder {
-
-    /// The configuration used by the decoder.
-    fileprivate let configuration: URICoderConfiguration
-
-    /// The node from which to decode a value on demand.
-    fileprivate let node: URIParsedNode
-
-    /// Attempt to decode an object from an URI-encoded string.
-    ///
-    /// Under the hood, `URICachedDecoder` already has a pre-parsed
-    /// `URIParsedNode` and uses `URIValueFromNodeDecoder` to decode
-    /// the `Decodable` value.
-    ///
-    /// - Parameters:
-    ///   - type: The type to decode.
-    ///   - key: The key of the decoded value. Only used with certain styles
-    ///     and explode options, ignored otherwise.
-    /// - Returns: The decoded value.
-    /// - Throws: An error if decoding fails.
-    func decode<T: Decodable>(_ type: T.Type = T.self, forKey key: String = "") throws -> T {
-        let decoder = URIValueFromNodeDecoder(
-            node: node,
-            rootKey: key[...],
-            style: configuration.style,
-            explode: configuration.explode,
-            dateTranscoder: configuration.dateTranscoder
-        )
-        return try decoder.decodeRoot()
-    }
-
-    /// Attempt to decode an object from an URI-encoded string, if present.
-    ///
-    /// Under the hood, `URICachedDecoder` already has a pre-parsed
-    /// `URIParsedNode` and uses `URIValueFromNodeDecoder` to decode
-    /// the `Decodable` value.
-    ///
-    /// - Parameters:
-    ///   - type: The type to decode.
-    ///   - key: The key of the decoded value. Only used with certain styles
-    ///     and explode options, ignored otherwise.
-    /// - Returns: The decoded value.
-    /// - Throws: An error if decoding fails.
-    func decodeIfPresent<T: Decodable>(_ type: T.Type = T.self, forKey key: String = "") throws -> T? {
-        let decoder = URIValueFromNodeDecoder(
-            node: node,
-            rootKey: key[...],
-            style: configuration.style,
-            explode: configuration.explode,
-            dateTranscoder: configuration.dateTranscoder
-        )
-        return try decoder.decodeRootIfPresent()
+    {
+        let decoder = URIValueFromNodeDecoder(data: data, rootKey: key[...], configuration: configuration)
+        return try decoder.decodeRootIfPresent(type)
     }
 }

Sources/OpenAPIRuntime/URICoder/Decoding/URIValueFromNodeDecoder+Keyed.swift

@@ -19,9 +19,6 @@ struct URIKeyedDecodingContainer<Key: CodingKey> {
 
     /// The associated decoder.
     let decoder: URIValueFromNodeDecoder
-
-    /// The underlying dictionary.
-    let values: URIParsedNode
 }
 
 extension URIKeyedDecodingContainer {
@@ -32,7 +29,7 @@ extension URIKeyedDecodingContainer {
     /// - Returns: The value found for the provided key.
     /// - Throws: An error if no value for the key was found.
     private func _decodeValue(forKey key: Key) throws -> URIParsedValue {
-        guard let value = values[key.stringValue[...]]?.first else {
+        guard let value = try decoder.nestedElementInCurrentDictionary(forKey: key.stringValue) else {
             throw DecodingError.keyNotFound(key, .init(codingPath: codingPath, debugDescription: "Key not found."))
         }
         return value
@@ -97,9 +94,9 @@ extension URIKeyedDecodingContainer {
 
 extension URIKeyedDecodingContainer: KeyedDecodingContainerProtocol {
 
-    var allKeys: [Key] { values.keys.map { key in Key.init(stringValue: String(key))! } }
+    var allKeys: [Key] { decoder.elementKeysInCurrentDictionary().compactMap { .init(stringValue: $0) } }
 
-    func contains(_ key: Key) -> Bool { values[key.stringValue[...]] != nil }
+    func contains(_ key: Key) -> Bool { decoder.containsElementInCurrentDictionary(forKey: key.stringValue) }
 
     var codingPath: [any CodingKey] { decoder.codingPath }
 
@@ -153,7 +150,7 @@ extension URIKeyedDecodingContainer: KeyedDecodingContainerProtocol {
         case is UInt64.Type: return try decode(UInt64.self, forKey: key) as! T
         case is Date.Type: return try decoder.dateTranscoder.decode(String(_decodeValue(forKey: key))) as! T
         default:
-            try decoder.push(.init(key))
+            decoder.push(.init(key))
             defer { decoder.pop() }
             return try type.init(from: decoder)
         }