Merge pull request #770 from kimdv/kimdv/add-retry-if-graphql-errors

Introducing delegate if there is GraphQL errors

Author: designatednerd

Sources/Apollo/HTTPNetworkTransport.swift

@@ -65,6 +65,21 @@ public protocol HTTPNetworkTransportRetryDelegate: HTTPNetworkTransportDelegate
                         retryHandler: @escaping (_ shouldRetry: Bool) -> Void)
 }
 
+/// Methods which will be called after some kind of response has been received and it contains GraphQLErrors
+public protocol HTTPNetworkTransportGraphQLErrorDelegate: HTTPNetworkTransportDelegate {
+
+
+  /// Called when response contains one or more GraphQL errors.
+  /// NOTE: Don't just call the `retryHandler` with `true` all the time, or you can potentially wind up in an infinite loop of errors
+  ///
+  /// - Parameters:
+  ///   - networkTransport: The network transport which received the error
+  ///   - errors: The received GraphQL errors
+  ///   - retryHandler: A closure indicating whether the operation should be retried. Asyncrhonous to allow for re-authentication or other async operations to complete.
+  func networkTransport(_ networkTransport: HTTPNetworkTransport, receivedGraphQLErrors errors: [GraphQLError], retryHandler: @escaping (_ shouldRetry: Bool) -> Void)
+}
+
+
 // MARK: -
 
 /// A network transport that uses HTTP POST requests to send GraphQL operations to a server, and that uses `URLSession` as the networking implementation.
