Skip to content

Commit

Permalink
Enable gzip compression for Sync PATCH payloads (#803)
Browse files Browse the repository at this point in the history
Task/Issue URL: https://app.asana.com/0/0/1206919211758354/f

Description:
This change adds gzip compression to all Sync data PATCH requests.
  • Loading branch information
ayoy authored May 10, 2024
1 parent 49d6842 commit 72be4e7
Show file tree
Hide file tree
Showing 12 changed files with 239 additions and 16 deletions.
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
Loading

0 comments on commit 72be4e7

Please sign in to comment.