From b131b28c73709e7d191df9fc1fb7ab1fa5f499e6 Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Thu, 18 Apr 2024 11:44:47 +0200 Subject: [PATCH 01/10] Uze gzip compression for sync PATCH payloads --- Package.resolved | 9 +++++++++ Package.swift | 2 ++ Sources/DDGSync/internal/SyncRequestMaker.swift | 7 +++++-- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/Package.resolved b/Package.resolved index 491a98c2c..5cd7d361c 100644 --- a/Package.resolved +++ b/Package.resolved @@ -36,6 +36,15 @@ "version" : "2.3.0" } }, + { + "identity" : "gzipswift", + "kind" : "remoteSourceControl", + "location" : "https://github.com/1024jp/GzipSwift.git", + "state" : { + "revision" : "731037f6cc2be2ec01562f6597c1d0aa3fe6fd05", + "version" : "6.0.1" + } + }, { "identity" : "privacy-dashboard", "kind" : "remoteSourceControl", diff --git a/Package.swift b/Package.swift index b518c1e4f..c3643d3f6 100644 --- a/Package.swift +++ b/Package.swift @@ -48,6 +48,7 @@ let package = Package( .package(url: "https://github.com/httpswift/swifter.git", exact: "1.5.0"), .package(url: "https://github.com/duckduckgo/bloom_cpp.git", exact: "3.0.0"), .package(url: "https://github.com/duckduckgo/wireguard-apple", exact: "1.1.3"), + .package(url: "https://github.com/1024jp/GzipSwift.git", exact: "6.0.1") ], targets: [ .target( @@ -152,6 +153,7 @@ let package = Package( "BrowserServicesKit", "Common", .product(name: "DDGSyncCrypto", package: "sync_crypto"), + .product(name: "Gzip", package: "GzipSwift"), "Networking", ], resources: [ diff --git a/Sources/DDGSync/internal/SyncRequestMaker.swift b/Sources/DDGSync/internal/SyncRequestMaker.swift index 12382e667..cb785924b 100644 --- a/Sources/DDGSync/internal/SyncRequestMaker.swift +++ b/Sources/DDGSync/internal/SyncRequestMaker.swift @@ -17,6 +17,7 @@ // import Foundation +import Gzip protocol SyncRequestMaking { func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting @@ -54,8 +55,10 @@ struct SyncRequestMaker: SyncRequestMaking { throw SyncError.unableToEncodeRequestBody("Sync PATCH payload is not a valid JSON") } - let body = try JSONSerialization.data(withJSONObject: json, options: []) - return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body) + let headers = ["Content-Encoding": "gzip"] + + let body = try JSONSerialization.data(withJSONObject: json, options: []).gzipped() + return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body, headers: headers) } private func getToken() throws -> String { From cd4df4ab31844e595c633bf5bf6a2971c9abeffd Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Tue, 30 Apr 2024 09:22:37 +0200 Subject: [PATCH 02/10] Fix unit tests --- Tests/DDGSyncTests/SyncOperationTests.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tests/DDGSyncTests/SyncOperationTests.swift b/Tests/DDGSyncTests/SyncOperationTests.swift index e76df782d..fc128b840 100644 --- a/Tests/DDGSyncTests/SyncOperationTests.swift +++ b/Tests/DDGSyncTests/SyncOperationTests.swift @@ -167,16 +167,16 @@ class SyncOperationTests: XCTestCase { for body in bodies.compactMap({$0}) { do { - let payload = try JSONDecoder.snakeCaseKeys.decode(BookmarksPayload.self, from: body) + let payload = try JSONDecoder.snakeCaseKeys.decode(BookmarksPayload.self, from: body.gunzipped()) XCTAssertEqual(payload, bookmarks) payloadCount -= 1 } catch { do { - let payload = try JSONDecoder.snakeCaseKeys.decode(SettingsPayload.self, from: body) + let payload = try JSONDecoder.snakeCaseKeys.decode(SettingsPayload.self, from: body.gunzipped()) XCTAssertEqual(payload, settings) payloadCount -= 1 } catch { - let payload = try JSONDecoder.snakeCaseKeys.decode(AutofillPayload.self, from: body) + let payload = try JSONDecoder.snakeCaseKeys.decode(AutofillPayload.self, from: body.gunzipped()) XCTAssertEqual(payload, autofill) payloadCount -= 1 } From 16d78ccabbafb5d902e8bce6ffb56fe9e67e439f Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Sun, 5 May 2024 22:25:46 +0200 Subject: [PATCH 03/10] Skip PATCH payload compression if it throws an error and report that error to data provider --- Sources/DDGSync/internal/SyncOperation.swift | 12 +++++++++--- Sources/DDGSync/internal/SyncRequestMaker.swift | 10 +++++++--- 2 files changed, 16 insertions(+), 6 deletions(-) diff --git a/Sources/DDGSync/internal/SyncOperation.swift b/Sources/DDGSync/internal/SyncOperation.swift index b4e84e756..2e4777d06 100644 --- a/Sources/DDGSync/internal/SyncOperation.swift +++ b/Sources/DDGSync/internal/SyncOperation.swift @@ -19,6 +19,7 @@ import Foundation import Combine import Common +import Gzip final class SyncOperation: Operation { @@ -143,7 +144,7 @@ final class SyncOperation: Operation { try checkCancellation() let syncRequest = try await self.makeSyncRequest(for: dataProvider, fetchOnly: fetchOnly) let clientTimestamp = Date() - let httpRequest = try self.makeHTTPRequest(with: syncRequest, timestamp: clientTimestamp) + let httpRequest = try self.makeHTTPRequest(for: dataProvider, with: syncRequest, timestamp: clientTimestamp) try checkCancellation() let httpResult: HTTPResult = try await httpRequest.execute() @@ -211,10 +212,15 @@ final class SyncOperation: Operation { return SyncRequest(feature: dataProvider.feature, previousSyncTimestamp: dataProvider.lastSyncTimestamp, sent: localChanges) } - private func makeHTTPRequest(with syncRequest: SyncRequest, timestamp: Date) throws -> HTTPRequesting { + private func makeHTTPRequest(for dataProvider: DataProviding, with syncRequest: SyncRequest, timestamp: Date) throws -> HTTPRequesting { let hasLocalChanges = !syncRequest.sent.isEmpty if hasLocalChanges { - return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp) + do { + return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: true) + } catch let error as GzipError { + dataProvider.handleSyncError(error) + return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: false) + } } return try requestMaker.makeGetRequest(with: syncRequest) } diff --git a/Sources/DDGSync/internal/SyncRequestMaker.swift b/Sources/DDGSync/internal/SyncRequestMaker.swift index cb785924b..dd78fd5aa 100644 --- a/Sources/DDGSync/internal/SyncRequestMaker.swift +++ b/Sources/DDGSync/internal/SyncRequestMaker.swift @@ -21,7 +21,7 @@ import Gzip protocol SyncRequestMaking { func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting - func makePatchRequest(with result: SyncRequest, clientTimestamp: Date) throws -> HTTPRequesting + func makePatchRequest(with result: SyncRequest, clientTimestamp: Date, isCompressed: Bool) throws -> HTTPRequesting } struct SyncRequestMaker: SyncRequestMaking { @@ -42,7 +42,7 @@ struct SyncRequestMaker: SyncRequestMaking { return api.createAuthenticatedGetRequest(url: url, authToken: try getToken(), parameters: parameters) } - func makePatchRequest(with result: SyncRequest, clientTimestamp: Date) throws -> HTTPRequesting { + func makePatchRequest(with result: SyncRequest, clientTimestamp: Date, isCompressed: Bool) throws -> HTTPRequesting { var json = [String: Any]() let modelPayload: [String: Any?] = [ "updates": result.sent.map(\.payload), @@ -55,8 +55,12 @@ struct SyncRequestMaker: SyncRequestMaking { throw SyncError.unableToEncodeRequestBody("Sync PATCH payload is not a valid JSON") } - let headers = ["Content-Encoding": "gzip"] + guard isCompressed else { + let body = try JSONSerialization.data(withJSONObject: json, options: []) + return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body) + } + let headers = ["Content-Encoding": "gzip"] let body = try JSONSerialization.data(withJSONObject: json, options: []).gzipped() return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body, headers: headers) } From bf18c542f0851a33b318e0f16fe08415e1311f5e Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Mon, 6 May 2024 13:04:16 +0200 Subject: [PATCH 04/10] Add SyncPayloadCompressing protocol and add unit tests for payload compression --- .../internal/ProductionDependencies.swift | 2 + .../DDGSync/internal/SyncDependencies.swift | 1 + .../internal/SyncPayloadCompressor.swift | 31 +++++++++ Sources/DDGSync/internal/SyncQueue.swift | 4 +- .../DDGSync/internal/SyncRequestMaker.swift | 14 ++-- Tests/DDGSyncTests/Mocks/Mocks.swift | 41 ++++++++++++ Tests/DDGSyncTests/SyncOperationTests.swift | 65 ++++++++++++++++++- Tests/DDGSyncTests/SyncQueueTests.swift | 31 +++++++-- 8 files changed, 178 insertions(+), 11 deletions(-) create mode 100644 Sources/DDGSync/internal/SyncPayloadCompressor.swift diff --git a/Sources/DDGSync/internal/ProductionDependencies.swift b/Sources/DDGSync/internal/ProductionDependencies.swift index 66724039e..d9a3241e8 100644 --- a/Sources/DDGSync/internal/ProductionDependencies.swift +++ b/Sources/DDGSync/internal/ProductionDependencies.swift @@ -27,6 +27,7 @@ struct ProductionDependencies: SyncDependencies { let endpoints: Endpoints let account: AccountManaging let api: RemoteAPIRequestCreating + let payloadCompressor: SyncPayloadCompressing var keyValueStore: KeyValueStoring let secureStore: SecureStoring let crypter: CryptingInternal @@ -72,6 +73,7 @@ struct ProductionDependencies: SyncDependencies { self.getLog = log api = RemoteAPIRequestCreator(log: log()) + payloadCompressor = SyncPayloadCompressor() crypter = Crypter(secureStore: secureStore) account = AccountManager(endpoints: endpoints, api: api, crypter: crypter) diff --git a/Sources/DDGSync/internal/SyncDependencies.swift b/Sources/DDGSync/internal/SyncDependencies.swift index 6e5740c0d..06c1bd390 100644 --- a/Sources/DDGSync/internal/SyncDependencies.swift +++ b/Sources/DDGSync/internal/SyncDependencies.swift @@ -31,6 +31,7 @@ protocol SyncDependencies: SyncDependenciesDebuggingSupport { var endpoints: Endpoints { get } var account: AccountManaging { get } var api: RemoteAPIRequestCreating { get } + var payloadCompressor: SyncPayloadCompressing { get } var keyValueStore: KeyValueStoring { get } var secureStore: SecureStoring { get } var crypter: CryptingInternal { get } diff --git a/Sources/DDGSync/internal/SyncPayloadCompressor.swift b/Sources/DDGSync/internal/SyncPayloadCompressor.swift new file mode 100644 index 000000000..9642e4251 --- /dev/null +++ b/Sources/DDGSync/internal/SyncPayloadCompressor.swift @@ -0,0 +1,31 @@ +// +// SyncPayloadCompressor.swift +// DuckDuckGo +// +// Copyright © 2024 DuckDuckGo. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +// + +import Foundation +import Gzip + +protocol SyncPayloadCompressing { + func compress(_ payload: Data) throws -> Data +} + +struct SyncPayloadCompressor: SyncPayloadCompressing { + func compress(_ payload: Data) throws -> Data { + try payload.gzipped() + } +} diff --git a/Sources/DDGSync/internal/SyncQueue.swift b/Sources/DDGSync/internal/SyncQueue.swift index fb6e4d22f..9bd4b9042 100644 --- a/Sources/DDGSync/internal/SyncQueue.swift +++ b/Sources/DDGSync/internal/SyncQueue.swift @@ -69,6 +69,7 @@ final class SyncQueue { crypter: dependencies.crypter, api: dependencies.api, endpoints: dependencies.endpoints, + payloadCompressor: dependencies.payloadCompressor, log: dependencies.log ) } @@ -79,13 +80,14 @@ final class SyncQueue { crypter: Crypting, api: RemoteAPIRequestCreating, endpoints: Endpoints, + payloadCompressor: SyncPayloadCompressing, log: @escaping @autoclosure () -> OSLog = .disabled ) { self.dataProviders = dataProviders self.storage = storage self.crypter = crypter self.getLog = log - requestMaker = SyncRequestMaker(storage: storage, api: api, endpoints: endpoints) + requestMaker = SyncRequestMaker(storage: storage, api: api, endpoints: endpoints, payloadCompressor: payloadCompressor) syncDidFinishPublisher = syncDidFinishSubject.eraseToAnyPublisher() syncHTTPRequestErrorPublisher = syncHTTPRequestErrorSubject.eraseToAnyPublisher() isSyncInProgressPublisher = Publishers diff --git a/Sources/DDGSync/internal/SyncRequestMaker.swift b/Sources/DDGSync/internal/SyncRequestMaker.swift index dd78fd5aa..2469c7053 100644 --- a/Sources/DDGSync/internal/SyncRequestMaker.swift +++ b/Sources/DDGSync/internal/SyncRequestMaker.swift @@ -28,6 +28,7 @@ struct SyncRequestMaker: SyncRequestMaking { let storage: SecureStoring let api: RemoteAPIRequestCreating let endpoints: Endpoints + let payloadCompressor: SyncPayloadCompressing let dateFormatter = ISO8601DateFormatter() func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting { @@ -55,14 +56,19 @@ struct SyncRequestMaker: SyncRequestMaking { throw SyncError.unableToEncodeRequestBody("Sync PATCH payload is not a valid JSON") } + let body = try JSONSerialization.data(withJSONObject: json, options: []) + guard isCompressed else { - let body = try JSONSerialization.data(withJSONObject: json, options: []) return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body) } - let headers = ["Content-Encoding": "gzip"] - let body = try JSONSerialization.data(withJSONObject: json, options: []).gzipped() - return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body, headers: headers) + let compressedBody = try payloadCompressor.compress(body) + return api.createAuthenticatedJSONRequest( + url: endpoints.syncPatch, + method: .PATCH, + authToken: try getToken(), + json: compressedBody, + headers: ["Content-Encoding": "gzip"]) } private func getToken() throws -> String { diff --git a/Tests/DDGSyncTests/Mocks/Mocks.swift b/Tests/DDGSyncTests/Mocks/Mocks.swift index 301e7cd7e..f76116f0a 100644 --- a/Tests/DDGSyncTests/Mocks/Mocks.swift +++ b/Tests/DDGSyncTests/Mocks/Mocks.swift @@ -20,6 +20,7 @@ import BrowserServicesKit import Combine import Common import Foundation +import Gzip import Persistence import TestUtils @@ -199,6 +200,7 @@ struct MockSyncDependencies: SyncDependencies, SyncDependenciesDebuggingSupport var endpoints: Endpoints = Endpoints(baseURL: URL(string: "https://dev.null")!) var account: AccountManaging = AccountManagingMock() var api: RemoteAPIRequestCreating = RemoteAPIRequestCreatingMock() + var payloadCompressor: SyncPayloadCompressing = SyncPayloadCompressorMock() var secureStore: SecureStoring = SecureStorageStub() var crypter: CryptingInternal = CryptingMock() var scheduler: SchedulingInternal = SchedulerMock() @@ -276,6 +278,45 @@ class RemoteAPIRequestCreatingMock: RemoteAPIRequestCreating { } } +class InspectableSyncRequestMaker: SyncRequestMaking { + + struct MakePatchRequestCallArgs { + let result: SyncRequest + let clientTimestamp: Date + let isCompressed: Bool + } + + func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting { + try requestMaker.makeGetRequest(with: result) + } + + func makePatchRequest(with result: SyncRequest, clientTimestamp: Date, isCompressed: Bool) throws -> HTTPRequesting { + makePatchRequestCallCount += 1 + makePatchRequestCallArgs.append(.init(result: result, clientTimestamp: clientTimestamp, isCompressed: isCompressed)) + return try requestMaker.makePatchRequest(with: result, clientTimestamp: clientTimestamp, isCompressed: isCompressed) + } + + let requestMaker: SyncRequestMaker + + init(requestMaker: SyncRequestMaker) { + self.requestMaker = requestMaker + } + + var makePatchRequestCallCount = 0 + var makePatchRequestCallArgs: [MakePatchRequestCallArgs] = [] +} + +class SyncPayloadCompressorMock: SyncPayloadCompressing { + var error: Error? + + func compress(_ payload: Data) throws -> Data { + if let error { + throw error + } + return try payload.gzipped() + } +} + struct CryptingMock: CryptingInternal { var _encryptAndBase64Encode: (String) throws -> String = { "encrypted_\($0)" } var _base64DecodeAndDecrypt: (String) throws -> String = { $0.dropping(prefix: "encrypted_") } diff --git a/Tests/DDGSyncTests/SyncOperationTests.swift b/Tests/DDGSyncTests/SyncOperationTests.swift index fc128b840..a849b220a 100644 --- a/Tests/DDGSyncTests/SyncOperationTests.swift +++ b/Tests/DDGSyncTests/SyncOperationTests.swift @@ -18,15 +18,17 @@ import XCTest +@testable import Gzip @testable import DDGSync class SyncOperationTests: XCTestCase { var apiMock: RemoteAPIRequestCreatingMock! var request: HTTPRequestingMock! var endpoints: Endpoints! + var payloadCompressor: SyncPayloadCompressorMock! var storage: SecureStorageStub! var crypter: CryptingMock! - var requestMaker: SyncRequestMaking! + var requestMaker: InspectableSyncRequestMaker! override func setUpWithError() throws { try super.setUpWithError() @@ -35,6 +37,7 @@ class SyncOperationTests: XCTestCase { request = HTTPRequestingMock() apiMock.request = request endpoints = Endpoints(baseURL: URL(string: "https://example.com")!) + payloadCompressor = SyncPayloadCompressorMock() storage = SecureStorageStub() crypter = CryptingMock() try storage.persistAccount( @@ -50,7 +53,9 @@ class SyncOperationTests: XCTestCase { ) ) - requestMaker = SyncRequestMaker(storage: storage, api: apiMock, endpoints: endpoints) + requestMaker = InspectableSyncRequestMaker( + requestMaker: .init(storage: storage, api: apiMock, endpoints: endpoints, payloadCompressor: payloadCompressor) + ) } func testWhenThereAreNoChangesThenGetRequestIsFired() async throws { @@ -245,6 +250,62 @@ class SyncOperationTests: XCTestCase { XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) } + + func testWhenPatchPayloadCompressionSucceedsThenPayloadIsSentCompressed() async throws { + let objectsToSync = [ + Syncable(jsonObject: ["id": "1", "name": "bookmark1", "url": "https://example.com"]), + ] + let dataProvider = DataProvidingMock(feature: .init(name: "bookmarks")) + var sentModels: [Syncable] = [] + var errors: [any Error] = [] + dataProvider.updateSyncTimestamps(server: "1234", local: nil) + dataProvider._fetchChangedObjects = { _ in objectsToSync } + dataProvider._handleSyncError = { errors.append($0) } + dataProvider.handleSyncResponse = { sent, _, _, _, _ in + sentModels = sent + } + + let syncOperation = SyncOperation(dataProviders: [dataProvider], storage: storage, crypter: crypter, requestMaker: requestMaker) + request.result = .init(data: nil, response: HTTPURLResponse(url: URL(string: "https://example.com")!, statusCode: 304, httpVersion: nil, headerFields: nil)!) + + try await syncOperation.sync(fetchOnly: false) + + XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) + XCTAssertTrue(errors.isEmpty) + XCTAssertEqual(requestMaker.makePatchRequestCallCount, 1) + XCTAssertEqual(requestMaker.makePatchRequestCallArgs[0].isCompressed, true) + } + + func testWhenPatchPayloadCompressionFailsThenPayloadIsSentUncompressed() async throws { + let errorCode = 100200300 + payloadCompressor.error = GzipError(code: Int32(errorCode), msg: nil) + + let objectsToSync = [ + Syncable(jsonObject: ["id": "1", "name": "bookmark1", "url": "https://example.com"]), + ] + let dataProvider = DataProvidingMock(feature: .init(name: "bookmarks")) + var sentModels: [Syncable] = [] + var errors: [any Error] = [] + dataProvider.updateSyncTimestamps(server: "1234", local: nil) + dataProvider._fetchChangedObjects = { _ in objectsToSync } + dataProvider._handleSyncError = { errors.append($0) } + dataProvider.handleSyncResponse = { sent, _, _, _, _ in + sentModels = sent + } + + let syncOperation = SyncOperation(dataProviders: [dataProvider], storage: storage, crypter: crypter, requestMaker: requestMaker) + request.result = .init(data: nil, response: HTTPURLResponse(url: URL(string: "https://example.com")!, statusCode: 304, httpVersion: nil, headerFields: nil)!) + + try await syncOperation.sync(fetchOnly: false) + + XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) + XCTAssertEqual(errors.count, 1) + let error = try XCTUnwrap(errors.first as? GzipError) + XCTAssertEqual(error.kind, .unknown(code: errorCode)) + XCTAssertEqual(requestMaker.makePatchRequestCallCount, 2) + XCTAssertEqual(requestMaker.makePatchRequestCallArgs[0].isCompressed, true) + XCTAssertEqual(requestMaker.makePatchRequestCallArgs[1].isCompressed, false) + } } private extension Array where Element == Syncable { diff --git a/Tests/DDGSyncTests/SyncQueueTests.swift b/Tests/DDGSyncTests/SyncQueueTests.swift index 202e3bd06..a9f5ef392 100644 --- a/Tests/DDGSyncTests/SyncQueueTests.swift +++ b/Tests/DDGSyncTests/SyncQueueTests.swift @@ -27,6 +27,7 @@ class SyncQueueTests: XCTestCase { var storage: SecureStorageStub! var crypter: CryptingMock! var requestMaker: SyncRequestMaking! + var payloadCompressor: SyncPayloadCompressing! override func setUpWithError() throws { try super.setUpWithError() @@ -34,6 +35,7 @@ class SyncQueueTests: XCTestCase { apiMock = RemoteAPIRequestCreatingMock() request = HTTPRequestingMock() apiMock.request = request + payloadCompressor = SyncPayloadCompressorMock() endpoints = Endpoints(baseURL: URL(string: "https://example.com")!) storage = SecureStorageStub() crypter = CryptingMock() @@ -50,11 +52,18 @@ class SyncQueueTests: XCTestCase { ) ) - requestMaker = SyncRequestMaker(storage: storage, api: apiMock, endpoints: endpoints) + requestMaker = SyncRequestMaker(storage: storage, api: apiMock, endpoints: endpoints, payloadCompressor: payloadCompressor) } func testWhenDataSyncingFeatureFlagIsDisabledThenNewOperationsAreNotEnqueued() async { - let syncQueue = SyncQueue(dataProviders: [], storage: storage, crypter: crypter, api: apiMock, endpoints: endpoints) + let syncQueue = SyncQueue( + dataProviders: [], + storage: storage, + crypter: crypter, + api: apiMock, + endpoints: endpoints, + payloadCompressor: payloadCompressor + ) XCTAssertFalse(syncQueue.operationQueue.isSuspended) var syncDidStartEvents = [Bool]() @@ -81,7 +90,14 @@ class SyncQueueTests: XCTestCase { func testThatInProgressPublisherEmitsValuesWhenSyncStartsAndEndsWithSuccess() async throws { let feature = Feature(name: "bookmarks") let dataProvider = DataProvidingMock(feature: feature) - let syncQueue = SyncQueue(dataProviders: [dataProvider], storage: storage, crypter: crypter, api: apiMock, endpoints: endpoints) + let syncQueue = SyncQueue( + dataProviders: [dataProvider], + storage: storage, + crypter: crypter, + api: apiMock, + endpoints: endpoints, + payloadCompressor: payloadCompressor + ) var isInProgressEvents = [Bool]() @@ -99,7 +115,14 @@ class SyncQueueTests: XCTestCase { func testThatInProgressPublisherEmitsValuesWhenSyncStartsAndEndsWithError() async throws { let feature = Feature(name: "bookmarks") let dataProvider = DataProvidingMock(feature: feature) - let syncQueue = SyncQueue(dataProviders: [dataProvider], storage: storage, crypter: crypter, api: apiMock, endpoints: endpoints) + let syncQueue = SyncQueue( + dataProviders: [dataProvider], + storage: storage, + crypter: crypter, + api: apiMock, + endpoints: endpoints, + payloadCompressor: payloadCompressor + ) var isInProgressEvents = [Bool]() From fbfcb614f152d244b475cee704e1c28fd80a3669 Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Mon, 6 May 2024 13:10:22 +0200 Subject: [PATCH 05/10] Make Swiftlint happy --- Sources/DDGSync/internal/SyncPayloadCompressor.swift | 1 - 1 file changed, 1 deletion(-) diff --git a/Sources/DDGSync/internal/SyncPayloadCompressor.swift b/Sources/DDGSync/internal/SyncPayloadCompressor.swift index 9642e4251..7b4c3d389 100644 --- a/Sources/DDGSync/internal/SyncPayloadCompressor.swift +++ b/Sources/DDGSync/internal/SyncPayloadCompressor.swift @@ -1,6 +1,5 @@ // // SyncPayloadCompressor.swift -// DuckDuckGo // // Copyright © 2024 DuckDuckGo. All rights reserved. // From dff5c287ff00b10c2065e3519b3a37e53c545fe8 Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Wed, 8 May 2024 13:52:36 +0200 Subject: [PATCH 06/10] Add GzipError.errorCode --- .../internal/SyncPayloadCompressor.swift | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Sources/DDGSync/internal/SyncPayloadCompressor.swift b/Sources/DDGSync/internal/SyncPayloadCompressor.swift index 7b4c3d389..f38391361 100644 --- a/Sources/DDGSync/internal/SyncPayloadCompressor.swift +++ b/Sources/DDGSync/internal/SyncPayloadCompressor.swift @@ -28,3 +28,24 @@ struct SyncPayloadCompressor: SyncPayloadCompressing { try payload.gzipped() } } + +extension GzipError: CustomNSError { + public static let errorDomain: String = "GzipError" + + public var errorCode: Int { + switch kind { + case .stream: + return -2 + case .data: + return -3 + case .memory: + return -4 + case .buffer: + return -5 + case .version: + return -6 + case .unknown(let code): + return code + } + } +} From 67baf349eccc3aedda20d105a11fc19339a2dec3 Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Wed, 8 May 2024 14:04:19 +0200 Subject: [PATCH 07/10] Add SyncError.patchPayloadCompressionFailed --- Sources/DDGSync/SyncError.swift | 4 ++++ Sources/DDGSync/internal/SyncOperation.swift | 2 +- Sources/DDGSync/internal/SyncPayloadCompressor.swift | 8 ++++---- Tests/DDGSyncTests/SyncOperationTests.swift | 11 +++++++---- 4 files changed, 16 insertions(+), 9 deletions(-) diff --git a/Sources/DDGSync/SyncError.swift b/Sources/DDGSync/SyncError.swift index c1cc51f61..ef3faad38 100644 --- a/Sources/DDGSync/SyncError.swift +++ b/Sources/DDGSync/SyncError.swift @@ -57,6 +57,7 @@ public enum SyncError: Error, Equatable { case settingsMetadataNotPresent case unauthenticatedWhileLoggedIn + case patchPayloadCompressionFailed(_ errorCode: Int) public var isServerError: Bool { switch self { @@ -137,6 +138,8 @@ public enum SyncError: Error, Equatable { return [syncErrorString: "settingsMetadataNotPresent"] case .unauthenticatedWhileLoggedIn: return [syncErrorString: "unauthenticatedWhileLoggedIn"] + case .patchPayloadCompressionFailed: + return [syncErrorString: "patchPayloadCompressionFailed"] } } } @@ -183,6 +186,7 @@ extension SyncError: CustomNSError { case .emailProtectionUsernamePresentButTokenMissing: return 26 case .settingsMetadataNotPresent: return 27 case .unauthenticatedWhileLoggedIn: return 28 + case .patchPayloadCompressionFailed: return 29 } } diff --git a/Sources/DDGSync/internal/SyncOperation.swift b/Sources/DDGSync/internal/SyncOperation.swift index 2e4777d06..ee1fa9669 100644 --- a/Sources/DDGSync/internal/SyncOperation.swift +++ b/Sources/DDGSync/internal/SyncOperation.swift @@ -218,7 +218,7 @@ final class SyncOperation: Operation { do { return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: true) } catch let error as GzipError { - dataProvider.handleSyncError(error) + dataProvider.handleSyncError(SyncError.patchPayloadCompressionFailed(error.errorCode)) return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: false) } } diff --git a/Sources/DDGSync/internal/SyncPayloadCompressor.swift b/Sources/DDGSync/internal/SyncPayloadCompressor.swift index f38391361..e2546e470 100644 --- a/Sources/DDGSync/internal/SyncPayloadCompressor.swift +++ b/Sources/DDGSync/internal/SyncPayloadCompressor.swift @@ -29,10 +29,10 @@ struct SyncPayloadCompressor: SyncPayloadCompressing { } } -extension GzipError: CustomNSError { - public static let errorDomain: String = "GzipError" - - public var errorCode: Int { +extension GzipError { + /// Mapping is taken from `GzipError.Kind` documentation which maps zlib error codes to enum cases, + /// and we're effectively reversing that mapping here. + var errorCode: Int { switch kind { case .stream: return -2 diff --git a/Tests/DDGSyncTests/SyncOperationTests.swift b/Tests/DDGSyncTests/SyncOperationTests.swift index a849b220a..ad0940c5c 100644 --- a/Tests/DDGSyncTests/SyncOperationTests.swift +++ b/Tests/DDGSyncTests/SyncOperationTests.swift @@ -271,9 +271,9 @@ class SyncOperationTests: XCTestCase { try await syncOperation.sync(fetchOnly: false) XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) - XCTAssertTrue(errors.isEmpty) XCTAssertEqual(requestMaker.makePatchRequestCallCount, 1) XCTAssertEqual(requestMaker.makePatchRequestCallArgs[0].isCompressed, true) + XCTAssertTrue(errors.isEmpty) } func testWhenPatchPayloadCompressionFailsThenPayloadIsSentUncompressed() async throws { @@ -299,12 +299,15 @@ class SyncOperationTests: XCTestCase { try await syncOperation.sync(fetchOnly: false) XCTAssertTrue(try sentModels.isJSONRepresentationEquivalent(to: objectsToSync)) - XCTAssertEqual(errors.count, 1) - let error = try XCTUnwrap(errors.first as? GzipError) - XCTAssertEqual(error.kind, .unknown(code: errorCode)) XCTAssertEqual(requestMaker.makePatchRequestCallCount, 2) XCTAssertEqual(requestMaker.makePatchRequestCallArgs[0].isCompressed, true) XCTAssertEqual(requestMaker.makePatchRequestCallArgs[1].isCompressed, false) + XCTAssertEqual(errors.count, 1) + let error = try XCTUnwrap(errors.first) + guard case SyncError.patchPayloadCompressionFailed(errorCode) = error else { + XCTFail("Unexpected error thrown: \(error)") + return + } } } From 9b73e18a83bada23d78f94008e6a039dd3fd4197 Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Wed, 8 May 2024 17:16:28 +0200 Subject: [PATCH 08/10] WIP --- Sources/DDGSync/internal/SyncPayloadCompressor.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/DDGSync/internal/SyncPayloadCompressor.swift b/Sources/DDGSync/internal/SyncPayloadCompressor.swift index e2546e470..2a3edb5bf 100644 --- a/Sources/DDGSync/internal/SyncPayloadCompressor.swift +++ b/Sources/DDGSync/internal/SyncPayloadCompressor.swift @@ -17,7 +17,7 @@ // import Foundation -import Gzip +@testable import Gzip protocol SyncPayloadCompressing { func compress(_ payload: Data) throws -> Data @@ -25,6 +25,7 @@ protocol SyncPayloadCompressing { struct SyncPayloadCompressor: SyncPayloadCompressing { func compress(_ payload: Data) throws -> Data { + throw GzipError(code: 400, msg: nil) try payload.gzipped() } } From 803be7265bac4eefff35be97aed6d7d693d8c8a9 Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Thu, 9 May 2024 21:14:44 +0200 Subject: [PATCH 09/10] Remove test code --- Sources/DDGSync/internal/SyncPayloadCompressor.swift | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Sources/DDGSync/internal/SyncPayloadCompressor.swift b/Sources/DDGSync/internal/SyncPayloadCompressor.swift index 2a3edb5bf..e2546e470 100644 --- a/Sources/DDGSync/internal/SyncPayloadCompressor.swift +++ b/Sources/DDGSync/internal/SyncPayloadCompressor.swift @@ -17,7 +17,7 @@ // import Foundation -@testable import Gzip +import Gzip protocol SyncPayloadCompressing { func compress(_ payload: Data) throws -> Data @@ -25,7 +25,6 @@ protocol SyncPayloadCompressing { struct SyncPayloadCompressor: SyncPayloadCompressing { func compress(_ payload: Data) throws -> Data { - throw GzipError(code: 400, msg: nil) try payload.gzipped() } } From c4b3b2e68b22e7564a26e85f319cb284eef64de7 Mon Sep 17 00:00:00 2001 From: Dominik Kapusta Date: Fri, 10 May 2024 14:31:48 +0200 Subject: [PATCH 10/10] Update naming and formatting according to code review comments --- Sources/DDGSync/internal/ProductionDependencies.swift | 2 +- ...oadCompressor.swift => SyncGzipPayloadCompressor.swift} | 4 ++-- Sources/DDGSync/internal/SyncRequestMaker.swift | 7 ++++++- Tests/DDGSyncTests/Mocks/Mocks.swift | 4 ++-- Tests/DDGSyncTests/SyncOperationTests.swift | 4 ++-- Tests/DDGSyncTests/SyncQueueTests.swift | 2 +- 6 files changed, 14 insertions(+), 9 deletions(-) rename Sources/DDGSync/internal/{SyncPayloadCompressor.swift => SyncGzipPayloadCompressor.swift} (93%) diff --git a/Sources/DDGSync/internal/ProductionDependencies.swift b/Sources/DDGSync/internal/ProductionDependencies.swift index d9a3241e8..720b91ce3 100644 --- a/Sources/DDGSync/internal/ProductionDependencies.swift +++ b/Sources/DDGSync/internal/ProductionDependencies.swift @@ -73,7 +73,7 @@ struct ProductionDependencies: SyncDependencies { self.getLog = log api = RemoteAPIRequestCreator(log: log()) - payloadCompressor = SyncPayloadCompressor() + payloadCompressor = SyncGzipPayloadCompressor() crypter = Crypter(secureStore: secureStore) account = AccountManager(endpoints: endpoints, api: api, crypter: crypter) diff --git a/Sources/DDGSync/internal/SyncPayloadCompressor.swift b/Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift similarity index 93% rename from Sources/DDGSync/internal/SyncPayloadCompressor.swift rename to Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift index e2546e470..fa295e9e8 100644 --- a/Sources/DDGSync/internal/SyncPayloadCompressor.swift +++ b/Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift @@ -1,5 +1,5 @@ // -// SyncPayloadCompressor.swift +// SyncGzipPayloadCompressor.swift // // Copyright © 2024 DuckDuckGo. All rights reserved. // @@ -23,7 +23,7 @@ protocol SyncPayloadCompressing { func compress(_ payload: Data) throws -> Data } -struct SyncPayloadCompressor: SyncPayloadCompressing { +struct SyncGzipPayloadCompressor: SyncPayloadCompressing { func compress(_ payload: Data) throws -> Data { try payload.gzipped() } diff --git a/Sources/DDGSync/internal/SyncRequestMaker.swift b/Sources/DDGSync/internal/SyncRequestMaker.swift index 2469c7053..b814c16e5 100644 --- a/Sources/DDGSync/internal/SyncRequestMaker.swift +++ b/Sources/DDGSync/internal/SyncRequestMaker.swift @@ -59,7 +59,12 @@ struct SyncRequestMaker: SyncRequestMaking { let body = try JSONSerialization.data(withJSONObject: json, options: []) guard isCompressed else { - return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body) + return api.createAuthenticatedJSONRequest( + url: endpoints.syncPatch, + method: .PATCH, + authToken: try getToken(), + json: body + ) } let compressedBody = try payloadCompressor.compress(body) diff --git a/Tests/DDGSyncTests/Mocks/Mocks.swift b/Tests/DDGSyncTests/Mocks/Mocks.swift index f76116f0a..b4cd4e896 100644 --- a/Tests/DDGSyncTests/Mocks/Mocks.swift +++ b/Tests/DDGSyncTests/Mocks/Mocks.swift @@ -200,7 +200,7 @@ struct MockSyncDependencies: SyncDependencies, SyncDependenciesDebuggingSupport var endpoints: Endpoints = Endpoints(baseURL: URL(string: "https://dev.null")!) var account: AccountManaging = AccountManagingMock() var api: RemoteAPIRequestCreating = RemoteAPIRequestCreatingMock() - var payloadCompressor: SyncPayloadCompressing = SyncPayloadCompressorMock() + var payloadCompressor: SyncPayloadCompressing = SyncGzipPayloadCompressorMock() var secureStore: SecureStoring = SecureStorageStub() var crypter: CryptingInternal = CryptingMock() var scheduler: SchedulingInternal = SchedulerMock() @@ -306,7 +306,7 @@ class InspectableSyncRequestMaker: SyncRequestMaking { var makePatchRequestCallArgs: [MakePatchRequestCallArgs] = [] } -class SyncPayloadCompressorMock: SyncPayloadCompressing { +class SyncGzipPayloadCompressorMock: SyncPayloadCompressing { var error: Error? func compress(_ payload: Data) throws -> Data { diff --git a/Tests/DDGSyncTests/SyncOperationTests.swift b/Tests/DDGSyncTests/SyncOperationTests.swift index ad0940c5c..0a90466e8 100644 --- a/Tests/DDGSyncTests/SyncOperationTests.swift +++ b/Tests/DDGSyncTests/SyncOperationTests.swift @@ -25,7 +25,7 @@ class SyncOperationTests: XCTestCase { var apiMock: RemoteAPIRequestCreatingMock! var request: HTTPRequestingMock! var endpoints: Endpoints! - var payloadCompressor: SyncPayloadCompressorMock! + var payloadCompressor: SyncGzipPayloadCompressorMock! var storage: SecureStorageStub! var crypter: CryptingMock! var requestMaker: InspectableSyncRequestMaker! @@ -37,7 +37,7 @@ class SyncOperationTests: XCTestCase { request = HTTPRequestingMock() apiMock.request = request endpoints = Endpoints(baseURL: URL(string: "https://example.com")!) - payloadCompressor = SyncPayloadCompressorMock() + payloadCompressor = SyncGzipPayloadCompressorMock() storage = SecureStorageStub() crypter = CryptingMock() try storage.persistAccount( diff --git a/Tests/DDGSyncTests/SyncQueueTests.swift b/Tests/DDGSyncTests/SyncQueueTests.swift index a9f5ef392..dafe96736 100644 --- a/Tests/DDGSyncTests/SyncQueueTests.swift +++ b/Tests/DDGSyncTests/SyncQueueTests.swift @@ -35,7 +35,7 @@ class SyncQueueTests: XCTestCase { apiMock = RemoteAPIRequestCreatingMock() request = HTTPRequestingMock() apiMock.request = request - payloadCompressor = SyncPayloadCompressorMock() + payloadCompressor = SyncGzipPayloadCompressorMock() endpoints = Endpoints(baseURL: URL(string: "https://example.com")!) storage = SecureStorageStub() crypter = CryptingMock()