Skip to content

Commit

Permalink
fix: Fix warnings and prevent potential issues (#165)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
fabriziodemaria authored Nov 5, 2024
1 parent bbe7e40 commit 448fb93
Show file tree
Hide file tree
Showing 10 changed files with 51 additions and 29 deletions.
2 changes: 1 addition & 1 deletion Sources/Confidence/ConfidenceScreenTracker.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
}
Expand Down
16 changes: 15 additions & 1 deletion Sources/Confidence/ConfidenceValueHash.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self>.size)
}
}

extension Float {
var data: Data {
var source = self
return .init(bytes: &source, count: MemoryLayout<Self>.size)
}
}

extension Double {
var data: Data {
var source = self
return .init(bytes: &source, count: MemoryLayout<Self>.size)
Expand Down
7 changes: 6 additions & 1 deletion Sources/Confidence/EventSenderEngine.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
23 changes: 13 additions & 10 deletions Sources/Confidence/EventStorage.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 []
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions Sources/Confidence/Http/NetworkClient.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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: []
)
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/ConfidenceProviderTests/ConfidenceProviderTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
2 changes: 1 addition & 1 deletion Tests/ConfidenceTests/ConfidenceTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion Tests/ConfidenceTests/ConfidenceValueTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
20 changes: 10 additions & 10 deletions Tests/ConfidenceTests/FlagApplierWithRetriesTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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()))
Expand Down Expand Up @@ -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()))
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Tests/ConfidenceTests/Helpers/StorageMock.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down

0 comments on commit 448fb93

Please sign in to comment.