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

Add event logging upload #127

Merged
merged 24 commits into from
Mar 30, 2020
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d1cd9c9
Implement Event Logging
jkmassel Aug 29, 2019
7dafc39
Bump the Podspec to Swift 5
jkmassel Mar 23, 2020
4f20dc6
Crash if event network response is invalid type
jkmassel Mar 25, 2020
fac0f65
Inject URLSession in EventLoggingNetworkService
jkmassel Mar 25, 2020
b3ce8fe
Move NetworkResultCallback inside the class
jkmassel Mar 25, 2020
6b135c5
Rename UploadError to EventLoggingFileUploadError
jkmassel Mar 25, 2020
01408c7
Fix code formatting
jkmassel Mar 25, 2020
ac75328
Add localization for missing files error message
jkmassel Mar 25, 2020
de728bd
Use current instance of FileManager in extensions
jkmassel Mar 25, 2020
810b5c9
Allow injecting storageDirectory and fileManager
jkmassel Mar 25, 2020
92afe2d
Fix some style issues
jkmassel Mar 25, 2020
4ea3255
Rework EventLoggingUploadManager initialization
jkmassel Mar 26, 2020
0f92472
Don’t leak LogFile abstractions when reading
jkmassel Mar 26, 2020
5e34b4e
Document delegate ordering
jkmassel Mar 26, 2020
d0b44f9
Add direct tests for EventLoggingNetworkService
jkmassel Mar 26, 2020
4f2808a
Test style fixes
jkmassel Mar 26, 2020
0c5b5bf
Use an enum instead of a struct
jkmassel Mar 26, 2020
a970df2
Avoid mutability in EventLoggingUploadManager
jkmassel Mar 26, 2020
5da3755
Rename `async_test` to `waitForExpectation`
jkmassel Mar 26, 2020
7458a4b
Merge branch 'develop' into add/event-logging-upload
jkmassel Mar 26, 2020
ffd68af
Fix Swiftlint nits
jkmassel Mar 26, 2020
a8fb2c0
Remove `open` modifier
jkmassel Mar 26, 2020
1d9e9f8
Improve inline documentation for force-unwrap
jkmassel Mar 26, 2020
8cc85d7
One last Swiftlint nit
jkmassel Mar 26, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Automattic-Tracks-iOS.podspec
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Pod::Spec.new do |spec|
spec.authors = { 'Automattic' => '[email protected]' }
spec.summary = 'Simple way to track events in an iOS app with Automattic Tracks internal service'
spec.source = { :git => 'https://github.com/Automattic/Automattic-Tracks-iOS.git', :tag => spec.version.to_s }
spec.swift_version = '4.2'
spec.swift_version = '5.0'

spec.ios.source_files = 'Automattic-Tracks-iOS/**/*.{h,m,swift}'
spec.ios.exclude_files = 'Automattic-Tracks-OSX/Automattic_Tracks_OSX.h'
Expand Down
90 changes: 84 additions & 6 deletions Automattic-Tracks-iOS.xcodeproj/project.pbxproj

Large diffs are not rendered by default.

19 changes: 19 additions & 0 deletions Automattic-Tracks-iOS/Event Logging/EventLoggingDataSource.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import Foundation

public protocol EventLoggingDataSource {
/// A base-64 encoded representation of the encryption key.
var loggingEncryptionKey: String { get }

/// The URL to the upload endpoint for encrypted logs.
var logUploadURL: URL { get }

/// The path to the log file for the most recent session.
var previousSessionLogPath: URL? { get }
}

public extension EventLoggingDataSource {
// The default implementation points to the WP.com private encrypted logging API
var logUploadURL: URL {
return URL(string: "https://public-api.wordpress.com/rest/v1.1/encrypted-logging")!
}
}
35 changes: 35 additions & 0 deletions Automattic-Tracks-iOS/Event Logging/EventLoggingDelegate.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import Foundation

