From 8b05ba1f43e20085214e68fbe7dd5221d06e78c6 Mon Sep 17 00:00:00 2001 From: Honza Dvorsky Date: Fri, 27 Oct 2023 12:44:49 +0200 Subject: [PATCH 1/3] Improved content type matching --- .../OpenAPIRuntime/Base/OpenAPIMIMEType.swift | 112 +++++++++++++ .../Conversion/Converter+Common.swift | 75 +++++---- .../Deprecated/Deprecated.swift | 45 ++++++ .../Base/Test_OpenAPIMIMEType.swift | 92 ++++++++++- .../Conversion/Test_Converter+Common.swift | 148 ++++++++++++++++++ 5 files changed, 436 insertions(+), 36 deletions(-) diff --git a/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift b/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift index 7766aa2f..21799410 100644 --- a/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift +++ b/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift @@ -187,3 +187,115 @@ extension OpenAPIMIMEType: LosslessStringConvertible { .joined(separator: "; ") } } + +// MARK: - Internals + +extension OpenAPIMIMEType { + + /// The result of a match evaluation between two MIME types. + enum Match: Hashable { + + /// The reason why two types are incompatible. + enum IncompatibilityReason: Hashable { + + /// The types don't match. + case type + + /// The subtypes don't match. + case subtype + + /// The parameter of the provided name is missing or doesn't match. + case parameter(name: String) + } + + /// The types are incompatible for the provided reason. + case incompatible(IncompatibilityReason) + + /// The types match based on a full wildcard `*/*`. + case wildcard + + /// The types match based on a subtype wildcard, such as `image/*`. + case subtypeWildcard + + /// The types match across the type, subtype, and the provided number + /// of parameters. + case typeAndSubtype(matchedParameterCount: Int) + + /// A numeric representation of the quality of the match, the higher + /// the closer the types are. + var score: Int { + switch self { + case .incompatible: + return 0 + case .wildcard: + return 1 + case .subtypeWildcard: + return 2 + case .typeAndSubtype(let matchedParameterCount): + return 3 + matchedParameterCount + } + } + } + + /// Computes whether two MIME types match. + /// - Parameters: + /// - receivedType: The type component of the received MIME type. + /// - receivedSubtype: The subtype component of the received MIME type. + /// - receivedParameters: The parameters of the received MIME type. + /// - option: The MIME type to match against. + /// - Returns: The match result. + static func evaluate( + receivedType: String, + receivedSubtype: String, + receivedParameters: [String: String], + against option: OpenAPIMIMEType + ) -> Match { + switch option.kind { + case .any: + return .wildcard + case .anySubtype(let expectedType): + guard receivedType.lowercased() == expectedType.lowercased() else { + return .incompatible(.type) + } + return .subtypeWildcard + case .concrete(let expectedType, let expectedSubtype): + guard + receivedType.lowercased() == expectedType.lowercased() + && receivedSubtype.lowercased() == expectedSubtype.lowercased() + else { + return .incompatible(.subtype) + } + + // A full concrete match, so also check parameters. + // The rule is: + // 1. If a received parameter is not found in the option, + // that's okay and gets ignored. + // 2. If an option parameter is not received, this is an + // incompatible content type match. + // This means we can just iterate over option parameters and + // check them against the received parameters, but we can + // ignore any received parameters that didn't appear in the + // option parameters. + + // According to RFC 2045: https://www.rfc-editor.org/rfc/rfc2045#section-5.1 + // "Type, subtype, and parameter names are case-insensitive." + // Inferred: Parameter values are be case-sensitive. + + let receivedNormalizedParameters = Dictionary( + uniqueKeysWithValues: receivedParameters.map { ($0.key.lowercased(), $0.value) } + ) + var matchedParameterCount = 0 + for optionParameter in option.parameters { + let normalizedParameterName = optionParameter.key.lowercased() + guard + let receivedValue = receivedNormalizedParameters[normalizedParameterName], + receivedValue == optionParameter.value + else { + return .incompatible(.parameter(name: normalizedParameterName)) + } + matchedParameterCount += 1 + } + return .typeAndSubtype(matchedParameterCount: matchedParameterCount) + } + } +} diff --git a/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift b/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift index 1123d1ff..cf2d21b8 100644 --- a/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift +++ b/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift @@ -29,45 +29,52 @@ extension Converter { return OpenAPIMIMEType(rawValue) } - /// Checks whether a concrete content type matches an expected content type. - /// - /// The concrete content type can contain parameters, such as `charset`, but - /// they are ignored in the equality comparison. - /// - /// The expected content type can contain wildcards, such as */* and text/*. + /// Chooses the most appropriate content type for the provided received + /// content type and a list of options. /// - Parameters: - /// - received: The concrete content type to validate against the other. - /// - expectedRaw: The expected content type, can contain wildcards. - /// - Throws: A `RuntimeError` when `expectedRaw` is not a valid content type. - /// - Returns: A Boolean value representing whether the concrete content - /// type matches the expected one. - public func isMatchingContentType(received: OpenAPIMIMEType?, expectedRaw: String) throws -> Bool { - guard let received else { - return false + /// - received: The received content type. + /// - options: The options to match against. + /// - Returns: The most appropriate option. + /// - Throws: If none of the options match the received content type. + /// - Precondition: `options` must not be empty. + public func bestContentType( + received: OpenAPIMIMEType?, + options: [String] + ) throws -> String { + precondition(!options.isEmpty, "bestContentType options must not be empty.") + guard + let received, + case let .concrete(type: receivedType, subtype: receivedSubtype) = received.kind + else { + // If none received or if we received a wildcard, use the first one. + // This behavior isn't well defined by the OpenAPI specification. + // Note: We treat a partial wildcard, like `image/*` as a full + // wildcard `*/*`, but that's okay because for a concrete received + // content type the behavior of a wildcard is not clearly defined + // either. + return options[0] } - guard case let .concrete(type: receivedType, subtype: receivedSubtype) = received.kind else { - return false + let evaluatedOptions = try options.map { stringOption in + guard let parsedOption = OpenAPIMIMEType(stringOption) else { + throw RuntimeError.invalidExpectedContentType(stringOption) + } + let match = OpenAPIMIMEType.evaluate( + receivedType: receivedType, + receivedSubtype: receivedSubtype, + receivedParameters: received.parameters, + against: parsedOption + ) + return (contentType: stringOption, match: match) } - guard let expectedContentType = OpenAPIMIMEType(expectedRaw) else { - throw RuntimeError.invalidExpectedContentType(expectedRaw) + let sortedOptions = evaluatedOptions.sorted { a, b in + a.match.score > b.match.score } - switch expectedContentType.kind { - case .any: - return true - case .anySubtype(let expectedType): - return receivedType.lowercased() == expectedType.lowercased() - case .concrete(let expectedType, let expectedSubtype): - return receivedType.lowercased() == expectedType.lowercased() - && receivedSubtype.lowercased() == expectedSubtype.lowercased() + let bestOption = sortedOptions[0] + let bestContentType = bestOption.contentType + if case .incompatible = bestOption.match { + throw RuntimeError.unexpectedContentTypeHeader(bestContentType) } - } - - /// Returns an error to be thrown when an unexpected content type is - /// received. - /// - Parameter contentType: The content type that was received. - /// - Returns: An error representing an unexpected content type. - public func makeUnexpectedContentTypeError(contentType: OpenAPIMIMEType?) -> any Error { - RuntimeError.unexpectedContentTypeHeader(contentType?.description ?? "") + return bestContentType } // MARK: - Converter helper methods diff --git a/Sources/OpenAPIRuntime/Deprecated/Deprecated.swift b/Sources/OpenAPIRuntime/Deprecated/Deprecated.swift index bf454112..c6d1d1af 100644 --- a/Sources/OpenAPIRuntime/Deprecated/Deprecated.swift +++ b/Sources/OpenAPIRuntime/Deprecated/Deprecated.swift @@ -98,3 +98,48 @@ extension ServerError { ) } } + +extension Converter { + /// Returns an error to be thrown when an unexpected content type is + /// received. + /// - Parameter contentType: The content type that was received. + /// - Returns: An error representing an unexpected content type. + @available(*, deprecated) + public func makeUnexpectedContentTypeError(contentType: OpenAPIMIMEType?) -> any Error { + RuntimeError.unexpectedContentTypeHeader(contentType?.description ?? "") + } + + /// Checks whether a concrete content type matches an expected content type. + /// + /// The concrete content type can contain parameters, such as `charset`, but + /// they are ignored in the equality comparison. + /// + /// The expected content type can contain wildcards, such as */* and text/*. + /// - Parameters: + /// - received: The concrete content type to validate against the other. + /// - expectedRaw: The expected content type, can contain wildcards. + /// - Throws: A `RuntimeError` when `expectedRaw` is not a valid content type. + /// - Returns: A Boolean value representing whether the concrete content + /// type matches the expected one. + @available(*, deprecated) + public func isMatchingContentType(received: OpenAPIMIMEType?, expectedRaw: String) throws -> Bool { + guard let received else { + return false + } + guard case let .concrete(type: receivedType, subtype: receivedSubtype) = received.kind else { + return false + } + guard let expectedContentType = OpenAPIMIMEType(expectedRaw) else { + throw RuntimeError.invalidExpectedContentType(expectedRaw) + } + switch expectedContentType.kind { + case .any: + return true + case .anySubtype(let expectedType): + return receivedType.lowercased() == expectedType.lowercased() + case .concrete(let expectedType, let expectedSubtype): + return receivedType.lowercased() == expectedType.lowercased() + && receivedSubtype.lowercased() == expectedSubtype.lowercased() + } + } +} diff --git a/Tests/OpenAPIRuntimeTests/Base/Test_OpenAPIMIMEType.swift b/Tests/OpenAPIRuntimeTests/Base/Test_OpenAPIMIMEType.swift index 5aacd455..3fbbf97d 100644 --- a/Tests/OpenAPIRuntimeTests/Base/Test_OpenAPIMIMEType.swift +++ b/Tests/OpenAPIRuntimeTests/Base/Test_OpenAPIMIMEType.swift @@ -12,10 +12,10 @@ // //===----------------------------------------------------------------------===// import XCTest -@_spi(Generated) import OpenAPIRuntime +@_spi(Generated) @testable import OpenAPIRuntime final class Test_OpenAPIMIMEType: Test_Runtime { - func test() throws { + func testParsing() throws { let cases: [(String, OpenAPIMIMEType?, String?)] = [ // Common @@ -87,4 +87,92 @@ final class Test_OpenAPIMIMEType: Test_Runtime { XCTAssertEqual(mime?.description, outputString) } } + + func testScore() throws { + let cases: [(OpenAPIMIMEType.Match, Int)] = [ + + (.incompatible(.type), 0), + (.incompatible(.subtype), 0), + (.incompatible(.parameter(name: "foo")), 0), + + (.wildcard, 1), + + (.subtypeWildcard, 2), + + (.typeAndSubtype(matchedParameterCount: 0), 3), + (.typeAndSubtype(matchedParameterCount: 2), 5), + ] + for (match, score) in cases { + XCTAssertEqual(match.score, score, "Mismatch for match: \(match)") + } + } + + func testEvaluate() throws { + func testCase( + receivedType: String, + receivedSubtype: String, + receivedParameters: [String: String], + against option: OpenAPIMIMEType, + expected expectedMatch: OpenAPIMIMEType.Match, + file: StaticString = #file, + line: UInt = #line + ) { + let result = OpenAPIMIMEType.evaluate( + receivedType: receivedType, + receivedSubtype: receivedSubtype, + receivedParameters: receivedParameters, + against: option + ) + XCTAssertEqual(result, expectedMatch, file: file, line: line) + } + + let jsonWith2Params = OpenAPIMIMEType("application/json; charset=utf-8; version=1")! + let jsonWith1Param = OpenAPIMIMEType("application/json; charset=utf-8")! + let json = OpenAPIMIMEType("application/json")! + let fullWildcard = OpenAPIMIMEType("*/*")! + let subtypeWildcard = OpenAPIMIMEType("application/*")! + + func testJSONWith2Params( + against option: OpenAPIMIMEType, + expected expectedMatch: OpenAPIMIMEType.Match, + file: StaticString = #file, + line: UInt = #line + ) { + testCase( + receivedType: "application", + receivedSubtype: "json", + receivedParameters: [ + "charset": "utf-8", + "version": "1", + ], + against: option, + expected: expectedMatch, + file: file, + line: line + ) + } + + // Actual test cases start here. + + testJSONWith2Params( + against: jsonWith2Params, + expected: .typeAndSubtype(matchedParameterCount: 2) + ) + testJSONWith2Params( + against: jsonWith1Param, + expected: .typeAndSubtype(matchedParameterCount: 1) + ) + testJSONWith2Params( + against: json, + expected: .typeAndSubtype(matchedParameterCount: 0) + ) + testJSONWith2Params( + against: subtypeWildcard, + expected: .subtypeWildcard + ) + testJSONWith2Params( + against: fullWildcard, + expected: .wildcard + ) + } } diff --git a/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift b/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift index cb5bd055..34532d8e 100644 --- a/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift +++ b/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift @@ -25,6 +25,7 @@ final class Test_CommonConverterExtensions: Test_Runtime { // MARK: Miscs + @available(*, deprecated) func testContentTypeMatching() throws { let cases: [(received: String, expected: String, isMatch: Bool)] = [ ("application/json", "application/json", true), @@ -54,6 +55,153 @@ final class Test_CommonConverterExtensions: Test_Runtime { } } + func testBestContentType() throws { + func testCase( + received: String?, + options: [String], + expected expectedChoice: String, + file: StaticString = #file, + line: UInt = #line + ) throws { + let choice = try converter.bestContentType( + received: received.map { .init($0)! }, + options: options + ) + XCTAssertEqual(choice, expectedChoice, file: file, line: line) + } + + try testCase( + received: nil, + options: [ + "application/json", + "*/*", + ], + expected: "application/json" + ) + try testCase( + received: "*/*", + options: [ + "application/json", + "*/*", + ], + expected: "application/json" + ) + try testCase( + received: "application/*", + options: [ + "application/json", + "*/*", + ], + expected: "application/json" + ) + XCTAssertThrowsError( + try testCase( + received: "application/json", + options: [ + "whoops" + ], + expected: "application/json" + ) + ) + XCTAssertThrowsError( + try testCase( + received: "application/json", + options: [ + "text/plain", + "image/*", + ], + expected: "application/json" + ) + ) + try testCase( + received: "application/json; charset=utf-8; version=1", + options: [ + "*/*", + "application/*", + "application/json", + "application/json; charset=utf-8", + "application/json; charset=utf-8; version=1", + ], + expected: "application/json; charset=utf-8; version=1" + ) + try testCase( + received: "application/json; version=1; CHARSET=utf-8", + options: [ + "*/*", + "application/*", + "application/json", + "application/json; charset=utf-8", + "application/json; charset=utf-8; version=1", + ], + expected: "application/json; charset=utf-8; version=1" + ) + try testCase( + received: "application/json", + options: [ + "application/json; charset=utf-8", + "application/json; charset=utf-8; version=1", + "*/*", + "application/*", + "application/json", + ], + expected: "application/json" + ) + try testCase( + received: "application/json; charset=utf-8", + options: [ + "application/json; charset=utf-8; version=1", + "*/*", + "application/*", + "application/json", + ], + expected: "application/json" + ) + try testCase( + received: "application/json; charset=utf-8; version=1", + options: [ + "*/*", + "application/*", + "application/json; charset=utf-8", + "application/json", + ], + expected: "application/json; charset=utf-8" + ) + try testCase( + received: "application/json; charset=utf-8; version=1", + options: [ + "*/*", + "application/*", + ], + expected: "application/*" + ) + try testCase( + received: "application/json; charset=utf-8; version=1", + options: [ + "*/*" + ], + expected: "*/*" + ) + + try testCase( + received: "image/png", + options: [ + "image/*", + "*/*", + ], + expected: "image/*" + ) + XCTAssertThrowsError( + try testCase( + received: "text/csv", + options: [ + "text/html", + "application/json", + ], + expected: "-" + ) + ) + } + // MARK: Converter helper methods // | common | set | header field | URI | both | setHeaderFieldAsURI | From 53ee458d49d0ac48eae59fd056c3fb87f64c807a Mon Sep 17 00:00:00 2001 From: Honza Dvorsky Date: Fri, 27 Oct 2023 18:44:44 +0200 Subject: [PATCH 2/3] Update OpenAPIMIMEType.swift Co-authored-by: Gustavo Cairo --- Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift b/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift index 21799410..95390a7f 100644 --- a/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift +++ b/Sources/OpenAPIRuntime/Base/OpenAPIMIMEType.swift @@ -279,7 +279,7 @@ extension OpenAPIMIMEType { // According to RFC 2045: https://www.rfc-editor.org/rfc/rfc2045#section-5.1 // "Type, subtype, and parameter names are case-insensitive." - // Inferred: Parameter values are be case-sensitive. + // Inferred: Parameter values are case-sensitive. let receivedNormalizedParameters = Dictionary( uniqueKeysWithValues: receivedParameters.map { ($0.key.lowercased(), $0.value) } From 5d30df169531203e30292f72963ccb8c558823a1 Mon Sep 17 00:00:00 2001 From: Honza Dvorsky Date: Mon, 30 Oct 2023 09:03:21 +0100 Subject: [PATCH 3/3] PR feedback --- Sources/OpenAPIRuntime/Conversion/Converter+Common.swift | 7 +++---- .../Conversion/Test_Converter+Common.swift | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift b/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift index cf2d21b8..a9c27f9c 100644 --- a/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift +++ b/Sources/OpenAPIRuntime/Conversion/Converter+Common.swift @@ -66,10 +66,9 @@ extension Converter { ) return (contentType: stringOption, match: match) } - let sortedOptions = evaluatedOptions.sorted { a, b in - a.match.score > b.match.score - } - let bestOption = sortedOptions[0] + let bestOption = evaluatedOptions.max { a, b in + a.match.score < b.match.score + }! // Safe, we only get here if the array is not empty. let bestContentType = bestOption.contentType if case .incompatible = bestOption.match { throw RuntimeError.unexpectedContentTypeHeader(bestContentType) diff --git a/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift b/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift index 34532d8e..4819e912 100644 --- a/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift +++ b/Tests/OpenAPIRuntimeTests/Conversion/Test_Converter+Common.swift @@ -100,7 +100,7 @@ final class Test_CommonConverterExtensions: Test_Runtime { options: [ "whoops" ], - expected: "application/json" + expected: "-" ) ) XCTAssertThrowsError( @@ -110,7 +110,7 @@ final class Test_CommonConverterExtensions: Test_Runtime { "text/plain", "image/*", ], - expected: "application/json" + expected: "-" ) ) try testCase(