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.

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

Enable gzip compression for Sync PATCH payloads #803

Merged
merged 13 commits into from
May 10, 2024
9 changes: 9 additions & 0 deletions Package.resolved
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions Package.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -153,6 +154,7 @@ let package = Package(
"BrowserServicesKit",
"Common",
.product(name: "DDGSyncCrypto", package: "sync_crypto"),
.product(name: "Gzip", package: "GzipSwift"),
"Networking",
],
resources: [
Expand Down
4 changes: 4 additions & 0 deletions Sources/DDGSync/SyncError.swift
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public enum SyncError: Error, Equatable {
case settingsMetadataNotPresent

case unauthenticatedWhileLoggedIn
case patchPayloadCompressionFailed(_ errorCode: Int)

public var isServerError: Bool {
switch self {
Expand Down Expand Up @@ -137,6 +138,8 @@ public enum SyncError: Error, Equatable {
return [syncErrorString: "settingsMetadataNotPresent"]
case .unauthenticatedWhileLoggedIn:
return [syncErrorString: "unauthenticatedWhileLoggedIn"]
case .patchPayloadCompressionFailed:
return [syncErrorString: "patchPayloadCompressionFailed"]
}
}
}
Expand Down Expand Up @@ -183,6 +186,7 @@ extension SyncError: CustomNSError {
case .emailProtectionUsernamePresentButTokenMissing: return 26
case .settingsMetadataNotPresent: return 27
case .unauthenticatedWhileLoggedIn: return 28
case .patchPayloadCompressionFailed: return 29
}
}

Expand Down
2 changes: 2 additions & 0 deletions Sources/DDGSync/internal/ProductionDependencies.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down
1 change: 1 addition & 0 deletions Sources/DDGSync/internal/SyncDependencies.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down
51 changes: 51 additions & 0 deletions Sources/DDGSync/internal/SyncGzipPayloadCompressor.swift
Original file line number Diff line number Diff line change
@@ -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
}
}
}
12 changes: 9 additions & 3 deletions Sources/DDGSync/internal/SyncOperation.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import Foundation
import Combine
import Common
import Gzip

final class SyncOperation: Operation {

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
}
Expand Down
4 changes: 3 additions & 1 deletion Sources/DDGSync/internal/SyncQueue.swift
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ final class SyncQueue {
crypter: dependencies.crypter,
api: dependencies.api,
endpoints: dependencies.endpoints,
payloadCompressor: dependencies.payloadCompressor,
log: dependencies.log
)
}
Expand All @@ -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
Expand Down
24 changes: 21 additions & 3 deletions Sources/DDGSync/internal/SyncRequestMaker.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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),
Expand All @@ -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 {
Expand Down
41 changes: 41 additions & 0 deletions Tests/DDGSyncTests/Mocks/Mocks.swift
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import BrowserServicesKit
import Combine
import Common
import Foundation
import Gzip
import Persistence
import TestUtils

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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_") }
Expand Down