From 8b915f60580373a8831775215ef54a7f43fccab4 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Fri, 23 Jun 2023 15:35:41 -0400 Subject: [PATCH 1/4] Adding check to only save datafile in cache if valid. --- .../DefaultDatafileHandler.swift | 18 ++++-- .../DatafileHandlerTests.swift | 60 ++++++++++++++++++- .../DatafileHandlerTests_MultiClients.swift | 8 +-- 3 files changed, 75 insertions(+), 11 deletions(-) diff --git a/Sources/Customization/DefaultDatafileHandler.swift b/Sources/Customization/DefaultDatafileHandler.swift index 3a9963a32..362ec2a94 100644 --- a/Sources/Customization/DefaultDatafileHandler.swift +++ b/Sources/Customization/DefaultDatafileHandler.swift @@ -1,5 +1,5 @@ // -// Copyright 2019-2022, Optimizely, Inc. and contributors +// Copyright 2019-2023, Optimizely, Inc. and contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -158,12 +158,18 @@ open class DefaultDatafileHandler: OPTDatafileHandler { open func getResponseData(sdkKey: String, response: HTTPURLResponse, url: URL?) -> Data? { if let url = url, let data = try? Data(contentsOf: url) { self.logger.d { String(data: data, encoding: .utf8) ?? "" } - self.saveDatafile(sdkKey: sdkKey, dataFile: data) - if let lastModified = response.getLastModified() { - self.sharedDataStore.setLastModified(sdkKey: sdkKey, lastModified: lastModified) + // Check datafile is valid json + do { + // Try deserializing datafile, isValidJSONObject is not applicable here + _ = try JSONSerialization.jsonObject(with: data) + self.saveDatafile(sdkKey: sdkKey, dataFile: data) + if let lastModified = response.getLastModified() { + self.sharedDataStore.setLastModified(sdkKey: sdkKey, lastModified: lastModified) + } + return data + } catch { + print("Error deserializing datafile: \(error.localizedDescription)") } - - return data } return nil diff --git a/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift b/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift index 70b3c9073..331c7756a 100644 --- a/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift +++ b/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift @@ -1,5 +1,5 @@ // -// Copyright 2019, 2021, Optimizely, Inc. and contributors +// Copyright 2019, 2021, 2023, Optimizely, Inc. and contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -65,7 +65,65 @@ class DatafileHandlerTests: XCTestCase { // This is an example of a functional test case. // Use XCTAssert and related functions to verify your tests produce the correct results. } + + func testDatafileDownloadWith200AndValidDatafile() { + // Datafile and last updated should not be saved in this case + let handler = MockDatafileHandler(statusCode: 200, localResponseData:"{}") + let expectation = XCTestExpectation(description: "no-nil data") + + handler.downloadDatafile(sdkKey: sdkKey) { (result) in + if case let .success(data) = result { + XCTAssert(data != nil) + expectation.fulfill() + } + } + + wait(for: [expectation], timeout: 3) + XCTAssertTrue(handler.isDatafileSaved(sdkKey: sdkKey)) + XCTAssertNotNil(handler.sharedDataStore.getLastModified(sdkKey: sdkKey)) + } + + func testDatafileDownloadWith200AndInvalidDatafile() { + // Datafile and last updated should not be saved in this case + let handler = MockDatafileHandler(statusCode: 200, localResponseData: "1231") + let expectation = XCTestExpectation(description: "wait to get failure") + + handler.downloadDatafile(sdkKey: sdkKey) { (result) in + if case .success(_) = result { + XCTFail() + } + if case .failure(_) = result { + expectation.fulfill() + } + } + + wait(for: [expectation], timeout: 3) + XCTAssertFalse(handler.isDatafileSaved(sdkKey: sdkKey)) + XCTAssertNil(handler.sharedDataStore.getLastModified(sdkKey: sdkKey)) + } + + func testStartWith200AndInvalidDatafile() { + let handler = MockDatafileHandler(statusCode: 200, localResponseData: "1231") + let expectation = XCTestExpectation(description: "wait to get failure") + + let optimizely = OptimizelyClient(sdkKey: sdkKey, + datafileHandler: handler, + periodicDownloadInterval: 1) + + optimizely.start(completion: { result in + if case let .failure(error) = result { + print(error) + XCTAssert(true) + expectation.fulfill() + } + }) + + wait(for: [expectation], timeout: 3) + XCTAssertFalse(handler.isDatafileSaved(sdkKey: sdkKey)) + XCTAssertNil(handler.sharedDataStore.getLastModified(sdkKey: sdkKey)) + } + func testDatafileDownload500() { OTUtils.createDatafileCache(sdkKey: sdkKey) diff --git a/Tests/OptimizelyTests-MultiClients/DatafileHandlerTests_MultiClients.swift b/Tests/OptimizelyTests-MultiClients/DatafileHandlerTests_MultiClients.swift index f0bb02cdf..c5ee64a96 100644 --- a/Tests/OptimizelyTests-MultiClients/DatafileHandlerTests_MultiClients.swift +++ b/Tests/OptimizelyTests-MultiClients/DatafileHandlerTests_MultiClients.swift @@ -1,5 +1,5 @@ // -// Copyright 2021, Optimizely, Inc. and contributors +// Copyright 2021, 2023, Optimizely, Inc. and contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -39,7 +39,7 @@ class DatafileHandlerTests_MultiClients: XCTestCase { func testConcurrentDownloadDatafiles() { // use a shared DatafileHandler instance - let mockHandler = MockDatafileHandler(statusCode: 200) + let mockHandler = MockDatafileHandler(statusCode: 200, localResponseData: "{}") sdkKeys = OTUtils.makeRandomSdkKeys(100) @@ -153,7 +153,7 @@ class DatafileHandlerTests_MultiClients: XCTestCase { func testConcurrentAccessLastModified() { // use a shared DatafileHandler instance - let mockHandler = MockDatafileHandler(statusCode: 200) + let mockHandler = MockDatafileHandler(statusCode: 200, localResponseData: "{}") sdkKeys = OTUtils.makeRandomSdkKeys(100) @@ -241,7 +241,7 @@ class DatafileHandlerTests_MultiClients: XCTestCase { } // use a shared DatafileHandler instance - let mockHandler = MockDatafileHandler(statusCode: 200) + let mockHandler = MockDatafileHandler(statusCode: 200, localResponseData: "{}") let numSdks = 50 sdkKeys = OTUtils.makeRandomSdkKeys(numSdks) From 14aefaff4d47d3adbdd8f3a217886f3cb3adfb87 Mon Sep 17 00:00:00 2001 From: Yasir Folio3 <39988750+yasirfolio3@users.noreply.github.com> Date: Fri, 23 Jun 2023 16:15:05 -0400 Subject: [PATCH 2/4] Update Sources/Customization/DefaultDatafileHandler.swift Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com> --- Sources/Customization/DefaultDatafileHandler.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Customization/DefaultDatafileHandler.swift b/Sources/Customization/DefaultDatafileHandler.swift index 362ec2a94..1fd50548c 100644 --- a/Sources/Customization/DefaultDatafileHandler.swift +++ b/Sources/Customization/DefaultDatafileHandler.swift @@ -168,7 +168,7 @@ open class DefaultDatafileHandler: OPTDatafileHandler { } return data } catch { - print("Error deserializing datafile: \(error.localizedDescription)") + self.logger.w("Error deserializing datafile: \(error.localizedDescription)") } } From a9728614c901c9afab9f8c82f3960324a2fa7c23 Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Mon, 26 Jun 2023 11:16:35 -0400 Subject: [PATCH 3/4] fixing unit tests. --- Tests/OptimizelyTests-Common/DatafileHandlerTests.swift | 6 +++--- .../OptimizelyTests-Common/NetworkReachabilityTests.swift | 8 ++++---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift b/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift index 331c7756a..6ec021951 100644 --- a/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift +++ b/Tests/OptimizelyTests-Common/DatafileHandlerTests.swift @@ -242,7 +242,7 @@ class DatafileHandlerTests: XCTestCase { func testPeriodicDownload() { let expection = XCTestExpectation(description: "Expect 10 periodic downloads") - let handler = MockDatafileHandler(statusCode: 200) + let handler = MockDatafileHandler(statusCode: 200, localResponseData: "{}") let now = Date() var count = 0 var seconds = 0 @@ -263,7 +263,7 @@ class DatafileHandlerTests: XCTestCase { func testPeriodicDownload_PollingShouldNotBeAccumulatedWhileInBackground() { let expectation = XCTestExpectation(description: "polling") - let handler = MockDatafileHandler(statusCode: 200) + let handler = MockDatafileHandler(statusCode: 200, localResponseData: "{}") let now = Date() let updateInterval = 1 @@ -394,7 +394,7 @@ class DatafileHandlerTests: XCTestCase { // will return 500 return MockUrlSession(withError: true) } else { - return MockUrlSession(statusCode: 200) + return MockUrlSession(statusCode: 200, localResponseData: "{}") } } } diff --git a/Tests/OptimizelyTests-Common/NetworkReachabilityTests.swift b/Tests/OptimizelyTests-Common/NetworkReachabilityTests.swift index 5c45e400e..cc6fb7a9a 100644 --- a/Tests/OptimizelyTests-Common/NetworkReachabilityTests.swift +++ b/Tests/OptimizelyTests-Common/NetworkReachabilityTests.swift @@ -1,5 +1,5 @@ // -// Copyright 2021, Optimizely, Inc. and contributors +// Copyright 2021, 2023 Optimizely, Inc. and contributors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -119,7 +119,7 @@ class NetworkReachabilityTests: XCTestCase { } func testFetchDatafile_numContiguousFails() { - let handler = MockDatafileHandler(withError: true) + let handler = MockDatafileHandler(withError: true, localResponseData: "{}") let reachability = handler.reachability var exp = expectation(description: "r") @@ -164,7 +164,7 @@ class NetworkReachabilityTests: XCTestCase { } func testFetchDatafile_checkReachability() { - let handler = MockDatafileHandler(withError: true) + let handler = MockDatafileHandler(withError: true, localResponseData: "{}") let reachability = handler.reachability reachability.stop() @@ -196,7 +196,7 @@ class NetworkReachabilityTests: XCTestCase { // no valid datafile cache - let handler = MockDatafileHandler(withError: true) + let handler = MockDatafileHandler(withError: true, localResponseData: "{}") let reachability = handler.reachability reachability.stop() From b4ec572c5737b93f066e77d7b8f4a83513652e1d Mon Sep 17 00:00:00 2001 From: Yasir Ali Date: Mon, 26 Jun 2023 14:45:48 -0400 Subject: [PATCH 4/4] Fixing tests. --- Tests/TestUtils/MockUrlSession.swift | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/Tests/TestUtils/MockUrlSession.swift b/Tests/TestUtils/MockUrlSession.swift index c8c498a5d..8845ff159 100644 --- a/Tests/TestUtils/MockUrlSession.swift +++ b/Tests/TestUtils/MockUrlSession.swift @@ -28,6 +28,7 @@ class MockUrlSession: URLSession { var localResponseData: String? var settingsMap: [String: (Int, Bool)]? var handler: MockDatafileHandler? + let lock = DispatchQueue(label: "mock-session-lock") class MockDownloadTask: URLSessionDownloadTask { var task: () -> Void @@ -54,7 +55,9 @@ class MockUrlSession: URLSession { } init(handler: MockDatafileHandler? = nil, statusCode: Int = 0, withError: Bool = false, localResponseData: String? = nil) { - Self.validSessions += 1 + lock.async { + Self.validSessions += 1 + } self.handler = handler self.statusCode = statusCode self.withError = withError @@ -62,7 +65,9 @@ class MockUrlSession: URLSession { } init(handler: MockDatafileHandler? = nil, settingsMap: [String: (Int, Bool)]) { - Self.validSessions += 1 + lock.async { + Self.validSessions += 1 + } self.handler = handler self.statusCode = 0 self.withError = false @@ -113,6 +118,8 @@ class MockUrlSession: URLSession { } override func finishTasksAndInvalidate() { - Self.validSessions -= 1 + lock.async { + Self.validSessions -= 1 + } } }