fix: Fail requests when encoding request body fails.

amplitude · Aug 20, 2024 · 79a51c2 · 79a51c2
1 parent 056ccbf
commit 79a51c2
Show file tree

Hide file tree

Showing 9 changed files with 47 additions and 18 deletions.
diff --git a/Examples/AmplitudeObjCExample/AmplitudeObjCExampleTests/TestAmplitude.swift b/Examples/AmplitudeObjCExample/AmplitudeObjCExampleTests/TestAmplitude.swift
@@ -98,7 +98,8 @@ class TestStorage: Storage {
         configuration: Configuration,
         eventPipeline: EventPipeline,
         eventBlock: URL,
-        eventsString: String
+        eventsString: String,
+        logger: (any Logger)?
     ) -> ResponseHandler {
         class TestResponseHandler: ResponseHandler {
 

diff --git a/Sources/Amplitude/Storages/InMemoryStorage.swift b/Sources/Amplitude/Storages/InMemoryStorage.swift
@@ -42,7 +42,8 @@ class InMemoryStorage: Storage {
         configuration: Configuration,
         eventPipeline: EventPipeline,
         eventBlock: EventBlock,
-        eventsString: String
+        eventsString: String,
+        logger: (any Logger)?
     ) -> ResponseHandler {
         abort()
     }

diff --git a/Sources/Amplitude/Storages/PersistentStorage.swift b/Sources/Amplitude/Storages/PersistentStorage.swift
@@ -129,14 +129,16 @@ class PersistentStorage: Storage {
         configuration: Configuration,
         eventPipeline: EventPipeline,
         eventBlock: EventBlock,
-        eventsString: String
+        eventsString: String,
+        logger: (any Logger)?
     ) -> ResponseHandler {
         return PersistentStorageResponseHandler(
             configuration: configuration,
             storage: self,
             eventPipeline: eventPipeline,
             eventBlock: eventBlock,
-            eventsString: eventsString
+            eventsString: eventsString,
+            logger: logger
         )
     }
 

diff --git a/Sources/Amplitude/Types.swift b/Sources/Amplitude/Types.swift
@@ -60,7 +60,8 @@ public protocol Storage {
         configuration: Configuration,
         eventPipeline: EventPipeline,
         eventBlock: URL,
-        eventsString: String
+        eventsString: String,
+        logger: (any Logger)?
     ) -> ResponseHandler
 }
 

diff --git a/Sources/Amplitude/Utilities/EventPipeline.swift b/Sources/Amplitude/Utilities/EventPipeline.swift
@@ -76,7 +76,8 @@ public class EventPipeline {
                         configuration: self.configuration,
                         eventPipeline: self,
                         eventBlock: eventFile,
-                        eventsString: eventsString
+                        eventsString: eventsString,
+                        logger: logger
                     )
                     responseHandler.handle(result: result)
                     self.completeUpload(for: eventFile)

diff --git a/Sources/Amplitude/Utilities/HttpClient.swift b/Sources/Amplitude/Utilities/HttpClient.swift
@@ -35,7 +35,11 @@ class HttpClient {
         let backgroundTaskCompletion = VendorSystem.current.beginBackgroundTask()
         do {
             let request = try getRequest()
-            let requestData = getRequestData(events: events)
+            // Attempt to generate requestData without diagnostics in case diagnostics are causing a failure.
+            let requestData = getRequestData(events: events) ?? getRequestData(events: events, includeDiagnostics: false)
+            guard let requestData = requestData else {
+                throw Exception.invalidRequestData
+            }
 
             sessionTask = session.uploadTask(with: request, from: requestData) { [callbackQueue, configuration] data, response, error in
                 callbackQueue.async {
@@ -63,8 +67,10 @@ class HttpClient {
             }
             sessionTask!.resume()
         } catch {
-            completion(.failure(Exception.httpError(code: 500, data: nil)))
-            backgroundTaskCompletion?()
+            callbackQueue.async {
+                completion(.failure(Exception.httpError(code: 500, data: nil)))
+                backgroundTaskCompletion?()
+            }
         }
         return sessionTask
     }
@@ -103,7 +109,7 @@ class HttpClient {
         return request
     }
 
-    func getRequestData(events: String) -> Data? {
+    func getRequestData(events: String, includeDiagnostics: Bool = true) -> Data? {
         let apiKey = configuration.apiKey
         let clientUploadTime: String = dateFormatter.string(from: getDate())
         var requestPayload = """
@@ -114,7 +120,7 @@ class HttpClient {
                 ,"options":{"min_id_length":\(minIdLength)}
                 """
         }
-        if diagnostics.hasDiagnostics() {
+        if includeDiagnostics, diagnostics.hasDiagnostics() {
             let diagnosticsInfo = diagnostics.extractDiagonosticsToString()
             if !diagnosticsInfo.isEmpty {
                 requestPayload += """
@@ -144,5 +150,6 @@ extension HttpClient {
     enum Exception: Error {
         case invalidUrl(url: String)
         case httpError(code: Int, data: Data?)
+        case invalidRequestData
     }
 }
diff --git a/Sources/Amplitude/Utilities/PersistentStorageResponseHandler.swift b/Sources/Amplitude/Utilities/PersistentStorageResponseHandler.swift
@@ -13,19 +13,22 @@ class PersistentStorageResponseHandler: ResponseHandler {
     var eventPipeline: EventPipeline
     var eventBlock: URL
     var eventsString: String
+    private let logger: (any Logger)?
 
     init(
         configuration: Configuration,
         storage: PersistentStorage,
         eventPipeline: EventPipeline,
         eventBlock: URL,
-        eventsString: String
+        eventsString: String,
+        logger: (any Logger)?
     ) {
         self.configuration = configuration
         self.storage = storage
         self.eventPipeline = eventPipeline
         self.eventBlock = eventBlock
         self.eventsString = eventsString
+        self.logger = logger
     }
 
     func handleSuccessResponse(code: Int) {
@@ -123,6 +126,11 @@ class PersistentStorageResponseHandler: ResponseHandler {
         // wait for next time to try again
     }
 
+    func handleInvalidDataResponse() {
+        logger?.error(message: "Unable to construct a request from block \(eventBlock), deleting...")
+        storage.remove(eventBlock: eventBlock)
+    }
+
     func handle(result: Result<Int, Error>) {
         switch result {
         case .success(let code):
@@ -149,6 +157,8 @@ class PersistentStorageResponseHandler: ResponseHandler {
                 default:
                     handleFailedResponse(data: json)
                 }
+            case HttpClient.Exception.invalidRequestData:
+                handleInvalidDataResponse()
             default:
                 break
             }

diff --git a/Tests/AmplitudeTests/Supports/TestUtilities.swift b/Tests/AmplitudeTests/Supports/TestUtilities.swift
@@ -122,7 +122,8 @@ class FakeInMemoryStorage: Storage {
         configuration: Configuration,
         eventPipeline: EventPipeline,
         eventBlock: EventBlock,
-        eventsString: String
+        eventsString: String,
+        logger: (any Logger)?
     ) -> ResponseHandler {
         FakeResponseHandler(
             configuration: configuration, storage: self, eventPipeline: eventPipeline, eventBlock: eventBlock,

diff --git a/Tests/AmplitudeTests/Utilities/PersistentStorageResponseHandlerTests.swift b/Tests/AmplitudeTests/Utilities/PersistentStorageResponseHandlerTests.swift
@@ -39,7 +39,8 @@ final class PersistentStorageResponseHandlerTests: XCTestCase {
             storage: storage,
             eventPipeline: eventPipeline,
             eventBlock: eventBlock,
-            eventsString: eventsString
+            eventsString: eventsString,
+            logger: nil
         )
 
         XCTAssertEqual(handler.eventsString, eventsString)
@@ -58,7 +59,8 @@ final class PersistentStorageResponseHandlerTests: XCTestCase {
             storage: fakePersistentStorage,
             eventPipeline: eventPipeline,
             eventBlock: eventBlock,
-            eventsString: eventsString
+            eventsString: eventsString,
+            logger: nil
         )
 
         handler.removeEventCallbackByEventsString(eventsString: eventsString)
@@ -91,7 +93,8 @@ final class PersistentStorageResponseHandlerTests: XCTestCase {
             storage: fakePersistentStorage,
             eventPipeline: eventPipeline,
             eventBlock: eventBlock,
-            eventsString: eventsString
+            eventsString: eventsString,
+            logger: nil
         )
 
         handler.removeEventCallbackByEventsString(eventsString: eventsString)
@@ -116,7 +119,8 @@ final class PersistentStorageResponseHandlerTests: XCTestCase {
             storage: fakePersistentStorage,
             eventPipeline: eventPipeline,
             eventBlock: eventBlock,
-            eventsString: eventsString
+            eventsString: eventsString,
+            logger: nil
         )
 
         handler.handleSuccessResponse(code: 200)
@@ -147,7 +151,8 @@ final class PersistentStorageResponseHandlerTests: XCTestCase {
             storage: fakePersistentStorage,
             eventPipeline: eventPipeline,
             eventBlock: eventBlock,
-            eventsString: eventsString
+            eventsString: eventsString,
+            logger: nil
         )
 
         handler.handleBadRequestResponse(data: ["error": "Invalid API key: \(configuration.apiKey)"])