Skip to content

[Collections] Improve error handling #3471

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 5, 2021
Merged

[Collections] Improve error handling #3471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 36 additions & 32 deletions Sources/PackageCollections/Providers/JSONPackageCollectionProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
}

if let errors = source.validate()?.errors() {
return callback(.failure(MultipleErrors(errors)))
return callback(.failure(JSONPackageCollectionProviderError.invalidSource("\(errors)")))
}

// Source is a local file
Expand All @@ -89,18 +89,18 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
switch result {
case .failure(HTTPClientError.badResponseStatusCode(let statusCode)):
if statusCode == 404 {
return callback(.failure(Errors.collectionNotFound(source.url)))
return callback(.failure(JSONPackageCollectionProviderError.collectionNotFound(source.url)))
} else {
return callback(.failure(Errors.collectionUnavailable(source.url, statusCode)))
return callback(.failure(JSONPackageCollectionProviderError.collectionUnavailable(source.url, statusCode)))
}
case .failure(let error):
return callback(.failure(error))
case .success(let response):
guard let contentLength = response.headers.get("Content-Length").first.flatMap(Int64.init) else {
return callback(.failure(Errors.invalidResponse(source.url, "Missing Content-Length header")))
return callback(.failure(JSONPackageCollectionProviderError.invalidResponse(source.url, "Missing Content-Length header")))
}
guard contentLength <= self.configuration.maximumSizeInBytes else {
return callback(.failure(Errors.responseTooLarge(source.url, contentLength)))
return callback(.failure(JSONPackageCollectionProviderError.responseTooLarge(source.url, contentLength)))
}
// next do a get request to get the actual content
var getOptions = self.makeRequestOptions(validResponseCodes: [200])
Expand All @@ -109,9 +109,9 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
switch result {
case .failure(HTTPClientError.badResponseStatusCode(let statusCode)):
if statusCode == 404 {
return callback(.failure(Errors.collectionNotFound(source.url)))
return callback(.failure(JSONPackageCollectionProviderError.collectionNotFound(source.url)))
} else {
return callback(.failure(Errors.collectionUnavailable(source.url, statusCode)))
return callback(.failure(JSONPackageCollectionProviderError.collectionUnavailable(source.url, statusCode)))
}
case .failure(let error):
return callback(.failure(error))
Expand All @@ -120,13 +120,13 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
// if not returning head and exceeding size
// TODO: store bad actors to prevent server DoS
guard let contentLength = response.headers.get("Content-Length").first.flatMap(Int64.init) else {
return callback(.failure(Errors.invalidResponse(source.url, "Missing Content-Length header")))
return callback(.failure(JSONPackageCollectionProviderError.invalidResponse(source.url, "Missing Content-Length header")))
}
guard contentLength < self.configuration.maximumSizeInBytes else {
return callback(.failure(Errors.responseTooLarge(source.url, contentLength)))
return callback(.failure(JSONPackageCollectionProviderError.responseTooLarge(source.url, contentLength)))
}
guard let body = response.body else {
return callback(.failure(Errors.invalidResponse(source.url, "Body is empty")))
return callback(.failure(JSONPackageCollectionProviderError.invalidResponse(source.url, "Body is empty")))
}

let certPolicyKey = self.sourceCertPolicy.certificatePolicyKey(for: source) ?? .default
Expand Down Expand Up @@ -175,15 +175,15 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
}
// Collection is unsigned
guard let collection = try? self.decoder.decode(JSONModel.Collection.self, from: data) else {
return callback(.failure(Errors.invalidJSON(source.url)))
return callback(.failure(JSONPackageCollectionProviderError.invalidJSON(source.url)))
}
callback(self.makeCollection(from: collection, source: source, signature: nil))
}
}

private func makeCollection(from collection: JSONModel.Collection, source: Model.CollectionSource, signature: Model.SignatureData?) -> Result<Model.Collection, Error> {
if let errors = self.validator.validate(collection: collection)?.errors() {
return .failure(MultipleErrors(errors))
return .failure(JSONPackageCollectionProviderError.invalidCollection("\(errors)"))
}

var serializationOkay = true
Expand Down Expand Up @@ -345,27 +345,31 @@ struct JSONPackageCollectionProvider: PackageCollectionProvider {
)
}
}
}