public protocol EventLoggingDelegate {
shiki marked this conversation as resolved.
Show resolved Hide resolved
/// The event logging system will call this delegate property prior to attempting to upload, giving the application a chance to determine
/// whether or not the upload should proceed. If this is not overridden, the default is `false`.
var shouldUploadLogFiles: Bool { get }

/// The event logging system will call this delegate method each time a log file starts uploading.
func didStartUploadingLog(_ log: LogFile)

/// The event logging system will call this delegate method if a log file upload is cancelled by the delegate.
func uploadCancelledByDelegate(_ log: LogFile)

/// The event logging system will call this delegate method if a log file fails to upload.
/// It may be called prior to upload starting if the file is missing, and is called prior to the `upload` callback.
func uploadFailed(withError: Error, forLog: LogFile)

/// The event logging system will call this delegate method each time a log file finishes uploading.
/// It is called prior to the `upload` callback.
func didFinishUploadingLog(_ log: LogFile)
}

/// Default implementations for EventLoggingDelegate
public extension EventLoggingDelegate {

var shouldUploadLogFiles: Bool {
return false // Use a privacy-preserving default
}

// Empty default implementations allow the developer to only implement these if they need them
func didStartUploadingLog(_ log: LogFile) {}
func uploadCancelledByDelegate(_ log: LogFile) {}
func uploadFailed(withError error: Error, forLog log: LogFile) {}
func didFinishUploadingLog(_ log: LogFile) {}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
import Foundation
import CocoaLumberjack
import Sodium

class EventLoggingUploadManager {
shiki marked this conversation as resolved.
Show resolved Hide resolved

private struct Constants {
oguzkocer marked this conversation as resolved.
Show resolved Hide resolved
static let uuidHeaderKey = "log-uuid"
static let uploadHttpMethod = "POST"
}

var dataSource: EventLoggingDataSource
var delegate: EventLoggingDelegate
var networkService: EventLoggingNetworkService = EventLoggingNetworkService()
var fileManager: FileManager = FileManager.default
oguzkocer marked this conversation as resolved.
Show resolved Hide resolved

typealias LogUploadCallback = (Result<Void, Error>) -> Void

init(dataSource: EventLoggingDataSource, delegate: EventLoggingDelegate) {
self.dataSource = dataSource
self.delegate = delegate
}

func upload(_ log: LogFile, then callback: @escaping LogUploadCallback) {
guard delegate.shouldUploadLogFiles else {
delegate.uploadCancelledByDelegate(log)
return
}

guard let fileContents = fileManager.contents(atUrl: log.url) else {
delegate.uploadFailed(withError: EventLoggingFileUploadError.fileMissing, forLog: log)
return
}

var request = URLRequest(url: dataSource.logUploadURL)
request.addValue(log.uuid, forHTTPHeaderField: Constants.uuidHeaderKey)
request.httpMethod = Constants.uploadHttpMethod
request.httpBody = fileContents

delegate.didStartUploadingLog(log)

networkService.uploadFile(request: request, fileURL: log.url) { result in
switch(result) {
case .success:
self.delegate.didFinishUploadingLog(log)
callback(.success(()))
case .failure(let error):
self.delegate.uploadFailed(withError: error, forLog: log)
callback(.failure(error))
}
}
}
}
27 changes: 27 additions & 0 deletions Automattic-Tracks-iOS/Extensions/FileManager+Extensions.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import Foundation

extension FileManager {

var documentsDirectory: URL {
let documentsDirectory = NSSearchPathForDirectoriesInDomains(.documentDirectory, .userDomainMask, true).first!
return URL(fileURLWithPath: documentsDirectory, isDirectory: true)
}

func contentsOfDirectory(at url: URL) throws -> [URL] {
return try self.contentsOfDirectory(atPath: url.path).map { url.appendingPathComponent($0) }
}

func contents(atUrl url: URL) -> Data? {
return self.contents(atPath: url.path)
}

func fileExistsAtURL(_ url: URL) -> Bool {
return self.fileExists(atPath: url.path)
}

func directoryExistsAtURL(_ url: URL) -> Bool {
var isDir: ObjCBool = false
let exists = self.fileExists(atPath: url.path, isDirectory: &isDir)
return exists && isDir.boolValue
}
}
15 changes: 15 additions & 0 deletions Automattic-Tracks-iOS/Model/EventLoggingFileUploadError.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import Foundation

public enum EventLoggingFileUploadError: Error, LocalizedError {
case httpError(String)
case fileMissing

public var errorDescription: String? {
switch self {
case .httpError(let message): return message
case .fileMissing: return NSLocalizedString(
"File not found", comment: "A message indicating that a file queued for upload could not be found"
)
}
}
}
8 changes: 8 additions & 0 deletions Automattic-Tracks-iOS/Model/LogFile.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,12 @@ public struct LogFile {
self.url = url
self.uuid = uuid
}

var fileName: String {
return uuid
}

static func fromExistingFile(at url: URL) -> LogFile {
return LogFile(url: url, uuid: url.lastPathComponent)
}
}
34 changes: 34 additions & 0 deletions Automattic-Tracks-iOS/Services/EventLoggingNetworkService.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import Foundation