@@ -131,6 +146,7 @@ public class HTTPNetworkTransport {
 
       if let receivedError = error {
         self.handleErrorOrRetry(operation: operation,
+                                files: files,
                                 error: receivedError,
                                 for: request,
                                 response: response,
@@ -147,6 +163,7 @@ public class HTTPNetworkTransport {
                                                          response: httpResponse,
                                                          kind: .errorResponse)
         self.handleErrorOrRetry(operation: operation,
+                                files: files,
                                 error: unsuccessfulError,
                                 for: request,
                                 response: response,
@@ -159,6 +176,7 @@ public class HTTPNetworkTransport {
                                              response: httpResponse,
                                              kind: .invalidResponse)
         self.handleErrorOrRetry(operation: operation,
+                                files: files,
                                 error: error,
                                 for: request,
                                 response: response,
@@ -170,10 +188,13 @@ public class HTTPNetworkTransport {
         guard let body = try self.serializationFormat.deserialize(data: data) as? JSONObject else {
           throw GraphQLHTTPResponseError(body: data, response: httpResponse, kind: .invalidResponse)
         }
+        
         let graphQLResponse = GraphQLResponse(operation: operation, body: body)
+        
         if let errors = graphQLResponse.parseErrorsOnlyFast() {
           // Handle specific errors from response
           self.handleGraphQLErrorsIfNeeded(operation: operation,
+                                           files: files,
                                            for: request,
                                            body: body,
                                            errors: errors,
@@ -183,6 +204,7 @@ public class HTTPNetworkTransport {
         }
       } catch let parsingError {
         self.handleErrorOrRetry(operation: operation,
+                                files: files,
                                 error: parsingError,
                                 for: request,
                                 response: response,
@@ -194,29 +216,60 @@ public class HTTPNetworkTransport {
 
     return task
   }
+
+  private func handleGraphQLErrorsOrComplete<Operation>(operation: Operation,
+                                                        files: [GraphQLFile]?,
+                                                        response: GraphQLResponse<Operation>,
+                                                        completionHandler: @escaping (_ result: Result<GraphQLResponse<Operation>, Error>) -> Void) {
+    guard let delegate = self.delegate as? HTTPNetworkTransportGraphQLErrorDelegate,
+      let graphQLErrors = response.parseErrorsOnlyFast(),
+      !graphQLErrors.isEmpty else {
+        completionHandler(.success(response))
+        return
+    }
+    
+    delegate.networkTransport(self, receivedGraphQLErrors: graphQLErrors, retryHandler: { [weak self] shouldRetry in
+      guard let self = self else {
+        // None of the rest of this really matters
+        return
+      }
+
+      guard shouldRetry else {
+        completionHandler(.success(response))
+        return
+      }
+      
+      _ = self.send(operation: operation,
+                    isPersistedQueryRetry: self.enableAutoPersistedQueries,
+                    files: files,
+                    completionHandler: completionHandler)
+    })
+  }
 
   private func handleGraphQLErrorsIfNeeded<Operation>(operation: Operation,
+                                                      files: [GraphQLFile]?,
                                                       for request: URLRequest,
                                                       body: JSONObject,
                                                       errors: [GraphQLError],
                                                       completionHandler: @escaping (_ results: Result<GraphQLResponse<Operation>, Error>) -> Void) {
-    
+
     let errorMessages = errors.compactMap { $0.message }
     if self.enableAutoPersistedQueries,
       errorMessages.contains("PersistedQueryNotFound") {
-        // We need to retry this with the full body.
-        _ = self.send(operation: operation,
-                      isPersistedQueryRetry: true,
-                      files: nil,
-                      completionHandler: completionHandler)
+      // We need to retry this with the full body.
+      _ = self.send(operation: operation,
+                    isPersistedQueryRetry: true,
+                    files: nil,
+                    completionHandler: completionHandler)
     } else {
       // Pass the response on to the rest of the chain
       let response = GraphQLResponse(operation: operation, body: body)
-      completionHandler(.success(response))
+      handleGraphQLErrorsOrComplete(operation: operation, files: files, response: response, completionHandler: completionHandler)
     }
   }
-  
+
   private func handleErrorOrRetry<Operation>(operation: Operation,
+                                             files: [GraphQLFile]?,
                                              error: Error,
                                              for request: URLRequest,
                                              response: URLResponse?,
@@ -234,12 +287,20 @@ public class HTTPNetworkTransport {
       for: request,
       response: response,
       retryHandler: { [weak self] shouldRetry in
+        guard let self = self else {
+          // None of the rest of this really matters
+          return
+        }
+        
         guard shouldRetry else {
           completionHandler(.failure(error))
           return
         }
 
-        _ = self?.send(operation: operation, completionHandler: completionHandler)
+        _ = self.send(operation: operation,
+                      isPersistedQueryRetry: self.enableAutoPersistedQueries,
+                      files: files,
+                      completionHandler: completionHandler)
     })
   }
 
@@ -287,7 +348,7 @@ public class HTTPNetworkTransport {
                                   sendQueryDocument: sendQueryDocument,
                                   autoPersistQueries: autoPersistQueries)
   }
-    
+
   private func createRequest<Operation: GraphQLOperation>(for operation: Operation,
                                                           files: [GraphQLFile]?,
                                                           httpMethod: GraphQLHTTPMethod,

Tests/ApolloTestSupport/MockURLSession.swift

@@ -9,19 +9,27 @@ import Foundation
 
 public final class MockURLSession: URLSession {
   public private (set) var lastRequest: URLRequest?
-  
+
+  public var data: Data?
+  public var response: HTTPURLResponse?
+  public var error: Error?
+
   override public func dataTask(with request: URLRequest) -> URLSessionDataTask {
     lastRequest = request
     return URLSessionDataTaskMock()
   }
 
   override public func dataTask(with request: URLRequest, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {
     lastRequest = request
+    if let response = response {
+      completionHandler(data, response, error)
+    }
     return URLSessionDataTaskMock()
   }
 }
 
 private final class URLSessionDataTaskMock: URLSessionDataTask {
   override func resume() {
+
   }
 }

Tests/ApolloTests/HTTPTransportTests.swift

@@ -9,6 +9,7 @@
 import XCTest
 @testable import Apollo
 import StarWarsAPI
+import ApolloTestSupport
 
 class HTTPTransportTests: XCTestCase {
 
@@ -23,6 +24,8 @@ class HTTPTransportTests: XCTestCase {
   private var shouldModifyURLInWillSend = false
   private var retryCount = 0
 
+  private var graphQlErrors = [GraphQLError]()
+
   private lazy var url = URL(string: "http://localhost:8080/graphql")!
   private lazy var networkTransport = HTTPNetworkTransport(url: self.url,
                                                            useGETForQueries: true,
@@ -59,7 +62,7 @@ class HTTPTransportTests: XCTestCase {
         line: line)
     }
   }
-  
+
   func testPreflightDelegateTellingRequestNotToSend() {
     self.shouldSend = false
 
@@ -193,6 +196,74 @@ class HTTPTransportTests: XCTestCase {
                                                      delegate: self)
     XCTAssertNotEqual(self.networkTransport, nonIdenticalTransport)
   }
+
+  func testErrorDelegateWithErrors() throws {
+    self.retryCount = 0
+    self.graphQlErrors = []
+    let query = HeroNameQuery()
+    // TODO: Replace this with once it is codable https://github.com/apollographql/apollo-ios/issues/467
+    let body = ["errors": [["message": "Test graphql error"]]]
+
+    let mockSession = MockURLSession()
+    mockSession.response = HTTPURLResponse(url: url, statusCode: 200, httpVersion: nil, headerFields: nil)
+    mockSession.data = try JSONSerialization.data(withJSONObject: body, options: .prettyPrinted)
+    let network = HTTPNetworkTransport(url: url, session: mockSession, delegate: self)
+    let expectation = self.expectation(description: "Send operation completed")
+
+    let _ = network.send(operation: query) { result in
+      switch result {
+      case .success:
+        expectation.fulfill()
+      case .failure:
+        break
+      }
+    }
+
+    guard let request = mockSession.lastRequest else {
+      XCTFail("last request should not be nil")
+      return
+    }
+    XCTAssertEqual(request.url?.host, network.url.host)
+    XCTAssertEqual(request.httpMethod, "POST")
+
+    XCTAssertEqual(self.graphQlErrors.count, 1)
+    XCTAssertEqual(retryCount, 1)
+    wait(for: [expectation], timeout: 1)
+  }
+
+  func testErrorDelegateWithNoErrors() throws {
+    self.retryCount = 0
+    self.graphQlErrors = []
+    let query = HeroNameQuery()
+    // TODO: Replace this with once it is codable https://github.com/apollographql/apollo-ios/issues/467
+    let body = ["errors": []]
+
+    let mockSession = MockURLSession()
+    mockSession.response = HTTPURLResponse(url: url, statusCode: 200, httpVersion: nil, headerFields: nil)
+    mockSession.data = try JSONSerialization.data(withJSONObject: body, options: .prettyPrinted)
+    let network = HTTPNetworkTransport(url: url, session: mockSession, delegate: self)
+    let expectation = self.expectation(description: "Send operation completed")
+
+    let _ = network.send(operation: query) { result in
+      switch result {
+      case .success:
+        expectation.fulfill()
+      case .failure:
+        break
+      }
+    }
+
+    guard let request = mockSession.lastRequest else {
+      XCTFail("last request should not be nil")
+      return
+    }
+    XCTAssertEqual(request.url?.host, network.url.host)
+    XCTAssertEqual(request.httpMethod, "POST")
+    XCTAssertEqual(self.retryCount, 0)
+    XCTAssertEqual(self.graphQlErrors.count, 0)
+    wait(for: [expectation], timeout: 1)
+
+  }
 }
 
 // MARK: - HTTPNetworkTransportPreflightDelegate
@@ -266,3 +337,14 @@ extension HTTPTransportTests: HTTPNetworkTransportRetryDelegate {
     }
   }
 }
+
+// MARK: - HTTPNetworkTransportGraphQLErrorDelegate
+
+extension HTTPTransportTests: HTTPNetworkTransportGraphQLErrorDelegate {
+  func networkTransport(_ networkTransport: HTTPNetworkTransport, receivedGraphQLErrors errors: [GraphQLError], retryHandler: @escaping (Bool) -> Void) {
+    self.retryCount += 1
+    let shouldRetry = retryCount == 2
+    self.graphQlErrors = errors
+    retryHandler(shouldRetry)
+  }
+}