From 54f51a89ae5e32ac9597809fba9a5fa461906a4a Mon Sep 17 00:00:00 2001 From: Jack Alto <384288+aokj4ck@users.noreply.github.com> Date: Tue, 19 Mar 2024 11:02:40 -0400 Subject: [PATCH] [SSDK-623] Improve error reporting when guard-let-self fails to unwrap 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` --- CHANGELOG.md | 3 ++ MapboxSearch.xcodeproj/project.pbxproj | 4 +++ .../Engine/CategorySearchEngine.swift | 7 +++- .../PublicAPI/Engine/SearchEngine.swift | 10 ++++-- .../PublicAPI/Errors/CustomNSError.swift | 9 +++++ .../PublicAPI/Errors/SearchError.swift | 4 +++ .../Address Autofill/AddressAutofill.swift | 10 ++++-- .../PlaceAutocomplete.swift | 5 ++- .../OwningObjectDeallocatedErrorTests.swift | 35 +++++++++++++++++++ 9 files changed, 81 insertions(+), 6 deletions(-) create mode 100644 Tests/MapboxSearchIntegrationTests/OwningObjectDeallocatedErrorTests.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ac0138f9..6d4e6fc8e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ Guide: https://keepachangelog.com/en/1.0.0/ +- [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 index c93462af0..9a07dc491 100644 --- 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 = ""; }; 042477C12B7290E700D870D5 /* SearchEngineGeocodingIntegrationTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SearchEngineGeocodingIntegrationTests.swift; sourceTree = ""; }; 042477C42B72CCB000D870D5 /* geocoding-reverse-geocoding.json */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.json; path = "geocoding-reverse-geocoding.json"; sourceTree = ""; }; 043A3D4C2B30F38300DB681B /* CoreAddress+AddressComponents.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "CoreAddress+AddressComponents.swift"; sourceTree = ""; }; @@ -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 index 0d2a83b82..a4e5aaf4e 100644 --- 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 index c6d16e15b..d8db6b1da 100644 --- 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) 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 index 3f8ff6526..ebbc56bd6 100644 --- 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 index 6c4185281..b21bb6a72 100644 --- 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 index f4a3efafc..c3473fd1a 100644 --- 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 index 51f906aa1..58b1b6141 100644 --- 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 new file mode 100644 index 000000000..cad98f7cd --- /dev/null +++ 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 { + /// 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) + } +}