Skip to content
This repository has been archived by the owner on Apr 29, 2019. It is now read-only.

Improve Error Messages and Recovery Suggestions #44

Merged
2 commits merged into from
Feb 20, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
18 changes: 17 additions & 1 deletion Sources/GitHubKit/Extension/HTTPURLResponse+Additions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// GitHub Scanner
//
// Created by Aaron McTavish on 10/02/2017.
// Copyright © 2016 ustwo Fampany Ltd. All rights reserved.
// Copyright © 2017 ustwo Fampany Ltd. All rights reserved.
//

import Foundation
Expand All @@ -14,6 +14,17 @@ import Foundation
extension HTTPURLResponse {


// MARK: - Types

private struct Constants {
static let nextLinkHeader = "next"
static let urlHeaderValue = "url"
}


// MARK: - Properties

/// Parses the link headers from the `HTTPURLResponse` and returns a dictionary.
public var links: [String: [String: String]] {
var result = [String: [String: String]]()

Expand Down Expand Up @@ -57,4 +68,9 @@ extension HTTPURLResponse {
return result
}

/// Returns the 'next' link header, if it exists.
public var nextLink: String? {
return links[Constants.nextLinkHeader]?[Constants.urlHeaderValue]
}

}
9 changes: 1 addition & 8 deletions Sources/GitHubKit/Networking/NetworkClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
// GitHub Scanner
//
// Created by Aaron McTavish on 09/02/2017.
// Copyright © 2016 ustwo Fampany Ltd. All rights reserved.
// Copyright © 2017 ustwo Fampany Ltd. All rights reserved.
//


Expand All @@ -12,13 +12,6 @@
import Foundation


public enum NetworkError: Error {
case failedRequest(status: Int)
case invalidJSON
case unknown(error: Error?)
}


public final class NetworkClient {


Expand Down
55 changes: 55 additions & 0 deletions Sources/GitHubKit/Networking/NetworkError.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
//
// NetworkError.swift
// GitHub Scanner
//
// Created by Aaron McTavish on 20/02/2017.
// Copyright © 2017 ustwo Fampany Ltd. All rights reserved.
//


import Foundation


public enum NetworkError: LocalizedError {
case failedRequest(status: Int)
case invalidJSON
case rateLimited
case unauthorized
case unknown(error: Error?)
}


extension NetworkError {


public var errorDescription: String? {
switch self {
case let .failedRequest(status):
return "Failed Request. Status Code: \(status)"
case .invalidJSON:
return "Invalid JSON returned from the server"
case .rateLimited:
return "Exceed rate limit for requests"
case .unauthorized:
return "Not authorized"
case let .unknown(error):
if let localError = error as? LocalizedError,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The if indentation looks a bit off.

let description = localError.errorDescription {

return "Unknown Error: " + description
} else {
return "Unknown Error"
}
}
}

public var recoverySuggestion: String? {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

beautiful

switch self {
case .rateLimited, .unauthorized:
return "Use the '--oauth' flag and supply an access token"
case .failedRequest, .invalidJSON, .unknown:
return nil
}
}

}
82 changes: 79 additions & 3 deletions Sources/GitHubKit/Networking/ResponseHandler.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,22 @@
// GitHub Scanner
//
// Created by Aaron McTavish on 10/02/2017.
// Copyright © 2016 ustwo Fampany Ltd. All rights reserved.
// Copyright © 2017 ustwo Fampany Ltd. All rights reserved.
//


// swiftlint:disable large_tuple

import Foundation
import Result


public typealias NetworkValidationResult = Result<(Data, HTTPURLResponse), NetworkError>


public enum ResponseHandlers {
static let `default` = JSONResponseHandler()
}


public protocol ResponseHandler {
Expand All @@ -24,6 +33,73 @@ public protocol ResponseHandler {
}


public enum ResponseHandlers {
static let `default` = JSONResponseHandler()
extension ResponseHandler {


/// Checks the response's status code to ensure it is in the 200 range.
///
/// - Parameter response: The `HTTPURLResponse` to validate.
/// - Returns: Whether or not the response is valid.
public static func isValidResponseStatus(_ response: HTTPURLResponse) -> Bool {
return 200..<300 ~= response.statusCode
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 3xx range is valid as well. Perhaps in a future iteration you should handle at least a few of them (i.e. 301, 302 and 307) https://developer.github.com/v3/#http-redirects

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Issue #45 Handle HTTP Redirects.

}

/// Checks the response to see if it failed due to rate limiting.
///
/// - Parameter response: The `HTTPURLResponse` to validate.
/// - Returns: Whether or not the response has been rate limited.
public static func isRateLimitedResponse(_ response: HTTPURLResponse) -> Bool {
if [401, 403].contains(response.statusCode),
let rateLimitString = response.allHeaderFields["X-RateLimit-Remaining"] as? String,
let rateLimit = Int(rateLimitString),
rateLimit == 0 {

return true
}

return false
}

/// Checks the response to see if it failed due to not having sufficient authorization on the request.
///
/// - Parameter response: The `HTTPURLResponse` to validate.
/// - Returns: Whether or not the response failed due to being unauthorized.
///
/// - Note: Should check `ResponseHandler.isRateLimitedResponse(_:)` before calling this method,
/// as being rate limited could also return a 401 status code.
public static func isUnauthorized(_ response: HTTPURLResponse) -> Bool {
return response.statusCode == 401
}

/// Validates a network response.
///
/// - Parameters:
/// - data: `Data` from the body of the response.
/// - response: `URLResponse` to validate.
/// - error: `Error` from making the network request.
/// - Returns: If the validation is successful, returns the `data` unwrapped and the response
/// as an `HTTPURLResponse`. If the validation is unsuccessful, returns the `NetworkError`
/// representing the issue.
///
/// - Note: Checks for a valid status code, rate limiting error, and authorization.
public static func validateResponse(data: Data?, response: URLResponse?, error: Error?) -> NetworkValidationResult {
guard let httpResponse = response as? HTTPURLResponse,
let data = data else {

return .failure(NetworkError.unknown(error: error))
}

guard Self.isValidResponseStatus(httpResponse) else {
if Self.isRateLimitedResponse(httpResponse) {
return .failure(NetworkError.rateLimited)
} else if Self.isUnauthorized(httpResponse) {
return .failure(NetworkError.unauthorized)
}

return .failure(NetworkError.failedRequest(status: httpResponse.statusCode))
}

return .success((data, httpResponse))
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -11,40 +11,57 @@


import Foundation
import Result


public final class JSONResponseHandler: ResponseHandler {


// MARK: - ResponseHandler

public func process<Output: JSONInitializable>(data: Data?,
response: URLResponse?,
error: Error?,
completion: ((_ result: Output?,
_ linkHeader: String?,
_ error: NetworkError?) -> Void)?) {

guard let httpResponse = response as? HTTPURLResponse,
let data = data else {
let validationResult = JSONResponseHandler.validateResponse(data: data,
response: response,
error: error)

completion?(nil, nil, NetworkError.unknown(error: error))
switch validationResult {
case let .success(body, httpResponse):
let deserializationResult: Result<Output, NetworkError> = deserializeJSON(data: body)

switch deserializationResult {
case let .success(result):
completion?(result, httpResponse.nextLink, nil)
return
}

guard 200..<300 ~= httpResponse.statusCode else {
completion?(nil, nil, NetworkError.failedRequest(status: httpResponse.statusCode))
case let .failure(deserializationError):
completion?(nil, nil, deserializationError)
return
}

case let .failure(validationError):
completion?(nil, nil, validationError)
return
}
}

if let json = try? JSONSerialization.jsonObject(with: data),
let result = Output(json: json) {
/// Deserializes the JSON into a model, if possible.
///
/// - Parameter data: The JSON `Data` to deserialize.
/// - Returns: If success, returns the deserialized model. Otherwise, returns a `NetworkError`.
func deserializeJSON<Output: JSONInitializable>(data: Data) -> Result<Output, NetworkError> {
guard let json = try? JSONSerialization.jsonObject(with: data),
let result = Output(json: json) else {

let links = httpResponse.links
let link = links["next"]?["url"]
completion?(result, link, nil)
return
return .failure(NetworkError.invalidJSON)
}

completion?(nil, nil, NetworkError.invalidJSON)
return
return .success(result)
}

}
32 changes: 0 additions & 32 deletions Sources/GitHubScannerKit/Command/ScanOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,38 +12,6 @@ import GitHubKit
import Result


public enum ScanOptionsError: GitHubScannerProtocolError {
case invalidCategory(value: String)
case invalidRepositoryType(value: String)
case missingAuthorization
case missingOwner
case unknown(error: Error)
}


extension ScanOptionsError: Equatable {

public static func == (lhs: ScanOptionsError, rhs: ScanOptionsError) -> Bool {
switch (lhs, rhs) {
case (let .invalidCategory(lhsValue), let .invalidCategory(rhsValue)):
return lhsValue == rhsValue

case (let .invalidRepositoryType(lhsValue), let .invalidRepositoryType(rhsValue)):
return lhsValue == rhsValue

case (.missingAuthorization, .missingAuthorization),
(.missingOwner, .missingOwner):

return true

default:
return false
}
}

}


public struct ScanOptions: OptionsProtocol {


Expand Down
Loading