Fix network errors treated as “Internal error”

This patch fixes a bug that would cause all network related errors (such
as when the user is offline) as opaque `OhMyAuthError.internalError`, which
prevents the appropriate error handling to be done in clients.

The problem was that the logic dealing with responses was first checking
if a valid `HTTPURLResponse` was received before checking the error. This
patch simply flips the order of those two operations.

Tests have also been implemented for `NetworkRequestable`.

Author: JohnSundell

OhMyAuth.xcodeproj/project.pbxproj

@@ -7,6 +7,8 @@
 	objects = {
 
 /* Begin PBXBuildFile section */
+		5255FEBC1FB457FA004D012B /* NetworkRequestableTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5255FEBB1FB457FA004D012B /* NetworkRequestableTests.swift */; };
+		5255FEBD1FB457FA004D012B /* NetworkRequestableTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5255FEBB1FB457FA004D012B /* NetworkRequestableTests.swift */; };
 		922A1E591DC28A680088A7E7 /* Networking.swift in Sources */ = {isa = PBXBuildFile; fileRef = 922A1E581DC28A680088A7E7 /* Networking.swift */; };
 		922A1E5A1DC28A680088A7E7 /* Networking.swift in Sources */ = {isa = PBXBuildFile; fileRef = 922A1E581DC28A680088A7E7 /* Networking.swift */; };
 		923CA22C1DC144290016B791 /* Utils.swift in Sources */ = {isa = PBXBuildFile; fileRef = 923CA22B1DC144290016B791 /* Utils.swift */; };
@@ -76,6 +78,7 @@
 /* End PBXContainerItemProxy section */
 
 /* Begin PBXFileReference section */
+		5255FEBB1FB457FA004D012B /* NetworkRequestableTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkRequestableTests.swift; sourceTree = "<group>"; };
 		922A1E581DC28A680088A7E7 /* Networking.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Networking.swift; sourceTree = "<group>"; };
 		923CA22B1DC144290016B791 /* Utils.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = Utils.swift; sourceTree = "<group>"; };
 		D20836161EF7D0A8004ED8E0 /* Keychains.framework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.framework; name = Keychains.framework; path = Carthage/Build/iOS/Keychains.framework; sourceTree = "<group>"; };
@@ -308,6 +311,7 @@
 				D5ED737D1C64B1E4003772EF /* Info-Mac.plist */,
 				D2DD2AB31DEEFF2900AFF5BA /* LockerTests.swift */,
 				D20A12A41E769648000DB14C /* ServiceTests.swift */,
+				5255FEBB1FB457FA004D012B /* NetworkRequestableTests.swift */,
 			);
 			path = OhMyAuthTests;
 			sourceTree = "<group>";
@@ -553,6 +557,7 @@
 			files = (
 				D2DD2AB41DEEFF2900AFF5BA /* LockerTests.swift in Sources */,
 				D20A12A51E769648000DB14C /* ServiceTests.swift in Sources */,
+				5255FEBC1FB457FA004D012B /* NetworkRequestableTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -588,6 +593,7 @@
 			files = (
 				D2DD2AB51DEEFF2900AFF5BA /* LockerTests.swift in Sources */,
 				D20A12A61E769648000DB14C /* ServiceTests.swift in Sources */,
+				5255FEBD1FB457FA004D012B /* NetworkRequestableTests.swift in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};

OhMyAuthTests/NetworkRequestableTests.swift

@@ -0,0 +1,61 @@
+import Foundation
+import XCTest
+@testable import OhMyAuth
+
+final class NetworkRequestableTests: XCTestCase {
+  func testSuccessfulResult() throws {
+    let networking = NetworkingMock(configuration: .default)
+    networking.data = try JSONSerialization.data(withJSONObject: [:], options: [])
+    networking.response = HTTPURLResponse()
+
+    var result: Result<Any>?
+    let request = RequestMock()
+    request.start(using: networking) { result = $0 }
+
+    switch result {
+    case .some(.success(let object)):
+      // Since we get an untyped object from the API, we need to use reflection for verification
+      XCTAssertEqual(String(reflecting: object), String(reflecting: [:]))
+    default:
+      XCTFail("Unexpected result: \(String(describing: result))")
+    }
+  }
+
+  func testOfflineErrorReturnedAsResult() {
+    let networking = NetworkingMock(configuration: .default)
+    networking.error = NSError(
+      domain: NSURLErrorDomain,
+      code: URLError.notConnectedToInternet.rawValue,
+      userInfo: nil
+    )
+
+    var result: Result<Any>?
+    let request = RequestMock()
+    request.start(using: networking) { result = $0 }
+
+    switch result {
+    case .some(.failure(let error as NSError)):
+      XCTAssertEqual(error.code, URLError.notConnectedToInternet.rawValue)
+    default:
+      XCTFail("Unexpected result: \(String(describing: result))")
+    }
+  }
+}
+
+private extension NetworkRequestableTests {
+  struct RequestMock: NetworkRequestable {
+    let url = URL(string: "https://www.hyper.no")!
+    let parameters = [String: Any]()
+    let headers = [String: String]()
+  }
+
+  class NetworkingMock: Networking {
+    var data: Data?
+    var response: URLResponse?
+    var error: Error?
+
+    override func post(url: URL, parameters: [String : Any], headers: [String : String], completion: @escaping ((Data?, URLResponse?, Error?) -> Void)) {
+      completion(data, response, error)
+    }
+  }
+}

Sources/Shared/Networking/Networking.swift

@@ -7,7 +7,7 @@ open class Networking {
     session = URLSession(configuration: configuration, delegate: nil, delegateQueue: OperationQueue.main)
   }
 
-  public func post(url: URL, parameters: [String: Any], headers: [String: String], completion: @escaping ((Data?, URLResponse?, Error?) -> Void)) {
+  open func post(url: URL, parameters: [String: Any], headers: [String: String], completion: @escaping ((Data?, URLResponse?, Error?) -> Void)) {
     var request = URLRequest(url: url)
 
     headers.forEach { (key, value) in

Sources/Shared/Protocols/NetworkRequestable.swift

@@ -8,13 +8,8 @@ protocol NetworkRequestable {
 
 extension NetworkRequestable {
 
-  func start(_ completion: @escaping (_ result: Result<Any>) -> Void) {
-    AuthConfig.networking.post(url: url, parameters: parameters, headers: headers) { (data, response, error) in
-      guard let response = response as? HTTPURLResponse else {
-        completion(Result.failure(OhMyAuthError.internalError.toNSError()))
-        return
-      }
-      
+  func start(using networking: Networking = AuthConfig.networking, completion: @escaping (_ result: Result<Any>) -> Void) {
+    networking.post(url: url, parameters: parameters, headers: headers) { (data, response, error) in
       guard error == nil
       else {
         if let error = error {
@@ -25,6 +20,11 @@ extension NetworkRequestable {
 
         return
       }
+
+      guard let response = response as? HTTPURLResponse else {
+        completion(Result.failure(OhMyAuthError.internalError.toNSError()))
+        return
+      }
 
       guard let data = data,
         let jsonObject = try? JSONSerialization.jsonObject(with: data, options: .allowFragments),