From 448fb930e869d8282566f5c1abfa147a1a6611e9 Mon Sep 17 00:00:00 2001 From: Fabrizio Demaria Date: Tue, 5 Nov 2024 11:09:12 +0100 Subject: [PATCH] fix: Fix warnings and prevent potential issues (#165) * fix: Safer data extraction from numerics * fix: Swift6 warning and potential deadlock * fix: Warnings in tests * fix: Linter check on data decoding * fix: Malformed events are ignored and then removed silently --- .../Confidence/ConfidenceScreenTracker.swift | 2 +- Sources/Confidence/ConfidenceValueHash.swift | 16 ++++++++++++- Sources/Confidence/EventSenderEngine.swift | 7 +++++- Sources/Confidence/EventStorage.swift | 23 +++++++++++-------- Sources/Confidence/Http/NetworkClient.swift | 4 ++-- .../ConfidenceProviderTest.swift | 2 +- Tests/ConfidenceTests/ConfidenceTest.swift | 2 +- .../ConfidenceValueTests.swift | 2 +- .../FlagApplierWithRetriesTest.swift | 20 ++++++++-------- .../ConfidenceTests/Helpers/StorageMock.swift | 2 +- 10 files changed, 51 insertions(+), 29 deletions(-) diff --git a/Sources/Confidence/ConfidenceScreenTracker.swift b/Sources/Confidence/ConfidenceScreenTracker.swift index 081342f8..a62cb30b 100644 --- a/Sources/Confidence/ConfidenceScreenTracker.swift +++ b/Sources/Confidence/ConfidenceScreenTracker.swift @@ -78,7 +78,7 @@ extension UIViewController { let encoder = JSONEncoder() do { let data = try encoder.encode(trackableWithMessage.trackMessage()) - let messageString = String(decoding: data, as: UTF8.self) + let messageString = String(data: data, encoding: .utf8) ?? "internal_error" message.updateValue(messageString, forKey: ConfidenceScreenTracker.messageKey) } catch { } diff --git a/Sources/Confidence/ConfidenceValueHash.swift b/Sources/Confidence/ConfidenceValueHash.swift index 214e260b..c1817c26 100644 --- a/Sources/Confidence/ConfidenceValueHash.swift +++ b/Sources/Confidence/ConfidenceValueHash.swift @@ -73,7 +73,21 @@ extension StringProtocol { var data: Data { .init(utf8) } } -extension Numeric { +extension FixedWidthInteger { + var data: Data { + var source = self + return .init(bytes: &source, count: MemoryLayout.size) + } +} + +extension Float { + var data: Data { + var source = self + return .init(bytes: &source, count: MemoryLayout.size) + } +} + +extension Double { var data: Data { var source = self return .init(bytes: &source, count: MemoryLayout.size) diff --git a/Sources/Confidence/EventSenderEngine.swift b/Sources/Confidence/EventSenderEngine.swift index dff0b232..9d0d8ca8 100644 --- a/Sources/Confidence/EventSenderEngine.swift +++ b/Sources/Confidence/EventSenderEngine.swift @@ -123,7 +123,12 @@ final class EventSenderEngineImpl: EventSenderEngine { } func withSemaphore(callback: @escaping () async -> Void) async { - semaphore.wait() + await withCheckedContinuation { continuation in + DispatchQueue.global().async { + self.semaphore.wait() + continuation.resume() + } + } await callback() semaphore.signal() } diff --git a/Sources/Confidence/EventStorage.swift b/Sources/Confidence/EventStorage.swift index 5d6591c3..673ae051 100644 --- a/Sources/Confidence/EventStorage.swift +++ b/Sources/Confidence/EventStorage.swift @@ -75,17 +75,20 @@ internal class EventStorageImpl: EventStorage { let decoder = JSONDecoder() let fileUrl = folderURL.appendingPathComponent(id) let data = try Data(contentsOf: fileUrl) - let dataString = String(decoding: data, as: UTF8.self) - return try dataString.components(separatedBy: "\n") - .filter { events in - !events.isEmpty - } - .compactMap { eventString in - guard let stringData = eventString.data(using: .utf8) else { - return nil + if let dataString = String(data: data, encoding: .utf8) { + return try dataString.components(separatedBy: "\n") + .filter { events in + !events.isEmpty } - return try decoder.decode(ConfidenceEvent.self, from: stringData) - } + .compactMap { eventString in + guard let stringData = eventString.data(using: .utf8) else { + return nil + } + return try decoder.decode(ConfidenceEvent.self, from: stringData) + } + } else { + return [] + } } } diff --git a/Sources/Confidence/Http/NetworkClient.swift b/Sources/Confidence/Http/NetworkClient.swift index 0ce563e7..92f71e46 100644 --- a/Sources/Confidence/Http/NetworkClient.swift +++ b/Sources/Confidence/Http/NetworkClient.swift @@ -134,10 +134,10 @@ extension NetworkClient { do { response.decodedError = try decoder.decode(HttpError.self, from: responseData) } catch { - let message = String(decoding: responseData, as: UTF8.self) + let message = String(data: responseData, encoding: .utf8) response.decodedError = HttpError( code: httpURLResponse.statusCode, - message: message, + message: message ?? "{Error when decoding error message}", details: [] ) } diff --git a/Tests/ConfidenceProviderTests/ConfidenceProviderTest.swift b/Tests/ConfidenceProviderTests/ConfidenceProviderTest.swift index d4dfb242..4271a091 100644 --- a/Tests/ConfidenceProviderTests/ConfidenceProviderTest.swift +++ b/Tests/ConfidenceProviderTests/ConfidenceProviderTest.swift @@ -139,7 +139,7 @@ private class StorageMock: Storage { func save(data: Encodable) throws { try storageQueue.sync { let dataB = try JSONEncoder().encode(data) - self.data = String(decoding: dataB, as: UTF8.self) + self.data = try XCTUnwrap(String(data: dataB, encoding: .utf8)) saveExpectation?.fulfill() } diff --git a/Tests/ConfidenceTests/ConfidenceTest.swift b/Tests/ConfidenceTests/ConfidenceTest.swift index e26d7d3a..af1c3c3d 100644 --- a/Tests/ConfidenceTests/ConfidenceTest.swift +++ b/Tests/ConfidenceTests/ConfidenceTest.swift @@ -233,7 +233,7 @@ class ConfidenceTest: XCTestCase { .build() try await confidence.fetchAndActivate() - let value = try confidence.getValue( + let value = confidence.getValue( key: "flag.size", defaultValue: 0 as Int64) diff --git a/Tests/ConfidenceTests/ConfidenceValueTests.swift b/Tests/ConfidenceTests/ConfidenceValueTests.swift index 645e1f6b..902f50a2 100644 --- a/Tests/ConfidenceTests/ConfidenceValueTests.swift +++ b/Tests/ConfidenceTests/ConfidenceValueTests.swift @@ -125,7 +125,7 @@ final class ConfidenceConfidenceValueTests: XCTestCase { let encoder = JSONEncoder() encoder.outputFormatting = .sortedKeys let data = try encoder.encode(value) - let resultString = try XCTUnwrap(String(decoding: data, as: UTF8.self)) + let resultString = try XCTUnwrap(String(data: data, encoding: .utf8)) let resultData = try XCTUnwrap(resultString.data(using: .utf8)) let decodedValue = try JSONDecoder().decode(ConfidenceValue.self, from: resultData) diff --git a/Tests/ConfidenceTests/FlagApplierWithRetriesTest.swift b/Tests/ConfidenceTests/FlagApplierWithRetriesTest.swift index 6ff64d34..4c9d7784 100644 --- a/Tests/ConfidenceTests/FlagApplierWithRetriesTest.swift +++ b/Tests/ConfidenceTests/FlagApplierWithRetriesTest.swift @@ -99,7 +99,7 @@ class FlagApplierWithRetriesTest: XCTestCase { await applier.apply(flagName: "flag2", resolveToken: "token1") await applier.apply(flagName: "flag3", resolveToken: "token1") - await waitForExpectations(timeout: 1.0) + await fulfillment(of: [networkExpectation], timeout: 1.0) // Then cache data is not stored on to the disk // But stored in the local cache as sent @@ -152,7 +152,7 @@ class FlagApplierWithRetriesTest: XCTestCase { metadata: metadata ) - await waitForExpectations(timeout: 5.0) + await fulfillment(of: [storageExpectation, expectation], timeout: 1.0) // Then http client sends 5 apply flag batch request, containing 20 records each let request = try XCTUnwrap(httpClient.data?.first as? ApplyFlagsRequest) @@ -184,7 +184,7 @@ class FlagApplierWithRetriesTest: XCTestCase { metadata: metadata ) - await waitForExpectations(timeout: 5.0) + await fulfillment(of: [storageExpectation, expectation], timeout: 1.0) // Then http client sends 5 apply flag batch request, containing 20 records each let request = try XCTUnwrap(httpClient.data?.first as? ApplyFlagsRequest) @@ -215,7 +215,7 @@ class FlagApplierWithRetriesTest: XCTestCase { metadata: metadata ) - await waitForExpectations(timeout: 5.0) + await fulfillment(of: [storageExpectation, expectation], timeout: 1.0) // Then http client sends 5 apply flags batch request, containing 20 records each let request = try XCTUnwrap(partiallyFailingHttpClient.data?.first as? ApplyFlagsRequest) @@ -254,7 +254,7 @@ class FlagApplierWithRetriesTest: XCTestCase { httpClient.testMode = .success await applier.apply(flagName: "flag2", resolveToken: "token1") - await waitForExpectations(timeout: 1.0) + await fulfillment(of: [networkExpectation], timeout: 1.0) // Then 3 post calls are issued (one offline, one batch apply containing 2 reconrds) XCTAssertEqual(httpClient.postCallCounter, 2) @@ -298,7 +298,7 @@ class FlagApplierWithRetriesTest: XCTestCase { // With test mode .success offlineClient.testMode = .success await applier.apply(flagName: "flag2", resolveToken: "token1") - await waitForExpectations(timeout: 1.0) + await fulfillment(of: [networkExpectation, storageExpectation], timeout: 1.0) // Then both requests are marked as sent in cache data let cacheData = await cacheDataInteractor.cache @@ -332,7 +332,7 @@ class FlagApplierWithRetriesTest: XCTestCase { metadata: metadata ) - await waitForExpectations(timeout: 1.0) + await fulfillment(of: [storageExpectation, networkExpectation], timeout: 1.0) // Then storage has been cleaned let storedData = try prefilledStorage.load(defaultValue: CacheData.empty()) @@ -361,7 +361,7 @@ class FlagApplierWithRetriesTest: XCTestCase { // When 100 apply calls are issued // And all http client requests fails with .invalidResponse await hundredApplyCalls(applier: applier, sameToken: true) - await waitForExpectations(timeout: 1.0) + await fulfillment(of: [networkExpectation], timeout: 1.0) // Then strored data is empty let storedData: CacheData = try XCTUnwrap(prefilledStorage.load(defaultValue: CacheData.empty())) @@ -471,7 +471,7 @@ class FlagApplierWithRetriesTest: XCTestCase { // And http client request fails with .invalidResponse await applier.apply(flagName: "flag1", resolveToken: "token1") - await waitForExpectations(timeout: 5.0) + await fulfillment(of: [networkExpectation], timeout: 1.0) // Then 2 resolve event records are stored on disk let storedData: CacheData = try XCTUnwrap(prefilledStorage.load(defaultValue: CacheData.empty())) @@ -535,7 +535,7 @@ class FlagApplierWithRetriesTest: XCTestCase { // When 100 apply calls are issued // And all http client requests fails with .invalidResponse await hundredApplyCalls(applier: applier, sameToken: true) - await waitForExpectations(timeout: 1.0) + await fulfillment(of: [networkExpectation], timeout: 1.0) // Then 1 resolve event record is stored on disk // And 200 flag event records are stored on disk diff --git a/Tests/ConfidenceTests/Helpers/StorageMock.swift b/Tests/ConfidenceTests/Helpers/StorageMock.swift index e36c0498..a3708ed9 100644 --- a/Tests/ConfidenceTests/Helpers/StorageMock.swift +++ b/Tests/ConfidenceTests/Helpers/StorageMock.swift @@ -17,7 +17,7 @@ class StorageMock: Storage { func save(data: Encodable) throws { try storageQueue.sync { let dataB = try JSONEncoder().encode(data) - self.data = String(decoding: dataB, as: UTF8.self) + self.data = try XCTUnwrap(String(data: dataB, encoding: .utf8)) saveExpectation?.fulfill() }