[SSDK-623] Improve error reporting when guard-let-self fails to unwra…

…p in SearchEngine (#193) ### Description Fixes https://mapbox.atlassian.net/browse/SSDK-623 - Invoking SDK functions on transient/weak objects (such as `Category().search()`) will create an object with no ownership, it will be deallocated, the network request will complete/fail, and the handler will not have any self object to unwrap and report to. - Previously this could cause an obtuse error message about the network API failing - With this change the error is more correctly attributed to object ownership in the local code ### Checklist - [x] Update `CHANGELOG`
mapbox · Mar 19, 2024 · 54f51a8 · 54f51a8
1 parent e0c8816
commit 54f51a8
Show file tree

Hide file tree

Showing 9 changed files with 81 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,9 @@ Guide: https://keepachangelog.com/en/1.0.0/
 
 <!-- Add changes for active work here -->
 
+- [Core] Add SearchError.owningObjectDeallocated when network responses fail to unwrap guard-let-self. If you encounter this error you must own the reference to the search engine.
+- [Tests] Add UnownedObjectError tests to validate behavior of SearchError.owningObjectDeallocated.
+
 - [Tests] Reorganize tests based on API type
 
 - [Privacy] Add Search history collected data for the purpose of product personalization (used for displaying the search history)

diff --git a/MapboxSearch.xcodeproj/project.pbxproj b/MapboxSearch.xcodeproj/project.pbxproj
@@ -7,6 +7,7 @@
 	objects = {
 
 /* Begin PBXBuildFile section */
+		0405809D2BA8E67D00A54CB9 /* OwningObjectDeallocatedErrorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0405809C2BA8E67D00A54CB9 /* OwningObjectDeallocatedErrorTests.swift */; };
 		042477C32B7290F900D870D5 /* SearchEngineGeocodingIntegrationTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 042477C12B7290E700D870D5 /* SearchEngineGeocodingIntegrationTests.swift */; };
 		042477C52B72CCB000D870D5 /* geocoding-reverse-geocoding.json in Resources */ = {isa = PBXBuildFile; fileRef = 042477C42B72CCB000D870D5 /* geocoding-reverse-geocoding.json */; };
 		042477C62B72CCB000D870D5 /* geocoding-reverse-geocoding.json in Resources */ = {isa = PBXBuildFile; fileRef = 042477C42B72CCB000D870D5 /* geocoding-reverse-geocoding.json */; };
@@ -528,6 +529,7 @@
 /* End PBXCopyFilesBuildPhase section */
 
 /* Begin PBXFileReference section */
