diff --git a/Sources/OpenAPIKit/Path Item/DereferencedPathItem.swift b/Sources/OpenAPIKit/Path Item/DereferencedPathItem.swift index 8bddb1ab75..3fb0438479 100644 --- a/Sources/OpenAPIKit/Path Item/DereferencedPathItem.swift +++ b/Sources/OpenAPIKit/Path Item/DereferencedPathItem.swift @@ -167,66 +167,73 @@ extension OpenAPI.PathItem: ExternallyDereferenceable { let oldAdditionalOperations = additionalOperations - async let (newParameters, c1, m1) = oldParameters.externallyDereferenced(with: loader) -// async let (newServers, c2, m2) = oldServers.externallyDereferenced(with: loader) - async let (newGet, c3, m3) = oldGet.externallyDereferenced(with: loader) - async let (newPut, c4, m4) = oldPut.externallyDereferenced(with: loader) - async let (newPost, c5, m5) = oldPost.externallyDereferenced(with: loader) - async let (newDelete, c6, m6) = oldDelete.externallyDereferenced(with: loader) - async let (newOptions, c7, m7) = oldOptions.externallyDereferenced(with: loader) - async let (newHead, c8, m8) = oldHead.externallyDereferenced(with: loader) - async let (newPatch, c9, m9) = oldPatch.externallyDereferenced(with: loader) - async let (newTrace, c10, m10) = oldTrace.externallyDereferenced(with: loader) - async let (newQuery, c11, m11) = oldQuery.externallyDereferenced(with: loader) - - async let (newAdditionalOperations, c12, m12) = oldAdditionalOperations.externallyDereferenced(with: loader) - var pathItem = self - var newComponents = try await c1 - var newMessages = try await m1 - - // ideally we would async let all of the props above and then set them here, - // but for now since there seems to be some sort of compiler bug we will do - // newServers in an if let below - pathItem.parameters = try await newParameters - pathItem.get = try await newGet - pathItem.put = try await newPut - pathItem.post = try await newPost - pathItem.delete = try await newDelete - pathItem.options = try await newOptions - pathItem.head = try await newHead - pathItem.patch = try await newPatch - pathItem.trace = try await newTrace - pathItem.query = try await newQuery - pathItem.additionalOperations = try await newAdditionalOperations - - try await newComponents.merge(c3) - try await newComponents.merge(c4) - try await newComponents.merge(c5) - try await newComponents.merge(c6) - try await newComponents.merge(c7) - try await newComponents.merge(c8) - try await newComponents.merge(c9) - try await newComponents.merge(c10) - try await newComponents.merge(c11) - try await newComponents.merge(c12) - - try await newMessages += m3 - try await newMessages += m4 - try await newMessages += m5 - try await newMessages += m6 - try await newMessages += m7 - try await newMessages += m8 - try await newMessages += m9 - try await newMessages += m10 - try await newMessages += m11 - try await newMessages += m12 + + // Older Swift 6.0/6.1 Linux runtimes have been observed crashing when this + // dereference path fans out into a large batch of `async let` child tasks. + // The sequential form keeps behavior the same while staying compatible + // across the full test matrix. + let (newParameters, c1, m1) = try await oldParameters.externallyDereferenced(with: loader) + pathItem.parameters = newParameters + + var newComponents = c1 + var newMessages = m1 + + let (newGet, c3, m3) = try await oldGet.externallyDereferenced(with: loader) + pathItem.get = newGet + try newComponents.merge(c3) + newMessages += m3 + + let (newPut, c4, m4) = try await oldPut.externallyDereferenced(with: loader) + pathItem.put = newPut + try newComponents.merge(c4) + newMessages += m4 + + let (newPost, c5, m5) = try await oldPost.externallyDereferenced(with: loader) + pathItem.post = newPost + try newComponents.merge(c5) + newMessages += m5 + + let (newDelete, c6, m6) = try await oldDelete.externallyDereferenced(with: loader) + pathItem.delete = newDelete + try newComponents.merge(c6) + newMessages += m6 + + let (newOptions, c7, m7) = try await oldOptions.externallyDereferenced(with: loader) + pathItem.options = newOptions + try newComponents.merge(c7) + newMessages += m7 + + let (newHead, c8, m8) = try await oldHead.externallyDereferenced(with: loader) + pathItem.head = newHead + try newComponents.merge(c8) + newMessages += m8 + + let (newPatch, c9, m9) = try await oldPatch.externallyDereferenced(with: loader) + pathItem.patch = newPatch + try newComponents.merge(c9) + newMessages += m9 + + let (newTrace, c10, m10) = try await oldTrace.externallyDereferenced(with: loader) + pathItem.trace = newTrace + try newComponents.merge(c10) + newMessages += m10 + + let (newQuery, c11, m11) = try await oldQuery.externallyDereferenced(with: loader) + pathItem.query = newQuery + try newComponents.merge(c11) + newMessages += m11 + + let (newAdditionalOperations, c12, m12) = try await oldAdditionalOperations.externallyDereferenced(with: loader) + pathItem.additionalOperations = newAdditionalOperations + try newComponents.merge(c12) + newMessages += m12 if let oldServers { - async let (newServers, c2, m2) = oldServers.externallyDereferenced(with: loader) - pathItem.servers = try await newServers - try await newComponents.merge(c2) - try await newMessages += m2 + let (newServers, c2, m2) = try await oldServers.externallyDereferenced(with: loader) + pathItem.servers = newServers + try newComponents.merge(c2) + newMessages += m2 } return (pathItem, newComponents, newMessages) diff --git a/Sources/OpenAPIKit/Validator/Validation+Builtins.swift b/Sources/OpenAPIKit/Validator/Validation+Builtins.swift index 327f8991ac..8b3567d095 100644 --- a/Sources/OpenAPIKit/Validator/Validation+Builtins.swift +++ b/Sources/OpenAPIKit/Validator/Validation+Builtins.swift @@ -377,6 +377,97 @@ extension Validation { ) } + /// Validate that `querystring` parameters are unique and do not coexist + /// with `query` parameters within a Path Item's effective operation + /// parameters. + /// + /// OpenAPI 3.2.0 requires that a `querystring` parameter + /// [must not appear more than once and must not appear in the same operation + /// as any `query` parameters](https://spec.openapis.org/oas/v3.2.0.html#parameter-locations). + /// + /// - Important: This is included in validation by default. + public static var querystringParametersAreCompatible: Validation { + .init( + description: "Querystring parameters are unique and do not coexist with query parameters", + check: { context in + let pathParameters = resolvedParameters(context.subject.parameters, components: context.document.components) + let pathSummary = parameterLocationSummary(pathParameters) + let pathParametersPath = context.codingPath + [Validator.CodingKey.init(stringValue: "parameters")] + var errors = [ValidationError]() + + if pathSummary.querystringCount > 1 { + errors.append( + ValidationError( + reason: "Path Item parameters must not contain more than one `querystring` parameter", + at: pathParametersPath + ) + ) + } + + if pathSummary.querystringCount > 0 && pathSummary.queryCount > 0 { + errors.append( + ValidationError( + reason: "Path Item parameters must not mix `querystring` and `query` parameter locations", + at: pathParametersPath + ) + ) + } + + for endpoint in context.subject.endpoints { + let operationParameters = resolvedParameters(endpoint.operation.parameters, components: context.document.components) + let operationSummary = parameterLocationSummary(operationParameters) + let operationParametersPath = context.codingPath + [ + Validator.CodingKey.init(stringValue: codingPathKey(for: endpoint.method)), + Validator.CodingKey.init(stringValue: "parameters") + ] + + if operationSummary.querystringCount > 1 { + errors.append( + ValidationError( + reason: "Operation parameters must not contain more than one `querystring` parameter", + at: operationParametersPath + ) + ) + } + + if operationSummary.querystringCount > 0 && operationSummary.queryCount > 0 { + errors.append( + ValidationError( + reason: "Operation parameters must not mix `querystring` and `query` parameter locations", + at: operationParametersPath + ) + ) + } + + if pathSummary.querystringCount <= 1 && + operationSummary.querystringCount <= 1 && + pathSummary.querystringCount + operationSummary.querystringCount > 1 { + errors.append( + ValidationError( + reason: "Operation parameters must not contain more than one `querystring` parameter, including inherited Path Item parameters", + at: operationParametersPath + ) + ) + } + + if !(pathSummary.querystringCount > 0 && pathSummary.queryCount > 0) && + !(operationSummary.querystringCount > 0 && operationSummary.queryCount > 0) && + pathSummary.querystringCount + operationSummary.querystringCount > 0 && + pathSummary.queryCount + operationSummary.queryCount > 0 { + errors.append( + ValidationError( + reason: "Operation parameters must not mix `querystring` and `query` parameter locations, including inherited Path Item parameters", + at: operationParametersPath + ) + ) + } + } + + return errors + } + ) + } + /// Validate that all OpenAPI Operation Ids are unique across the whole Document. /// /// The OpenAPI Specification requires that Operation Ids [are unique](https://spec.openapis.org/oas/v3.2.0.html#operation-object). @@ -575,9 +666,34 @@ extension Validation { /// Used by both the Path Item parameter check and the /// Operation parameter check in the default validations. fileprivate func parametersAreUnique(_ parameters: OpenAPI.Parameter.Array, components: OpenAPI.Components) -> Bool { - let foundParameters = parameters.compactMap { try? components.lookup($0) } + let foundParameters = resolvedParameters(parameters, components: components) let identities = foundParameters.map { OpenAPI.Parameter.ParameterIdentity(name: $0.name, location: $0.location) } return Set(identities).count == foundParameters.count } + +fileprivate func resolvedParameters(_ parameters: OpenAPI.Parameter.Array, components: OpenAPI.Components) -> [OpenAPI.Parameter] { + parameters.compactMap { try? components.lookup($0) } +} + +fileprivate struct ParameterLocationSummary { + let queryCount: Int + let querystringCount: Int +} + +fileprivate func parameterLocationSummary(_ parameters: [OpenAPI.Parameter]) -> ParameterLocationSummary { + .init( + queryCount: parameters.filter { $0.location == .query }.count, + querystringCount: parameters.filter { $0.location == .querystring }.count + ) +} + +fileprivate func codingPathKey(for method: OpenAPI.HttpMethod) -> String { + switch method { + case .builtin(let builtin): + return builtin.rawValue.lowercased() + case .other(let other): + return other + } +} diff --git a/Sources/OpenAPIKit/Validator/Validator.swift b/Sources/OpenAPIKit/Validator/Validator.swift index 4b8c5fadea..73b4df3bd4 100644 --- a/Sources/OpenAPIKit/Validator/Validator.swift +++ b/Sources/OpenAPIKit/Validator/Validator.swift @@ -77,6 +77,7 @@ extension OpenAPI.Document { /// - Document-level tag names are unique. /// - Parameters are unique within each Path Item. /// - Parameters are unique within each Operation. +/// - Querystring parameters are unique and do not coexist with query parameters. /// - Operation Ids are unique across the whole Document. /// - All OpenAPI.References that refer to components in this /// document can be found in the components dictionary. @@ -158,6 +159,7 @@ public final class Validator { .init(.documentTagNamesAreUnique), .init(.pathItemParametersAreUnique), .init(.operationParametersAreUnique), + .init(.querystringParametersAreCompatible), .init(.operationIdsAreUnique), .init(.serverVariableEnumIsValid), .init(.serverVariableDefaultExistsInEnum), @@ -202,6 +204,7 @@ public final class Validator { /// - Document-level tag names are unique. /// - Parameters are unique within each Path Item. /// - Parameters are unique within each Operation. + /// - Querystring parameters are unique and do not coexist with query parameters. /// - Operation Ids are unique across the whole Document. /// - All OpenAPI.References that refer to components in this document can /// be found in the components dictionary. diff --git a/Sources/OpenAPIKit30/Path Item/DereferencedPathItem.swift b/Sources/OpenAPIKit30/Path Item/DereferencedPathItem.swift index ae9d6e3be4..fdd27f1b63 100644 --- a/Sources/OpenAPIKit30/Path Item/DereferencedPathItem.swift +++ b/Sources/OpenAPIKit30/Path Item/DereferencedPathItem.swift @@ -153,57 +153,63 @@ extension OpenAPI.PathItem: ExternallyDereferenceable { let oldPatch = patch let oldTrace = trace - async let (newParameters, c1, m1) = oldParameters.externallyDereferenced(with: loader) -// async let (newServers, c2, m2) = oldServers.externallyDereferenced(with: loader) - async let (newGet, c3, m3) = oldGet.externallyDereferenced(with: loader) - async let (newPut, c4, m4) = oldPut.externallyDereferenced(with: loader) - async let (newPost, c5, m5) = oldPost.externallyDereferenced(with: loader) - async let (newDelete, c6, m6) = oldDelete.externallyDereferenced(with: loader) - async let (newOptions, c7, m7) = oldOptions.externallyDereferenced(with: loader) - async let (newHead, c8, m8) = oldHead.externallyDereferenced(with: loader) - async let (newPatch, c9, m9) = oldPatch.externallyDereferenced(with: loader) - async let (newTrace, c10, m10) = oldTrace.externallyDereferenced(with: loader) - var pathItem = self - var newComponents = try await c1 - var newMessages = try await m1 - - // ideally we would async let all of the props above and then set them here, - // but for now since there seems to be some sort of compiler bug we will do - // newServers in an if let below - pathItem.parameters = try await newParameters - pathItem.get = try await newGet - pathItem.put = try await newPut - pathItem.post = try await newPost - pathItem.delete = try await newDelete - pathItem.options = try await newOptions - pathItem.head = try await newHead - pathItem.patch = try await newPatch - pathItem.trace = try await newTrace - - try await newComponents.merge(c3) - try await newComponents.merge(c4) - try await newComponents.merge(c5) - try await newComponents.merge(c6) - try await newComponents.merge(c7) - try await newComponents.merge(c8) - try await newComponents.merge(c9) - try await newComponents.merge(c10) - - try await newMessages += m3 - try await newMessages += m4 - try await newMessages += m5 - try await newMessages += m6 - try await newMessages += m7 - try await newMessages += m8 - try await newMessages += m9 - try await newMessages += m10 + + // Older Swift 6.0/6.1 Linux runtimes have been observed crashing when this + // dereference path fans out into a large batch of `async let` child tasks. + // The sequential form keeps behavior the same while staying compatible + // across the full test matrix. + let (newParameters, c1, m1) = try await oldParameters.externallyDereferenced(with: loader) + pathItem.parameters = newParameters + + var newComponents = c1 + var newMessages = m1 + + let (newGet, c3, m3) = try await oldGet.externallyDereferenced(with: loader) + pathItem.get = newGet + try newComponents.merge(c3) + newMessages += m3 + + let (newPut, c4, m4) = try await oldPut.externallyDereferenced(with: loader) + pathItem.put = newPut + try newComponents.merge(c4) + newMessages += m4 + + let (newPost, c5, m5) = try await oldPost.externallyDereferenced(with: loader) + pathItem.post = newPost + try newComponents.merge(c5) + newMessages += m5 + + let (newDelete, c6, m6) = try await oldDelete.externallyDereferenced(with: loader) + pathItem.delete = newDelete + try newComponents.merge(c6) + newMessages += m6 + + let (newOptions, c7, m7) = try await oldOptions.externallyDereferenced(with: loader) + pathItem.options = newOptions + try newComponents.merge(c7) + newMessages += m7 + + let (newHead, c8, m8) = try await oldHead.externallyDereferenced(with: loader) + pathItem.head = newHead + try newComponents.merge(c8) + newMessages += m8 + + let (newPatch, c9, m9) = try await oldPatch.externallyDereferenced(with: loader) + pathItem.patch = newPatch + try newComponents.merge(c9) + newMessages += m9 + + let (newTrace, c10, m10) = try await oldTrace.externallyDereferenced(with: loader) + pathItem.trace = newTrace + try newComponents.merge(c10) + newMessages += m10 if let oldServers { - async let (newServers, c2, m2) = oldServers.externallyDereferenced(with: loader) - pathItem.servers = try await newServers - try await newComponents.merge(c2) - try await newMessages += m2 + let (newServers, c2, m2) = try await oldServers.externallyDereferenced(with: loader) + pathItem.servers = newServers + try newComponents.merge(c2) + newMessages += m2 } return (pathItem, newComponents, newMessages) diff --git a/Tests/OpenAPIKitTests/Validator/BuiltinValidationTests.swift b/Tests/OpenAPIKitTests/Validator/BuiltinValidationTests.swift index f8ffa15b14..8e0c37b7de 100644 --- a/Tests/OpenAPIKitTests/Validator/BuiltinValidationTests.swift +++ b/Tests/OpenAPIKitTests/Validator/BuiltinValidationTests.swift @@ -24,11 +24,12 @@ final class BuiltinValidationTests: XCTestCase { ]) let withoutReferenceValidations = Validator().skippingReferenceValidations() - XCTAssertEqual(withoutReferenceValidations.validationDescriptions.count, 7) + XCTAssertEqual(withoutReferenceValidations.validationDescriptions.count, 8) XCTAssertEqual(withoutReferenceValidations.validationDescriptions, [ "The names of Tags in the Document are unique", "Path Item parameters are unique (identity is defined by the \'name\' and \'location\')", "Operation parameters are unique (identity is defined by the \'name\' and \'location\')", + "Querystring parameters are unique and do not coexist with query parameters", "All Operation Ids in Document are unique", "Server Variable\'s enum is either not defined or is non-empty (if defined).", "Server Variable\'s default must exist in enum, if enum is defined.", @@ -36,11 +37,12 @@ final class BuiltinValidationTests: XCTestCase { ]) let defaultValidations = Validator() - XCTAssertEqual(defaultValidations.validationDescriptions.count, 17) + XCTAssertEqual(defaultValidations.validationDescriptions.count, 18) XCTAssertEqual(defaultValidations.validationDescriptions, [ "The names of Tags in the Document are unique", "Path Item parameters are unique (identity is defined by the \'name\' and \'location\')", "Operation parameters are unique (identity is defined by the \'name\' and \'location\')", + "Querystring parameters are unique and do not coexist with query parameters", "All Operation Ids in Document are unique", "Server Variable\'s enum is either not defined or is non-empty (if defined).", "Server Variable\'s default must exist in enum, if enum is defined.", @@ -58,11 +60,12 @@ final class BuiltinValidationTests: XCTestCase { ]) let stricterReferenceValidations = Validator().validatingAllReferencesFoundInComponents() - XCTAssertEqual(stricterReferenceValidations.validationDescriptions.count, 17) + XCTAssertEqual(stricterReferenceValidations.validationDescriptions.count, 18) XCTAssertEqual(stricterReferenceValidations.validationDescriptions, [ "The names of Tags in the Document are unique", "Path Item parameters are unique (identity is defined by the \'name\' and \'location\')", "Operation parameters are unique (identity is defined by the \'name\' and \'location\')", + "Querystring parameters are unique and do not coexist with query parameters", "All Operation Ids in Document are unique", "Server Variable\'s enum is either not defined or is non-empty (if defined).", "Server Variable\'s default must exist in enum, if enum is defined.", @@ -1411,4 +1414,135 @@ final class BuiltinValidationTests: XCTestCase { XCTAssertEqual(errorCollection?.values.first?.codingPath.stringValue, ".paths[\'/hello\'].get.parameters[0]") } } + + func test_duplicateQuerystringParametersOnPathItem_fails() throws { + let document = OpenAPI.Document( + info: .init(title: "test", version: "1.0"), + servers: [], + paths: [ + "/hello": .init( + parameters: [ + .parameter(OpenAPI.Parameter.querystring(name: "first", content: [:])), + .parameter(OpenAPI.Parameter.querystring(name: "second", content: [:])) + ], + get: .init(responses: [:]) + ) + ], + components: .noComponents + ) + + let validator = Validator.blank.validating(.querystringParametersAreCompatible) + + XCTAssertThrowsError(try document.validate(using: validator)) { error in + let errorCollection = error as? ValidationErrorCollection + XCTAssertEqual(errorCollection?.values.first?.reason, "Path Item parameters must not contain more than one `querystring` parameter") + XCTAssertEqual(errorCollection?.values.first?.codingPath.stringValue, ".paths[\'/hello\'].parameters") + } + } + + func test_duplicateQuerystringParametersAcrossPathItemAndOperation_fails() throws { + let document = OpenAPI.Document( + info: .init(title: "test", version: "1.0"), + servers: [], + paths: [ + "/hello": .init( + parameters: [ + .parameter(OpenAPI.Parameter.querystring(name: "first", content: [:])) + ], + get: .init( + parameters: [ + .parameter(OpenAPI.Parameter.querystring(name: "second", content: [:])) + ], + responses: [:] + ) + ) + ], + components: .noComponents + ) + + let validator = Validator.blank.validating(.querystringParametersAreCompatible) + + XCTAssertThrowsError(try document.validate(using: validator)) { error in + let errorCollection = error as? ValidationErrorCollection + XCTAssertEqual(errorCollection?.values.first?.reason, "Operation parameters must not contain more than one `querystring` parameter, including inherited Path Item parameters") + XCTAssertEqual(errorCollection?.values.first?.codingPath.stringValue, ".paths[\'/hello\'].get.parameters") + } + } + + func test_querystringAndQueryParametersOnOperation_fails() throws { + let document = OpenAPI.Document( + info: .init(title: "test", version: "1.0"), + servers: [], + paths: [ + "/hello": .init( + get: .init( + parameters: [ + .parameter(OpenAPI.Parameter.query(name: "query", schema: .string)), + .parameter(OpenAPI.Parameter.querystring(name: "querystring", content: [:])) + ], + responses: [:] + ) + ) + ], + components: .noComponents + ) + + let validator = Validator.blank.validating(.querystringParametersAreCompatible) + + XCTAssertThrowsError(try document.validate(using: validator)) { error in + let errorCollection = error as? ValidationErrorCollection + XCTAssertEqual(errorCollection?.values.first?.reason, "Operation parameters must not mix `querystring` and `query` parameter locations") + XCTAssertEqual(errorCollection?.values.first?.codingPath.stringValue, ".paths[\'/hello\'].get.parameters") + } + } + + func test_querystringAndQueryParametersAcrossPathItemAndOperation_fails() throws { + let document = OpenAPI.Document( + info: .init(title: "test", version: "1.0"), + servers: [], + paths: [ + "/hello": .init( + parameters: [ + .parameter(OpenAPI.Parameter.query(name: "query", schema: .string)) + ], + get: .init( + parameters: [ + .parameter(OpenAPI.Parameter.querystring(name: "querystring", content: [:])) + ], + responses: [:] + ) + ) + ], + components: .noComponents + ) + + let validator = Validator.blank.validating(.querystringParametersAreCompatible) + + XCTAssertThrowsError(try document.validate(using: validator)) { error in + let errorCollection = error as? ValidationErrorCollection + XCTAssertEqual(errorCollection?.values.first?.reason, "Operation parameters must not mix `querystring` and `query` parameter locations, including inherited Path Item parameters") + XCTAssertEqual(errorCollection?.values.first?.codingPath.stringValue, ".paths[\'/hello\'].get.parameters") + } + } + + func test_singleQuerystringParameter_succeeds() throws { + let document = OpenAPI.Document( + info: .init(title: "test", version: "1.0"), + servers: [], + paths: [ + "/hello": .init( + get: .init( + parameters: [ + .parameter(OpenAPI.Parameter.querystring(name: "querystring", content: [:])) + ], + responses: [:] + ) + ) + ], + components: .noComponents + ) + + let validator = Validator.blank.validating(.querystringParametersAreCompatible) + try document.validate(using: validator) + } }