Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Fix warnings and prevent potential issues #165

Merged
merged 5 commits into from
Nov 5, 2024
Merged

fix: Fix warnings and prevent potential issues #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b785afd
fix: Safer data extraction from numerics
fabriziodemaria Nov 4, 2024
bb566a3
fix: Swift6 warning and potential deadlock
fabriziodemaria Nov 4, 2024
3bc9a04
fix: Warnings in tests
fabriziodemaria Nov 4, 2024
136e5a8
fix: Linter check on data decoding
fabriziodemaria Nov 4, 2024
b5d1396
fix: Malformed events are ignored and then removed silently
fabriziodemaria Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Sources/Confidence/ConfidenceScreenTracker.swift
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fully sure if this is a no-op change or not?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should behave the same way, just removes a warning (and apparently being a safer approach against deadlocks)

DispatchQueue.global().async {
self.semaphore.wait()
continuation.resume()
}
}
await callback()
semaphore.signal()
}
Expand Down
23 changes: 13 additions & 10 deletions Sources/Confidence/EventStorage.swift
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
Loading