From f550d32802e1ad2e06294f7c6bb4468e93e71595 Mon Sep 17 00:00:00 2001 From: Stelios Petrakis Date: Wed, 3 Jul 2024 07:49:50 +0200 Subject: [PATCH 1/5] Ensure that callbacks won't capture self strongly Given that the main SDK instance (`tx`) can be set to `nil` using the `dispose()` method at any time, we need to ensure that the callback methods of the networking logic (`fetchTranslations`, `pushTranslations`, `foceCacheInvalidation`) will not capture the instance strongly, retaining it more than necessary. In order to address this, the internal `CDSPullRequest` class has been removed in favor of a private method in the `CDSHandler` class (`performFetch(...)`) that behaves similarly and ensures that the network callbacks are capturing a weak reference to the `CDSHandler` instance. --- Sources/Transifex/CDSHandler.swift | 207 ++++++++++++++--------------- Sources/Transifex/Cache.swift | 6 +- Sources/Transifex/Core.swift | 22 ++- 3 files changed, 126 insertions(+), 109 deletions(-) diff --git a/Sources/Transifex/CDSHandler.swift b/Sources/Transifex/CDSHandler.swift index f5af103..ae0e955 100644 --- a/Sources/Transifex/CDSHandler.swift +++ b/Sources/Transifex/CDSHandler.swift @@ -119,94 +119,6 @@ TXPushConfiguration(purge: \(purge), overrideTags: \(overrideTags), overrideOccu } } -/// Handles the logic of a pull HTTP request to CDS for a certain locale code -class CDSPullRequest { - let code : String - let request : URLRequest - let session : URLSession - - private var retryCount = 0 - - struct RequestData : Codable { - var data: TXLocaleStrings - } - - init(with request : URLRequest, code : String, session : URLSession) { - self.code = code - self.request = request - self.session = session - } - - /// Performs the request to CDS and offers a completion handler when the request succeeds or fails. - /// - /// - Parameter completionHandler: The completion handler that includes the locale code, - /// the extracted LocaleStrings structure from the server response and the error object when the - /// request fails - func perform(with completionHandler: @escaping (String, - TXLocaleStrings?, - TXCDSError?) -> Void) { - session.dataTask(with: request) { (data, response, error) in - guard error == nil else { - completionHandler(self.code, - nil, - .requestFailed(error: error!)) - return - } - - guard let httpResponse = response as? HTTPURLResponse else { - completionHandler(self.code, - nil, - .invalidHTTPResponse) - return - } - - let statusCode = httpResponse.statusCode - - switch statusCode { - - case CDSHandler.HTTP_STATUS_CODE_OK: - if let data = data { - let decoder = JSONDecoder() - - do { - let request = try decoder.decode(RequestData.self, - from: data) - completionHandler(self.code, - request.data, - nil) - } - catch { - completionHandler(self.code, - nil, - .requestFailed(error: error)) - } - } - else { - completionHandler(self.code, - nil, - .nonParsableResponse) - } - case CDSHandler.HTTP_STATUS_CODE_ACCEPTED: - Logger.info("Received 202 response while fetching locale: \(self.code)") - - if self.retryCount < CDSHandler.MAX_RETRIES { - self.retryCount += 1 - self.perform(with: completionHandler) - } - else { - completionHandler(self.code, - nil, - .maxRetriesReached) - } - default: - completionHandler(self.code, - nil, - .serverError(statusCode: statusCode)) - } - }.resume() - } -} - /// The struct used to configure the communication with CDS, passed into the CDSHander initializer. struct CDSConfiguration { /// A list of locale codes for the configured languages in the application @@ -326,7 +238,13 @@ class CDSHandler { case completed case failed } - + + private struct RequestData : Codable { + var data: TXLocaleStrings + } + + private typealias CDSPullRequestResult = Result + /// The url session to be used for the requests to the CDS, defaults to an ephemeral URLSession with /// a disabled URL cache. let session: URLSession @@ -404,24 +322,28 @@ class CDSHandler { } var requestsFinished = 0 + let totalRequests = requestsByLocale.count var translationsByLocale: TXTranslations = [:] var errors: [Error] = [] for (code, requestByLocale) in requestsByLocale { - let cdsRequest = CDSPullRequest(with: requestByLocale, - code: code, - session: self.session) - cdsRequest.perform { (code, localeStrings, error) in - requestsFinished += 1 - - if let error = error { - errors.append(error) + performFetch(retryCount: 0, + code: code, + request: requestByLocale) { [weak self] result in + guard let _ = self else { + return } - else { + + requestsFinished += 1 + + switch result { + case .success(let localeStrings): translationsByLocale[code] = localeStrings + case .failure(let error): + errors.append(error) } - - if requestsFinished == requestsByLocale.count { + + if requestsFinished == totalRequests { completionHandler(translationsByLocale, errors) } } @@ -446,7 +368,10 @@ class CDSHandler { request.httpMethod = "POST" request.allHTTPHeaderFields = getHeaders(withSecret: true) - session.dataTask(with: request) { (data, response, error) in + session.dataTask(with: request) { [weak self] (data, response, error) in + guard let _ = self else { + return + } guard error == nil else { Logger.error("Error invalidating CDS cache: \(error!)") completionHandler(false) @@ -523,6 +448,71 @@ class CDSHandler { } } + // MARK: - Private + + /// Performs the fetch (pull) request to CDS and offers a completion handler when the request succeeds + /// or fails, with the associated Result type (CDSPullRequestResult). + /// + /// - Parameters: + /// - retryCount: The current retry count [0, CDSHandler.MAX_RETRIES] + /// - code: The locale code that is pulled from CDS + /// - request: The actual URLRequest + /// - requestCompleted: The completion handler. + private func performFetch(retryCount: Int, + code: String, + request: URLRequest, + requestCompleted: @escaping (CDSPullRequestResult) -> Void) { + session.dataTask(with: request) { [weak self] + (data, response, error) in + guard let self = self else { + return + } + + if let error = error { + requestCompleted(.failure(.requestFailed(error: error))) + return + } + + guard let httpResponse = response as? HTTPURLResponse else { + requestCompleted(.failure(.invalidHTTPResponse)) + return + } + + let statusCode = httpResponse.statusCode + + switch statusCode { + + case CDSHandler.HTTP_STATUS_CODE_OK: + if let data = data { + do { + let request = try JSONDecoder().decode(RequestData.self, + from: data) + requestCompleted(.success(request.data)) + } + catch { + requestCompleted(.failure(.requestFailed(error: error))) + } + } + else { + requestCompleted(.failure(.nonParsableResponse)) + } + case CDSHandler.HTTP_STATUS_CODE_ACCEPTED: + Logger.info("Received 202 response while fetching locale: \(code)") + if retryCount < CDSHandler.MAX_RETRIES { + performFetch(retryCount: retryCount + 1, + code: code, + request: request, + requestCompleted: requestCompleted) + } + else { + requestCompleted(.failure(.maxRetriesReached)) + } + default: + requestCompleted(.failure(.serverError(statusCode: statusCode))) + } + }.resume() + } + /// Pushes the generated JSON data containing the source strings and propagates any generated /// warnings to the final completion handler. /// @@ -544,7 +534,10 @@ class CDSHandler { request.httpMethod = "POST" request.allHTTPHeaderFields = getHeaders(withSecret: true) - session.dataTask(with: request) { (data, response, error) in + session.dataTask(with: request) { [weak self] (data, response, error) in + guard let self = self else { + return + } if let error = error { completionHandler(false, [.requestFailed(error: error)], warnings) return @@ -606,8 +599,11 @@ class CDSHandler { // have enough time to process the job. Thread.sleep(forTimeInterval: 1.0) - fetchJobStatus(jobURL: jobURL) { + fetchJobStatus(jobURL: jobURL) { [weak self] jobStatus, jobErrors, jobDetails in + guard let self = self else { + return + } guard let finalJobStatus = jobStatus else { completionHandler(false, [.failedJobRequest], warnings) return @@ -683,7 +679,10 @@ failed: \(details.failed) request.httpMethod = "GET" request.allHTTPHeaderFields = getHeaders(withSecret: true) - session.dataTask(with: request) { (data, response, error) in + session.dataTask(with: request) { [weak self] (data, response, error) in + guard let _ = self else { + return + } guard error == nil else { Logger.error("Error retrieving job status: \(error!)") completionHandler(nil, nil, nil) diff --git a/Sources/Transifex/Cache.swift b/Sources/Transifex/Cache.swift index a5a9c1a..f0adac9 100644 --- a/Sources/Transifex/Cache.swift +++ b/Sources/Transifex/Cache.swift @@ -265,7 +265,11 @@ public final class TXFileOutputCacheDecorator: TXDecoratorCache { public override func update(translations: TXTranslations) { super.update(translations: translations) - cacheQueue.async { + cacheQueue.async { [weak self] in + guard let self = self else { + return + } + guard let fileURL = self.fileURL else { return } diff --git a/Sources/Transifex/Core.swift b/Sources/Transifex/Core.swift index 566a907..6f07510 100644 --- a/Sources/Transifex/Core.swift +++ b/Sources/Transifex/Core.swift @@ -171,7 +171,11 @@ class NativeCore : TranslationProvider { completionHandler: TXPullCompletionHandler? = nil) { cdsHandler.fetchTranslations(localeCode: localeCode, tags: tags, - status: status) { (translations, errors) in + status: status) { [weak self] (translations, errors) in + guard let self = self else { + return + } + if errors.count > 0 { Logger.error("\(#function) Errors: \(errors)") } @@ -196,8 +200,13 @@ class NativeCore : TranslationProvider { configuration: TXPushConfiguration = TXPushConfiguration(), completionHandler: @escaping (Bool, [Error], [Error]) -> Void) { cdsHandler.pushTranslations(translations, - configuration: configuration, - completionHandler: completionHandler) + configuration: configuration) { [weak self] + success, errors, warnings in + guard let _ = self else { + return + } + completionHandler(success, errors, warnings) + } } /// Forces CDS cache invalidation. @@ -206,7 +215,12 @@ class NativeCore : TranslationProvider { /// complete with a boolean argument that informs the caller that the operation was successful (true) or /// not (false). func forceCacheInvalidation(completionHandler: @escaping (Bool) -> Void) { - cdsHandler.forceCacheInvalidation(completionHandler: completionHandler) + cdsHandler.forceCacheInvalidation { [weak self] success in + guard let _ = self else { + return + } + completionHandler(success) + } } /// Used by the Swift localizedString(format:arguments:) methods found in the From a954a7998ed4b130b5f7cdbe293f59552b128aef Mon Sep 17 00:00:00 2001 From: Stelios Petrakis Date: Wed, 3 Jul 2024 09:02:57 +0200 Subject: [PATCH 2/5] Ensure Designed for iPhone/iPad apps use the proper device name In device variation rules, the logic uses the iPad device name when the user interface idiom is Pad,but that does not cover the corner case where an iOS application targets a Vision device as a 'Designed for iPad' application. In order to on par with the logic implemented by Apple, the 'Designed for iPad' applications on Vision devices, now use the correct device name. The rest of the cases were also reviewed ('Designed for iPhone' / 'Designed for iPad' iOS apps running either on Mac or Vision devices). More specifically: * iPad applications running inside a Mac ("Designed for iPad") return the "ipad" device name. * iPhone applications running inside a Mac ("Designed for iPhone") return the "iphone" device name. * iPad applications running inside a Vision Pro ("Designed for iPad") return the "applevision" device name. * iPhone applications running inside a Vision Pro ("Designed for iPhone") return the "iphone" device name. --- Sources/Transifex/Plurals.swift | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/Sources/Transifex/Plurals.swift b/Sources/Transifex/Plurals.swift index 94b372e..ddee83b 100644 --- a/Sources/Transifex/Plurals.swift +++ b/Sources/Transifex/Plurals.swift @@ -496,24 +496,40 @@ final class XMLPluralParser: NSObject { return pluralRules.sorted { $0.key.rawValue < $1.key.rawValue } } + /// There is currently no native way of knowing whether the current iOS application runs on a Vision + /// device (something like `isiOSAppOnMac` but for Vision devices). So here's a (hacky) way of doing + /// that until Apple adds a helper method, by using a public class that is only available on VisionOS. + /// + /// - Returns: True if the iOS app runs on a Vision device, False otherwise. + private static func isiOSAppOnVision() -> Bool { + struct Static { + static var isOnVisionDevice: Bool = { + return NSClassFromString("UIWindowSceneGeometryPreferencesVision") != nil + }() + } + return Static.isOnVisionDevice + } + /// Returns the current device name in the form used by the `.xcstrings` file type. /// /// - Returns: The current device name private class func currentDeviceName() -> String { #if os(iOS) - // For iOS, figure out whether the current device is an iPhone, an iPad - // or an iPod. - #if canImport(UIKit) + // For iOS applications running on a Mac or a Vision device (as + // 'Designed for iPhone' / 'Designed for iPad'), we need to respect the + // user interface idiom. + // The only exception is that if the user interface idiom is Pad (so + // the iOS app is running as 'Designed for iPad') and the current device + // is Vision Pro. In that case we need to set the current device name as + // 'applevision' instead of 'ipad'. For 'Designed for iPhone' iOS apps + // running in Vision Pro devices, we should return 'iphone'. let currentDevice = UIDevice.current - if currentDevice.userInterfaceIdiom == UIUserInterfaceIdiom.pad { - return DEVICE_NAME_IPAD + if currentDevice.userInterfaceIdiom == .pad { + return isiOSAppOnVision() ? DEVICE_NAME_VISION : DEVICE_NAME_IPAD } else { return currentDevice.model.hasPrefix("iPod") ? DEVICE_NAME_IPOD : DEVICE_NAME_IPHONE } - #else - return DEVICE_NAME_IPHONE - #endif #elseif os(macOS) return DEVICE_NAME_MAC #elseif os(watchOS) From 486c5006ccdab95e09098e7bdcebf0943fe2da5c Mon Sep 17 00:00:00 2001 From: Stelios Petrakis Date: Wed, 3 Jul 2024 12:17:04 +0200 Subject: [PATCH 3/5] Disclose that completion handlers are called from background threads Add documentation on the public methods that accept completion handlers that the handlers are called from background threads. --- Sources/Transifex/Core.swift | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Sources/Transifex/Core.swift b/Sources/Transifex/Core.swift index 6f07510..292c6ed 100644 --- a/Sources/Transifex/Core.swift +++ b/Sources/Transifex/Core.swift @@ -164,7 +164,8 @@ class NativeCore : TranslationProvider { /// - Parameter status: An optional status so that only strings matching translation status are /// fetched. /// - Parameter completionHandler: The completion handler that informs the caller with the - /// new translations and a list of possible errors that might have occured + /// new translations and a list of possible errors that might have occured. The completion handler is + /// called from a background thread. func fetchTranslations(_ localeCode: String? = nil, tags: [String] = [], status: String? = nil, @@ -195,7 +196,8 @@ class NativeCore : TranslationProvider { /// - completionHandler: A callback to be called when the push operation is complete with a /// boolean argument that informs the caller that the operation was successful (true) or not (false) and /// an array that may or may not contain any errors produced during the push operation and an array of - /// non-blocking errors (warnings) that may have been generated during the push procedure. + /// non-blocking errors (warnings) that may have been generated during the push procedure. The + /// completion handler is called from a background thread. func pushTranslations(_ translations: [TXSourceString], configuration: TXPushConfiguration = TXPushConfiguration(), completionHandler: @escaping (Bool, [Error], [Error]) -> Void) { @@ -213,7 +215,7 @@ class NativeCore : TranslationProvider { /// /// - Parameter completionHandler: A callback to be called when force cache invalidation is /// complete with a boolean argument that informs the caller that the operation was successful (true) or - /// not (false). + /// not (false). The completion handler is called from a background thread. func forceCacheInvalidation(completionHandler: @escaping (Bool) -> Void) { cdsHandler.forceCacheInvalidation { [weak self] success in guard let _ = self else { @@ -580,6 +582,7 @@ token: \(token) /// - status: An optional status so that only strings matching translation status are fetched. /// - completionHandler: The completion handler that informs the caller when the operation /// is complete, reporting the new translations and a list of possible errors that might have occured. + /// The completion handler is called from a background thread. @objc public static func fetchTranslations(_ localeCode: String? = nil, tags: [String]? = nil, @@ -600,7 +603,8 @@ token: \(token) /// - completionHandler: A callback to be called when the push operation is complete with a /// boolean argument that informs the caller that the operation was successful (true) or not (false) and /// an array that may or may not contain any errors produced during the push operation and an array of - /// non-blocking errors (warnings) that may have been generated during the push procedure. + /// non-blocking errors (warnings) that may have been generated during the push procedure. The + /// completion handler is called from a background thread. @objc public static func pushTranslations(_ translations: [TXSourceString], configuration: TXPushConfiguration = TXPushConfiguration(), @@ -614,7 +618,7 @@ token: \(token) /// /// - Parameter completionHandler: A callback to be called when force cache invalidation is /// complete with a boolean argument that informs the caller that the operation was successful (true) or - /// not (false). + /// not (false). The completion handler is called from a background thread. @objc public static func forceCacheInvalidation(completionHandler: @escaping (Bool) -> Void) { tx?.forceCacheInvalidation(completionHandler: completionHandler) From 87e156807948e487baeb6788f6de898146b217e9 Mon Sep 17 00:00:00 2001 From: Stelios Petrakis Date: Wed, 3 Jul 2024 12:18:03 +0200 Subject: [PATCH 4/5] Resolve cache update issues after fetch translations After the translations are fetched (pulled) from CDS, the cache is now updated in the main thread and only if the translations structure is not empty. --- Sources/Transifex/Core.swift | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/Sources/Transifex/Core.swift b/Sources/Transifex/Core.swift index 292c6ed..04aee22 100644 --- a/Sources/Transifex/Core.swift +++ b/Sources/Transifex/Core.swift @@ -181,7 +181,21 @@ class NativeCore : TranslationProvider { Logger.error("\(#function) Errors: \(errors)") } - self.cache.update(translations: translations) + // Only update the cache if the translations structure is not + // empty, avoiding cases where no translations were fetched due + // to errors (e.g. network offline etc). + // We do not check for errors here as some translations might + // have failed and some others might not. As long as the + // translations structure is not empty, we are OK to update the + // cache (and, by extension, the stored file). + if !translations.isEmpty { + // Update cache using the fetched translations in main thread + // to ensure proper cache updates for custom implemented caching + // solutions. + DispatchQueue.main.async { + self.cache.update(translations: translations) + } + } completionHandler?(translations, errors) } From eabf479ec5d8b8583535b0f7a47f7b72c3797bb1 Mon Sep 17 00:00:00 2001 From: Stelios Petrakis Date: Wed, 3 Jul 2024 12:23:47 +0200 Subject: [PATCH 5/5] Bump version to 2.0.5 * Bumps `TXNative.version` to 2.0.5. * Updates CHANGELOG with the changes implemented for 2.0.5. --- CHANGELOG.md | 9 +++++++++ Sources/Transifex/Core.swift | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c1da063..784536b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -129,3 +129,12 @@ logic now normalizes the locale name to match the format that iOS accepts. *June 21, 2024* - Updates minimum supported OS versions. + +## Transifex iOS SDK 2.0.5 + +*July 3, 2024* + +* Ensures that callbacks won't capture `self` strongly. +* Ensures Designed for iPhone/iPad apps use the proper device name. +* Discloses that completion handlers are called from background threads. +* Improves cache update after a `fetchTranslations` call. diff --git a/Sources/Transifex/Core.swift b/Sources/Transifex/Core.swift index 04aee22..27fd12c 100644 --- a/Sources/Transifex/Core.swift +++ b/Sources/Transifex/Core.swift @@ -391,7 +391,7 @@ render '\(stringToRender)' locale code: \(localeCode) params: \(params). Error: /// A static class that is the main point of entry for all the functionality of Transifex Native throughout the SDK. public final class TXNative : NSObject { /// The SDK version - internal static let version = "2.0.4" + internal static let version = "2.0.5" /// The filename of the file that holds the translated strings and it's bundled inside the app. public static let STRINGS_FILENAME = "txstrings.json"