+		0405809C2BA8E67D00A54CB9 /* OwningObjectDeallocatedErrorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OwningObjectDeallocatedErrorTests.swift; sourceTree = "<group>"; };
 		042477C12B7290E700D870D5 /* SearchEngineGeocodingIntegrationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SearchEngineGeocodingIntegrationTests.swift; sourceTree = "<group>"; };
 		042477C42B72CCB000D870D5 /* geocoding-reverse-geocoding.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "geocoding-reverse-geocoding.json"; sourceTree = "<group>"; };
 		043A3D4C2B30F38300DB681B /* CoreAddress+AddressComponents.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "CoreAddress+AddressComponents.swift"; sourceTree = "<group>"; };
@@ -1627,6 +1629,7 @@
 		F9C557292670C88E00BE8B94 /* MapboxSearchIntegrationTests */ = {
 			isa = PBXGroup;
 			children = (
+				0405809C2BA8E67D00A54CB9 /* OwningObjectDeallocatedErrorTests.swift */,
 				04BBC6332B61898F00E24E99 /* LocalhostMockServiceProvider.swift */,
 				2CE1B9F72A13C4E6005B043F /* AddressAutofillIntegrationTests.swift */,
 				2C10133329F1C6200094413F /* PlaceAutocompleteIntegrationTests.swift */,
@@ -2827,6 +2830,7 @@
 				F9C557BA2670CCAB00BE8B94 /* TestDataProviderRecord.swift in Sources */,
 				042477C32B7290F900D870D5 /* SearchEngineGeocodingIntegrationTests.swift in Sources */,
 				F9C557672670CB0400BE8B94 /* MockResponse.swift in Sources */,
+				0405809D2BA8E67D00A54CB9 /* OwningObjectDeallocatedErrorTests.swift in Sources */,
 				04BBC6342B61898F00E24E99 /* LocalhostMockServiceProvider.swift in Sources */,
 				2C10133429F1C6200094413F /* PlaceAutocompleteIntegrationTests.swift in Sources */,
 				F9C557682670CB0400BE8B94 /* MockWebServer.swift in Sources */,

diff --git a/Sources/MapboxSearch/PublicAPI/Engine/CategorySearchEngine.swift b/Sources/MapboxSearch/PublicAPI/Engine/CategorySearchEngine.swift
@@ -24,7 +24,12 @@ public class CategorySearchEngine: AbstractSearchEngine {
             categories: [categoryName],
             options: coreOptions
         ) { [weak self, weak eventsManager] coreResponse in
-            guard let self, let coreResponse else {
+            guard let self else {
+                completion(.failure(SearchError.owningObjectDeallocated))
+                return
+            }
+
+            guard let coreResponse else {
                 let error = SearchError.categorySearchRequestFailed(reason: SearchError.responseProcessingFailed)
                 eventsManager?.reportError(error)
 

diff --git a/Sources/MapboxSearch/PublicAPI/Engine/SearchEngine.swift b/Sources/MapboxSearch/PublicAPI/Engine/SearchEngine.swift
@@ -393,7 +393,10 @@ extension SearchEngine {
             retrieve(suggestion: querySuggestion)
         case let resultSuggestion as SearchResultSuggestion:
             resolve(suggestion: resultSuggestion) { [weak self] (result: Result<SearchResult, SearchError>) in
-                guard let self else { return }
+                guard let self else {
+                    assertionFailure("Owning object was deallocated")
+                    return
+                }
 
                 switch result {
                 case .success(let resolvedResult):
@@ -463,7 +466,10 @@ extension SearchEngine {
         }
 
         engineReverseGeocodingFunction(options.toCore()) { [weak self] response in
-            guard let self else { return }
+            guard let self else {
+                assertionFailure("Owning object was deallocated")
+                return
+            }
 
             guard let response else {
                 self.eventsManager.reportError(.responseProcessingFailed)

diff --git a/Sources/MapboxSearch/PublicAPI/Errors/CustomNSError.swift b/Sources/MapboxSearch/PublicAPI/Errors/CustomNSError.swift
@@ -19,6 +19,7 @@ extension SearchError: CustomNSError {
         case .reverseGeocodingFailed: return -9
         case .searchRequestCancelled: return -10
         case .internalSearchRequestError: return -11
+        case .owningObjectDeallocated: return -12
         }
     }
 
@@ -90,6 +91,14 @@ extension SearchError: CustomNSError {
                 NSLocalizedDescriptionKey: description,
                 NSLocalizedFailureReasonErrorKey: "Error:[\(error.description)]",
             ]
+        case .owningObjectDeallocated:
+            let description = "Owning object deallocated"
+            let reason =
+                "Weak-self could not be unwrapped because the owning reference was weak. Please replace with a strong-ownership reference."
+            return [
+                NSLocalizedDescriptionKey: description,
+                NSLocalizedFailureReasonErrorKey: reason,
+            ]
         }
     }
 }
diff --git a/Sources/MapboxSearch/PublicAPI/Errors/SearchError.swift b/Sources/MapboxSearch/PublicAPI/Errors/SearchError.swift
@@ -46,6 +46,10 @@ public enum SearchError: Error {
     /// Reverse geocoding request was failed. Checkout `reason` value for details.
     case reverseGeocodingFailed(reason: Error, options: ReverseGeocodingOptions)
 
+    /// Weak-self could not be unwrapped because the owning reference was weak. Please replace with a strong-ownership
+    /// reference.
+    case owningObjectDeallocated
+
     init(_ error: NSError) {
         self = .generic(code: error.code, domain: error.domain, message: error.description)
     }

diff --git a/Sources/MapboxSearch/PublicAPI/Use Cases/Address Autofill/AddressAutofill.swift b/Sources/MapboxSearch/PublicAPI/Use Cases/Address Autofill/AddressAutofill.swift
@@ -118,7 +118,10 @@ extension AddressAutofill {
         switch suggestion.underlying {
         case .suggestion(let coreSearch, let coreOptions):
             searchEngine.nextSearch(for: coreSearch, with: coreOptions) { [weak self] coreResponse in
-                guard let self else { return }
+                guard let self else {
+                    completion(.failure(SearchError.owningObjectDeallocated))
+                    return
+                }
 
                 self.manage(response: coreResponse, completion: completion)
             }
@@ -187,7 +190,10 @@ extension AddressAutofill {
             categories: [],
             options: options
         ) { [weak self] response in
-            guard let self else { return }
+            guard let self else {
+                completion(.failure(SearchError.owningObjectDeallocated))
+                return
+            }
 
             self.manage(response: response, for: query, completion: completion)
         }

diff --git a/Sources/MapboxSearch/PublicAPI/Use Cases/Place Autocomplete/PlaceAutocomplete.swift b/Sources/MapboxSearch/PublicAPI/Use Cases/Place Autocomplete/PlaceAutocomplete.swift
@@ -198,7 +198,10 @@ extension PlaceAutocomplete {
             categories: [],
             options: options
         ) { [weak self] response in
-            guard let self else { return }
+            guard let self else {
+                completion(.failure(SearchError.owningObjectDeallocated))
+                return
+            }
 
             self.manage(response: response, for: query, completion: completion)
         }

diff --git a/Tests/MapboxSearchIntegrationTests/OwningObjectDeallocatedErrorTests.swift b/Tests/MapboxSearchIntegrationTests/OwningObjectDeallocatedErrorTests.swift
@@ -0,0 +1,35 @@
+// Copyright © 2024 Mapbox. All rights reserved.
+
+@testable import MapboxSearch
+import XCTest
+
+/// Tests for SearchEngine objects that fail `guard-let-self` unwrapping during network response completion blocks.
+/// These exemplify incorrect behavior such as: `Category().search(for: ...` —— you must keep an owning
+/// reference to a search engine that you instantiate and use for network requests.
+final class OwningObjectDeallocatedErrorTests: MockServerIntegrationTestCase<SearchBoxMockResponse> {
+    /// Do not use Category() this way because you will hit the same error.
+    /// Instead you should own your category instance, such as: `let category = Category()`
+    func test_category_object() throws {
+        try server.setResponse(.categoryHotelSearchAlongRoute_JP)
+        let expectation = XCTestExpectation(description: "Expecting results")
+        let coordinate1 = CLLocationCoordinate2D(latitude: 35.655614, longitude: 139.7081684)
+
+        Category(locationProvider: DefaultLocationProvider())
+            .search(for: .cafe, proximity: coordinate1) { response in
+                if case .failure(let failure) = response,
+                   let searchFailure = failure as? SearchError
+                {
+                    XCTAssertEqual(
+                        searchFailure,
+                        SearchError.owningObjectDeallocated
+                    )
+                    expectation.fulfill()
+                } else {
+                    XCTFail(
+                        "Expected transient Category() instance to be deallocated before network response completed resulting in an error."
+                    )
+                }
+            }
+        wait(for: [expectation], timeout: 10)
+    }
+}