public enum Errors: Error, Equatable, CustomStringConvertible {
case invalidJSON(URL)
case invalidResponse(URL, String)
case responseTooLarge(URL, Int64)
case collectionNotFound(URL)
case collectionUnavailable(URL, Int)

public var description: String {
switch self {
case .invalidJSON(let url):
return "The package collection at \(url.absoluteString) contains invalid JSON."
case .invalidResponse(let url, let message):
return "Received invalid response for package collection at \(url.absoluteString): \(message)"
case .responseTooLarge(let url, _):
return "The package collection at \(url.absoluteString) is too large."
case .collectionNotFound(let url):
return "No package collection found at \(url.absoluteString). Please make sure the URL is correct."
case .collectionUnavailable(let url, _):
return "The package collection at \(url.absoluteString) is unavailable. Please make sure the URL is correct or try again later."
}
public enum JSONPackageCollectionProviderError: Error, Equatable, CustomStringConvertible {
case invalidSource(String)
case invalidJSON(URL)
case invalidCollection(String)
case invalidResponse(URL, String)
case responseTooLarge(URL, Int64)
case collectionNotFound(URL)
case collectionUnavailable(URL, Int)

public var description: String {
switch self {
case .invalidSource(let errorMessage), .invalidCollection(let errorMessage):
return errorMessage
case .invalidJSON(let url):
return "The package collection at \(url.absoluteString) contains invalid JSON."
case .invalidResponse(let url, let message):
return "Received invalid response for package collection at \(url.absoluteString): \(message)"
case .responseTooLarge(let url, _):
return "The package collection at \(url.absoluteString) is too large."
case .collectionNotFound(let url):
return "No package collection found at \(url.absoluteString). Please make sure the URL is correct."
case .collectionUnavailable(let url, _):
return "The package collection at \(url.absoluteString) is unavailable. Please make sure the URL is correct or try again later."
}
}
}
Expand Down
23 changes: 10 additions & 13 deletions Tests/PackageCollectionsTests/JSONPackageCollectionProviderTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,10 @@ class JSONPackageCollectionProviderTests: XCTestCase {
httpClient.configuration.retryStrategy = .none
let provider = JSONPackageCollectionProvider(httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
guard let internalError = (error as? MultipleErrors)?.errors.first else {
guard case .invalidSource(let errorMessage) = error as? JSONPackageCollectionProviderError else {
return XCTFail("invalid error \(error)")
}
guard let validationError = internalError as? ValidationError, case .other(let message) = validationError else {
return XCTFail("invalid error \(error)")
}
XCTAssertTrue(message.contains("Scheme (\"ftp\") not allowed: \(url.absoluteString)"))
XCTAssertTrue(errorMessage.contains("Scheme (\"ftp\") not allowed: \(url.absoluteString)"))
})
}

Expand All @@ -152,7 +149,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
let configuration = JSONPackageCollectionProvider.Configuration(maximumSizeInBytes: 10)
let provider = JSONPackageCollectionProvider(configuration: configuration, httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .responseTooLarge(url, maxSize * 2))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .responseTooLarge(url, maxSize * 2))
})
}

Expand Down Expand Up @@ -181,7 +178,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
let configuration = JSONPackageCollectionProvider.Configuration(maximumSizeInBytes: 10)
let provider = JSONPackageCollectionProvider(configuration: configuration, httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .responseTooLarge(url, maxSize * 2))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .responseTooLarge(url, maxSize * 2))
})
}

Expand All @@ -201,7 +198,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
let configuration = JSONPackageCollectionProvider.Configuration(maximumSizeInBytes: 10)
let provider = JSONPackageCollectionProvider(configuration: configuration, httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .invalidResponse(url, "Missing Content-Length header"))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .invalidResponse(url, "Missing Content-Length header"))
})
}

Expand Down Expand Up @@ -249,7 +246,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
httpClient.configuration.retryStrategy = .none
let provider = JSONPackageCollectionProvider(httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .collectionUnavailable(url, statusCode))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .collectionUnavailable(url, statusCode))
})
}

Expand All @@ -275,7 +272,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
httpClient.configuration.retryStrategy = .none
let provider = JSONPackageCollectionProvider(httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .collectionUnavailable(url, statusCode))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .collectionUnavailable(url, statusCode))
})
}

Expand All @@ -294,7 +291,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
httpClient.configuration.retryStrategy = .none
let provider = JSONPackageCollectionProvider(httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .collectionNotFound(url))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .collectionNotFound(url))
})
}

Expand All @@ -319,7 +316,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
httpClient.configuration.retryStrategy = .none
let provider = JSONPackageCollectionProvider(httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .collectionNotFound(url))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .collectionNotFound(url))
})
}

Expand Down Expand Up @@ -347,7 +344,7 @@ class JSONPackageCollectionProviderTests: XCTestCase {
let provider = JSONPackageCollectionProvider(httpClient: httpClient, diagnosticsEngine: DiagnosticsEngine())
let source = PackageCollectionsModel.CollectionSource(type: .json, url: url)
XCTAssertThrowsError(try tsc_await { callback in provider.get(source, callback: callback) }, "expected error", { error in
XCTAssertEqual(error as? JSONPackageCollectionProvider.Errors, .invalidJSON(url))
XCTAssertEqual(error as? JSONPackageCollectionProviderError, .invalidJSON(url))
})
}

Expand Down