diff --git a/Package.resolved b/Package.resolved index dadbd9a5c..2a423b3a6 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 8cd51ddc5..ebf53737c 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( @@ -153,6 +154,7 @@ let package = Package( "BrowserServicesKit", "Common", .product(name: "DDGSyncCrypto", package: "sync_crypto"), + .product(name: "Gzip", package: "GzipSwift"), "Networking", ], resources: [ 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/ProductionDependencies.swift b/Sources/DDGSync/internal/ProductionDependencies.swift index 66724039e..720b91ce3 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 = SyncGzipPayloadCompressor() 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/SyncGzipPayloadCompressor.swift b/Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift new file mode 100644 index 000000000..fa295e9e8 --- /dev/null +++ b/Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift @@ -0,0 +1,51 @@ +// +// SyncGzipPayloadCompressor.swift +// +// 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 SyncGzipPayloadCompressor: SyncPayloadCompressing { + func compress(_ payload: Data) throws -> Data { + try payload.gzipped() + } +} + +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 + case .data: + return -3 + case .memory: + return -4 + case .buffer: + return -5 + case .version: + return -6 + case .unknown(let code): + return code + } + } +} diff --git a/Sources/DDGSync/internal/SyncOperation.swift b/Sources/DDGSync/internal/SyncOperation.swift index b4e84e756..ee1fa9669 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(SyncError.patchPayloadCompressionFailed(error.errorCode)) + return try requestMaker.makePatchRequest(with: syncRequest, clientTimestamp: timestamp, isCompressed: false) + } } return try requestMaker.makeGetRequest(with: syncRequest) } 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 12382e667..b814c16e5 100644 --- a/Sources/DDGSync/internal/SyncRequestMaker.swift +++ b/Sources/DDGSync/internal/SyncRequestMaker.swift @@ -17,16 +17,18 @@ // import Foundation +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 { let storage: SecureStoring let api: RemoteAPIRequestCreating let endpoints: Endpoints + let payloadCompressor: SyncPayloadCompressing let dateFormatter = ISO8601DateFormatter() func makeGetRequest(with result: SyncRequest) throws -> HTTPRequesting { @@ -41,7 +43,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,7 +57,23 @@ struct SyncRequestMaker: SyncRequestMaking { } let body = try JSONSerialization.data(withJSONObject: json, options: []) - return api.createAuthenticatedJSONRequest(url: endpoints.syncPatch, method: .PATCH, authToken: try getToken(), json: body) + + guard isCompressed else { + return api.createAuthenticatedJSONRequest( + url: endpoints.syncPatch, + method: .PATCH, + authToken: try getToken(), + json: body + ) + } + + 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..b4cd4e896 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 = SyncGzipPayloadCompressorMock() 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 SyncGzipPayloadCompressorMock: 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 e76df782d..0a90466e8 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: SyncGzipPayloadCompressorMock! 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 = SyncGzipPayloadCompressorMock() 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 { @@ -167,16 +172,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 } @@ -245,6 +250,65 @@ 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)) + XCTAssertEqual(requestMaker.makePatchRequestCallCount, 1) + XCTAssertEqual(requestMaker.makePatchRequestCallArgs[0].isCompressed, true) + XCTAssertTrue(errors.isEmpty) + } + + 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(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 + } + } } private extension Array where Element == Syncable { diff --git a/Tests/DDGSyncTests/SyncQueueTests.swift b/Tests/DDGSyncTests/SyncQueueTests.swift index 202e3bd06..dafe96736 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 = SyncGzipPayloadCompressorMock() 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]()