open class EventLoggingNetworkService {
oguzkocer marked this conversation as resolved.
Show resolved Hide resolved

typealias ResultCallback = (Result<Data?, Error>) -> Void
oguzkocer marked this conversation as resolved.
Show resolved Hide resolved

private let urlSession: URLSession

init(urlSession: URLSession = URLSession.shared) {
self.urlSession = urlSession
}

func uploadFile(request: URLRequest, fileURL: URL, completion: @escaping ResultCallback) {
urlSession.uploadTask(with: request, fromFile: fileURL, completionHandler: { data, response, error in

if let error = error {
completion(.failure(error))
return
}

/// The `response` should *always* be an HTTPURLResponse. Crash if notl
let statusCode = (response as! HTTPURLResponse).statusCode

/// Generate a reasonable error message based on the HTTP status
if !(200 ... 299).contains(statusCode) {
let errorMessage = HTTPURLResponse.localizedString(forStatusCode: statusCode)
completion(.failure(EventLoggingFileUploadError.httpError(errorMessage)))
return
}

completion(.success(data))
}).resume()
}
}
40 changes: 40 additions & 0 deletions Automattic-Tracks-iOS/Services/EventLoggingUploadQueue.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import Foundation

open class EventLoggingUploadQueue {
oguzkocer marked this conversation as resolved.
Show resolved Hide resolved

private let fileManager: FileManager
var storageDirectory: URL

init(storageDirectory: URL? = nil, fileManager: FileManager = FileManager.default) {
let defaultStorageDirectory = fileManager.documentsDirectory.appendingPathComponent("log-upload-queue")
self.storageDirectory = storageDirectory ?? defaultStorageDirectory
self.fileManager = fileManager
}

/// The log file on top of the queue
var first: LogFile? {
guard let url = try? fileManager.contentsOfDirectory(at: storageDirectory).first else {
return nil
}

return LogFile.fromExistingFile(at: url)
}

func add(_ log: LogFile) throws {
try createStorageDirectoryIfNeeded()
try fileManager.copyItem(at: log.url, to: storageDirectory.appendingPathComponent(log.fileName))
}

func remove(_ log: LogFile) throws {
let url = storageDirectory.appendingPathComponent(log.fileName)
if fileManager.fileExistsAtURL(url) {
try fileManager.removeItem(at: url)
}
}

func createStorageDirectoryIfNeeded() throws {
if !fileManager.directoryExistsAtURL(storageDirectory) {
try fileManager.createDirectory(at: storageDirectory, withIntermediateDirectories: true, attributes: nil)
}
}
}
9 changes: 9 additions & 0 deletions Automattic-Tracks-iOSTests/Test Helpers/Extensions.swift
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import Foundation
import Sodium
import XCTest

extension String {
static func randomString(length: Int) -> String {
Expand All @@ -26,3 +27,11 @@ extension FileManager {
return fileURL
}
}

extension XCTestCase {
func async_test(timeout: TimeInterval, _ block: (XCTestExpectation) -> ()) {
shiki marked this conversation as resolved.
Show resolved Hide resolved
let exp = XCTestExpectation()
block(exp)
wait(for: [exp], timeout: timeout)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,8 @@ extension LogFile {
static func containingRandomString(length: Int = 128) -> LogFile {
return LogFile(containing: String.randomString(length: length))
}

static func withInvalidPath() -> LogFile {
return LogFile(url: URL(fileURLWithPath: "invalid"))
}
}
107 changes: 107 additions & 0 deletions Automattic-Tracks-iOSTests/Test Helpers/Mocks.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
import Foundation
import Sodium
@testable import AutomatticTracks

typealias LogFileCallback = (LogFile) -> ()
typealias ErrorWithLogFileCallback = (Error, LogFile) -> ()

enum MockError: Error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we use a more specific name for the file? Since I can't comment on the file itself, I am adding it here.

case generic
}

class MockEventLoggingDataSource: EventLoggingDataSource {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the way this class is written is totally fine for tests, but here is how I would have written it myself: 8723261.

The idea is to promote immutability and control how this class can be used. It has 0 value in this PR in practice, but I thought I'd take the time to mention this in a couple places in this PR for future reference, only if you find it useful of course.


var loggingEncryptionKey: String = "foo"
var previousSessionLogPath: URL? = nil

/// Overrides for logUploadURL
var _logUploadURL: URL = URL(string: "example.com")!
var logUploadURL: URL {
return _logUploadURL
}

func setLogUploadUrl(_ url: URL) {
self._logUploadURL = url
}

func withEncryptionKeys() -> Self {
let keyPair = Sodium().box.keyPair()!
loggingEncryptionKey = Data(keyPair.publicKey).base64EncodedString()
return self
}
}

class MockEventLoggingDelegate: EventLoggingDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very similar to my last comment, here is how I'd have written this class: c3debf1


var didStartUploadingTriggered = false
var didStartUploadingCallback: LogFileCallback?

func didStartUploadingLog(_ log: LogFile) {
didStartUploadingTriggered = true
didStartUploadingCallback?(log)
}

var didFinishUploadingTriggered = false
var didFinishUploadingCallback: LogFileCallback?

func didFinishUploadingLog(_ log: LogFile) {
didFinishUploadingTriggered = true
didFinishUploadingCallback?(log)
}

var uploadCancelledByDelegateTriggered = false
var uploadCancelledByDelegateCallback: LogFileCallback?

func uploadCancelledByDelegate(_ log: LogFile) {
uploadCancelledByDelegateTriggered = true
uploadCancelledByDelegateCallback?(log)
}

var uploadFailedTriggered = false
var uploadFailedCallback: ErrorWithLogFileCallback?

func uploadFailed(withError error: Error, forLog log: LogFile) {
uploadFailedTriggered = true
uploadFailedCallback?(error, log)
}

func setShouldUploadLogFiles(_ newValue: Bool) {
_shouldUploadLogFiles = newValue
}

private var _shouldUploadLogFiles: Bool = true
var shouldUploadLogFiles: Bool {
return _shouldUploadLogFiles
}
}

class MockEventLoggingNetworkService: EventLoggingNetworkService {
var shouldSucceed = true

override func uploadFile(request: URLRequest, fileURL: URL, completion: @escaping EventLoggingNetworkService.ResultCallback) {
shouldSucceed ? completion(.success(Data())) : completion(.failure(MockError.generic))
}
}

class MockEventLoggingUploadQueue: EventLoggingUploadQueue {

typealias LogFileCallback = (LogFile) -> ()

var queue = [LogFile]()

var addCallback: LogFileCallback?
override func add(_ log: LogFile) {
self.addCallback?(log)
self.queue.append(log)
}

var removeCallback: LogFileCallback?
override func remove(_ log: LogFile) {
self.removeCallback?(log)
self.queue.removeAll(where: { $0.uuid == log.uuid })
}

override var first: LogFile? {
return self.queue.first
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Foundation
import OHHTTPStubs

func stubResponse(domain: String, status: String, statusCode: Int32 = 200) {
stub(condition: isHost(domain)) { _ in
let stubData = "{status: \"\(status)\"}".data(using: .utf8)!
return HTTPStubsResponse(data: stubData, statusCode: statusCode, headers: nil)
}
}
Loading