diff --git a/ElementX/Sources/Application/AppSettings.swift b/ElementX/Sources/Application/AppSettings.swift index 9a1698e35..5387d8985 100644 --- a/ElementX/Sources/Application/AppSettings.swift +++ b/ElementX/Sources/Application/AppSettings.swift @@ -125,6 +125,8 @@ final class AppSettings { // Use the name allocated by the bug report server let bugReportApplicationId = "element-x-ios" let bugReportUISIId = "element-auto-uisi" + /// The maximum size of the upload request. Default value is just below CloudFlare's max request size. + let bugReportMaxUploadSize = 50 * 1024 * 1024 // MARK: - Analytics diff --git a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift index 7aac6594a..2b19b4cba 100644 --- a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift +++ b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift @@ -172,25 +172,21 @@ class BugReportServiceMock: BugReportServiceProtocol { } //MARK: - submitBugReport - var submitBugReportProgressListenerThrowableError: Error? var submitBugReportProgressListenerCallsCount = 0 var submitBugReportProgressListenerCalled: Bool { return submitBugReportProgressListenerCallsCount > 0 } var submitBugReportProgressListenerReceivedArguments: (bugReport: BugReport, progressListener: ProgressListener?)? var submitBugReportProgressListenerReceivedInvocations: [(bugReport: BugReport, progressListener: ProgressListener?)] = [] - var submitBugReportProgressListenerReturnValue: SubmitBugReportResponse! - var submitBugReportProgressListenerClosure: ((BugReport, ProgressListener?) async throws -> SubmitBugReportResponse)? + var submitBugReportProgressListenerReturnValue: Result! + var submitBugReportProgressListenerClosure: ((BugReport, ProgressListener?) async -> Result)? - func submitBugReport(_ bugReport: BugReport, progressListener: ProgressListener?) async throws -> SubmitBugReportResponse { - if let error = submitBugReportProgressListenerThrowableError { - throw error - } + func submitBugReport(_ bugReport: BugReport, progressListener: ProgressListener?) async -> Result { submitBugReportProgressListenerCallsCount += 1 submitBugReportProgressListenerReceivedArguments = (bugReport: bugReport, progressListener: progressListener) submitBugReportProgressListenerReceivedInvocations.append((bugReport: bugReport, progressListener: progressListener)) if let submitBugReportProgressListenerClosure = submitBugReportProgressListenerClosure { - return try await submitBugReportProgressListenerClosure(bugReport, progressListener) + return await submitBugReportProgressListenerClosure(bugReport, progressListener) } else { return submitBugReportProgressListenerReturnValue } diff --git a/ElementX/Sources/Other/Logging/MXLogger.swift b/ElementX/Sources/Other/Logging/MXLogger.swift index 81e907767..eccbf53a1 100644 --- a/ElementX/Sources/Other/Logging/MXLogger.swift +++ b/ElementX/Sources/Other/Logging/MXLogger.swift @@ -126,23 +126,29 @@ class MXLogger { } } - /// The list of all log file URLs. + /// The list of all log file URLs, sorted chronologically. static var logFiles: [URL] { - var logFiles = [URL]() + var logFiles = [(url: URL, modificationDate: Date)]() let fileManager = FileManager.default - let enumerator = fileManager.enumerator(at: logsFolderURL, includingPropertiesForKeys: nil) + let enumerator = fileManager.enumerator(at: logsFolderURL, includingPropertiesForKeys: [.contentModificationDateKey]) - // Find all *.log files + // Find all *.log files and their modification dates. while let logURL = enumerator?.nextObject() as? URL { - if logURL.lastPathComponent.hasPrefix("console") { - logFiles.append(logURL) + guard let resourceValues = try? logURL.resourceValues(forKeys: [.contentModificationDateKey]), + let modificationDate = resourceValues.contentModificationDate + else { continue } + + if logURL.pathExtension == "log" { + logFiles.append((logURL, modificationDate)) } } - MXLog.info("logFiles: \(logFiles)") + let sortedFiles = logFiles.sorted { $0.modificationDate > $1.modificationDate }.map(\.url) - return logFiles + MXLog.info("logFiles: \(sortedFiles.map(\.lastPathComponent))") + + return sortedFiles } // MARK: - Exceptions and crashes diff --git a/ElementX/Sources/Screens/BugReportScreen/BugReportScreenCoordinator.swift b/ElementX/Sources/Screens/BugReportScreen/BugReportScreenCoordinator.swift index 99dfbd01e..0acdd9201 100644 --- a/ElementX/Sources/Screens/BugReportScreen/BugReportScreenCoordinator.swift +++ b/ElementX/Sources/Screens/BugReportScreen/BugReportScreenCoordinator.swift @@ -100,6 +100,6 @@ final class BugReportScreenCoordinator: CoordinatorProtocol { } private func showError(label: String) { - parameters.userIndicatorController?.submitIndicator(UserIndicator(title: label)) + parameters.userIndicatorController?.submitIndicator(UserIndicator(title: label, iconName: "xmark")) } } diff --git a/ElementX/Sources/Screens/BugReportScreen/BugReportScreenViewModel.swift b/ElementX/Sources/Screens/BugReportScreen/BugReportScreenViewModel.swift index 9cf862d30..cc07ae77b 100644 --- a/ElementX/Sources/Screens/BugReportScreen/BugReportScreenViewModel.swift +++ b/ElementX/Sources/Screens/BugReportScreen/BugReportScreenViewModel.swift @@ -64,27 +64,35 @@ class BugReportScreenViewModel: BugReportScreenViewModelType, BugReportScreenVie private func submitBugReport() async { let progressTracker = ProgressTracker() actionsSubject.send(.submitStarted(progressTracker: progressTracker)) - do { - var files: [URL] = [] - if let screenshot = context.viewState.screenshot { - let imageURL = URL.temporaryDirectory.appendingPathComponent("Screenshot.png") - let pngData = screenshot.pngData() - try pngData?.write(to: imageURL) + + var files: [URL] = [] + if let screenshot = context.viewState.screenshot, + let pngData = screenshot.pngData() { + let imageURL = URL.temporaryDirectory.appendingPathComponent("Screenshot.png") + + do { + try pngData.write(to: imageURL) files.append(imageURL) + } catch { + MXLog.error("Failed writing screenshot to disk") + // Continue anyway without the screenshot. } - let bugReport = BugReport(userID: userID, - deviceID: deviceID, - text: context.reportText, - includeLogs: context.sendingLogsEnabled, - includeCrashLog: true, - githubLabels: [], - files: files) - let result = try await bugReportService.submitBugReport(bugReport, - progressListener: progressTracker) - MXLog.info("SubmitBugReport succeeded, result: \(result.reportUrl)") + } + let bugReport = BugReport(userID: userID, + deviceID: deviceID, + text: context.reportText, + includeLogs: context.sendingLogsEnabled, + includeCrashLog: true, + githubLabels: [], + files: files) + + switch await bugReportService.submitBugReport(bugReport, + progressListener: progressTracker) { + case .success(let response): + MXLog.info("Submission uploaded to: \(response.reportUrl)") actionsSubject.send(.submitFinished) - } catch { - MXLog.error("SubmitBugReport failed: \(error)") + case .failure(let error): + MXLog.error("Submission failed: \(error)") actionsSubject.send(.submitFailed(error: error)) } } diff --git a/ElementX/Sources/Services/BugReport/BugReportService.swift b/ElementX/Sources/Services/BugReport/BugReportService.swift index b7b3e5aa6..1aea713ea 100644 --- a/ElementX/Sources/Services/BugReport/BugReportService.swift +++ b/ElementX/Sources/Services/BugReport/BugReportService.swift @@ -24,6 +24,7 @@ class BugReportService: NSObject, BugReportServiceProtocol { private let baseURL: URL private let sentryURL: URL private let applicationId: String + private let maxUploadSize: Int private let session: URLSession private var lastCrashEventId: String? private let progressSubject = PassthroughSubject() @@ -32,10 +33,12 @@ class BugReportService: NSObject, BugReportServiceProtocol { init(withBaseURL baseURL: URL, sentryURL: URL, applicationId: String = ServiceLocator.shared.settings.bugReportApplicationId, + maxUploadSize: Int = ServiceLocator.shared.settings.bugReportMaxUploadSize, session: URLSession = .shared) { self.baseURL = baseURL self.sentryURL = sentryURL self.applicationId = applicationId + self.maxUploadSize = maxUploadSize self.session = session super.init() @@ -95,8 +98,9 @@ class BugReportService: NSObject, BugReportServiceProtocol { SentrySDK.crash() } - // swiftlint:disable:next function_body_length - func submitBugReport(_ bugReport: BugReport, progressListener: ProgressListener?) async throws -> SubmitBugReportResponse { + // swiftlint:disable:next function_body_length cyclomatic_complexity + func submitBugReport(_ bugReport: BugReport, + progressListener: ProgressListener?) async -> Result { var params = [ MultipartFormData(key: "user_id", type: .text(value: bugReport.userID)), MultipartFormData(key: "text", type: .text(value: bugReport.text)) @@ -111,13 +115,11 @@ class BugReportService: NSObject, BugReportServiceProtocol { for label in bugReport.githubLabels { params.append(MultipartFormData(key: "label", type: .text(value: label))) } - let zippedFiles = try await zipFiles(includeLogs: bugReport.includeLogs, - includeCrashLog: bugReport.includeCrashLog) - // log or compressed-log - if !zippedFiles.isEmpty { - for url in zippedFiles { - params.append(MultipartFormData(key: "compressed-log", type: .file(url: url))) - } + let logAttachments = await zipFiles(includeLogs: bugReport.includeLogs, + includeCrashLog: bugReport.includeCrashLog) + + for url in logAttachments.files { + params.append(MultipartFormData(key: "compressed-log", type: .file(url: url))) } if let crashEventId = lastCrashEventId { @@ -131,7 +133,12 @@ class BugReportService: NSObject, BugReportServiceProtocol { let boundary = "Boundary-\(UUID().uuidString)" var body = Data() for param in params { - try body.appendParam(param, boundary: boundary) + do { + try body.appendParam(param, boundary: boundary) + } catch { + MXLog.error("Failed to attach parameter at \(param.key)") + // Continue to the next parameter and try to submit something. + } } body.appendString(string: "--\(boundary)--\r\n") @@ -141,30 +148,48 @@ class BugReportService: NSObject, BugReportServiceProtocol { request.httpMethod = "POST" request.httpBody = body as Data - let data: Data + var delegate: URLSessionTaskDelegate? if let progressListener { progressSubject .receive(on: DispatchQueue.main) .weakAssign(to: \.value, on: progressListener.progressSubject) .store(in: &cancellables) - (data, _) = try await session.data(for: request, delegate: self) - } else { - (data, _) = try await session.data(for: request) - } - - // Parse the JSON data - let decoder = JSONDecoder() - decoder.keyDecodingStrategy = .convertFromSnakeCase - let result = try decoder.decode(SubmitBugReportResponse.self, from: data) - - if !result.reportUrl.isEmpty { - MXLogger.deleteCrashLog() - lastCrashEventId = nil + delegate = self } - MXLog.info("Feedback submitted.") - - return result + do { + let (data, response) = try await session.dataWithRetry(for: request, delegate: delegate) + + guard let httpResponse = response as? HTTPURLResponse else { + let errorDescription = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "Unknown" + MXLog.error("Failed to submit bug report: \(errorDescription)") + MXLog.error("Response: \(response)") + return .failure(.serverError(response, errorDescription)) + } + + guard httpResponse.statusCode == 200 else { + let errorDescription = String(data: data, encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "Unknown" + MXLog.error("Failed to submit bug report: \(errorDescription) (\(httpResponse.statusCode))") + MXLog.error("Response: \(httpResponse)") + return .failure(.httpError(httpResponse, errorDescription)) + } + + // Parse the JSON data + let decoder = JSONDecoder() + decoder.keyDecodingStrategy = .convertFromSnakeCase + let uploadResponse = try decoder.decode(SubmitBugReportResponse.self, from: data) + + if !uploadResponse.reportUrl.isEmpty { + MXLogger.deleteCrashLog() + lastCrashEventId = nil + } + + MXLog.info("Feedback submitted.") + + return .success(uploadResponse) + } catch { + return .failure(.uploadFailure(error)) + } } // MARK: - Private @@ -200,7 +225,7 @@ class BugReportService: NSObject, BugReportServiceProtocol { } private func zipFiles(includeLogs: Bool, - includeCrashLog: Bool) async throws -> [URL] { + includeCrashLog: Bool) async -> Logs { MXLog.info("zipFiles: includeLogs: \(includeLogs), includeCrashLog: \(includeCrashLog)") var filesToCompress: [URL] = [] @@ -210,37 +235,69 @@ class BugReportService: NSObject, BugReportServiceProtocol { if includeCrashLog, let crashLogFile = MXLogger.crashLog { filesToCompress.append(crashLogFile) } - - var totalSize = 0 - var totalZippedSize = 0 - var zippedFiles: [URL] = [] - + + var compressedLogs = Logs(maxFileSize: maxUploadSize) + for url in filesToCompress { - let zippedFileURL = URL.temporaryDirectory - .appendingPathComponent(url.lastPathComponent) - - // remove old zipped file if exists - try? FileManager.default.removeItem(at: zippedFileURL) - - let rawData = try Data(contentsOf: url) - if rawData.isEmpty { - continue + do { + try attachFile(at: url, to: &compressedLogs) + } catch { + MXLog.error("Failed to compress log at \(url)") + // Continue so that other logs can still be sent. } - guard let zippedData = (rawData as NSData).gzipped() else { - continue - } - - totalSize += rawData.count - totalZippedSize += zippedData.count - - try zippedData.write(to: zippedFileURL) - - zippedFiles.append(zippedFileURL) } + + MXLog.info("zipFiles: originalSize: \(compressedLogs.originalSize), zippedSize: \(compressedLogs.zippedSize)") - MXLog.info("zipFiles: totalSize: \(totalSize), totalZippedSize: \(totalZippedSize)") - - return zippedFiles + return compressedLogs + } + + /// Zips a file creating chunks based on 10MB inputs. + private func attachFile(at url: URL, to zippedFiles: inout Logs) throws { + let fileHandle = try FileHandle(forReadingFrom: url) + + var chunkIndex = -1 + while let data = try fileHandle.read(upToCount: 10 * 1024 * 1024) { + do { + chunkIndex += 1 + if let zippedData = (data as NSData).gzipped() { + let zippedFilename = url.deletingPathExtension().lastPathComponent + "_\(chunkIndex).log" + let chunkURL = URL.temporaryDirectory.appending(path: zippedFilename) + + // Remove old zipped file if exists + try? FileManager.default.removeItem(at: chunkURL) + + try zippedData.write(to: chunkURL) + zippedFiles.appendFile(at: chunkURL, zippedSize: zippedData.count, originalSize: data.count) + } + } catch { + MXLog.error("Failed attaching log chunk \(chunkIndex) from (\(url.lastPathComponent)") + continue + } + } + } + + /// A collection of logs to be uploaded to the bug report service. + struct Logs { + /// The maximum total size of all the files. + let maxFileSize: Int + + /// The files included. + private(set) var files: [URL] = [] + /// The total size of the files after compression. + private(set) var zippedSize = 0 + /// The original size of the files. + private(set) var originalSize = 0 + + mutating func appendFile(at url: URL, zippedSize: Int, originalSize: Int) { + guard self.zippedSize + zippedSize < maxFileSize else { + MXLog.error("Logs too large, skipping attachment: \(url.lastPathComponent)") + return + } + files.append(url) + self.originalSize += originalSize + self.zippedSize += zippedSize + } } } @@ -285,3 +342,14 @@ extension BugReportService: URLSessionTaskDelegate { .store(in: &cancellables) } } + +private extension URLSession { + /// The same as `data(for:delegate:)` but with an additional immediate retry if the first attempt fails. + func dataWithRetry(for request: URLRequest, delegate: URLSessionTaskDelegate? = nil) async throws -> (Data, URLResponse) { + if let firstTryResult = try? await data(for: request, delegate: delegate) { + return firstTryResult + } + + return try await data(for: request, delegate: delegate) + } +} diff --git a/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift b/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift index 445d5e09b..7b43c2a3a 100644 --- a/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift +++ b/ElementX/Sources/Services/BugReport/BugReportServiceProtocol.swift @@ -31,6 +31,23 @@ struct SubmitBugReportResponse: Decodable { var reportUrl: String } +enum BugReportServiceError: LocalizedError { + case uploadFailure(Error) + case serverError(URLResponse, String) + case httpError(HTTPURLResponse, String) + + var errorDescription: String? { + switch self { + case .uploadFailure(let error): + return error.localizedDescription + case .serverError(_, let errorDescription): + return errorDescription + case .httpError(_, let errorDescription): + return errorDescription + } + } +} + // sourcery: AutoMockable protocol BugReportServiceProtocol { var isRunning: Bool { get } @@ -46,5 +63,5 @@ protocol BugReportServiceProtocol { func crash() func submitBugReport(_ bugReport: BugReport, - progressListener: ProgressListener?) async throws -> SubmitBugReportResponse + progressListener: ProgressListener?) async -> Result } diff --git a/UnitTests/Sources/BugReportServiceTests.swift b/UnitTests/Sources/BugReportServiceTests.swift index 9e8e50e35..3b06100c0 100644 --- a/UnitTests/Sources/BugReportServiceTests.swift +++ b/UnitTests/Sources/BugReportServiceTests.swift @@ -24,7 +24,7 @@ class BugReportServiceTests: XCTestCase { override func setUpWithError() throws { bugReportService = BugReportServiceMock() bugReportService.underlyingCrashedLastRun = false - bugReportService.submitBugReportProgressListenerReturnValue = SubmitBugReportResponse(reportUrl: "https://www.example.com/123") + bugReportService.submitBugReportProgressListenerReturnValue = .success(SubmitBugReportResponse(reportUrl: "https://www.example.com/123")) } func testInitialStateWithMockService() { @@ -39,8 +39,8 @@ class BugReportServiceTests: XCTestCase { includeCrashLog: true, githubLabels: [], files: []) - let result = try await bugReportService.submitBugReport(bugReport, progressListener: nil) - XCTAssertFalse(result.reportUrl.isEmpty) + let response = try await bugReportService.submitBugReport(bugReport, progressListener: nil).get() + XCTAssertFalse(response.reportUrl.isEmpty) } func testInitialStateWithRealService() throws { @@ -64,17 +64,43 @@ class BugReportServiceTests: XCTestCase { includeCrashLog: true, githubLabels: [], files: []) - let result = try await service.submitBugReport(bugReport, progressListener: nil) + let response = try await service.submitBugReport(bugReport, progressListener: nil).get() - XCTAssertEqual(result.reportUrl, "https://example.com/123") + XCTAssertEqual(response.reportUrl, "https://example.com/123") + } + + func testLogsMaxSize() { + // Given a new set of logs + var logs = BugReportService.Logs(maxFileSize: 1000) + XCTAssertEqual(logs.zippedSize, 0) + XCTAssertEqual(logs.originalSize, 0) + XCTAssertTrue(logs.files.isEmpty) + + // When adding new files within the size limit + logs.appendFile(at: .homeDirectory, zippedSize: 250, originalSize: 1000) + logs.appendFile(at: .picturesDirectory, zippedSize: 500, originalSize: 2000) + + // Then the logs should be included + XCTAssertEqual(logs.zippedSize, 750) + XCTAssertEqual(logs.originalSize, 3000) + XCTAssertEqual(logs.files, [.homeDirectory, .picturesDirectory]) + + // When adding a new file larger that will exceed the size limit + logs.appendFile(at: .homeDirectory, zippedSize: 500, originalSize: 2000) + + // Then the files shouldn't be included. + XCTAssertEqual(logs.zippedSize, 750) + XCTAssertEqual(logs.originalSize, 3000) + XCTAssertEqual(logs.files, [.homeDirectory, .picturesDirectory]) } } private class MockURLProtocol: URLProtocol { override func startLoading() { let response = "{\"report_url\":\"https://example.com/123\"}" - if let data = response.data(using: .utf8) { - let urlResponse = URLResponse() + if let data = response.data(using: .utf8), + let url = request.url, + let urlResponse = HTTPURLResponse(url: url, statusCode: 200, httpVersion: nil, headerFields: nil) { client?.urlProtocol(self, didReceive: urlResponse, cacheStoragePolicy: .allowedInMemoryOnly) client?.urlProtocol(self, didLoad: data) client?.urlProtocolDidFinishLoading(self) diff --git a/UnitTests/Sources/BugReportViewModelTests.swift b/UnitTests/Sources/BugReportViewModelTests.swift index d398afbbb..2ce60b07f 100644 --- a/UnitTests/Sources/BugReportViewModelTests.swift +++ b/UnitTests/Sources/BugReportViewModelTests.swift @@ -64,7 +64,7 @@ class BugReportViewModelTests: XCTestCase { let mockService = BugReportServiceMock() mockService.submitBugReportProgressListenerClosure = { _, _ in await Task.yield() - return SubmitBugReportResponse(reportUrl: "https://test.test") + return .success(SubmitBugReportResponse(reportUrl: "https://test.test")) } let viewModel = BugReportScreenViewModel(bugReportService: mockService, userID: "@mock.client.com", @@ -90,7 +90,7 @@ class BugReportViewModelTests: XCTestCase { func testSendReportWithError() async throws { let mockService = BugReportServiceMock() mockService.submitBugReportProgressListenerClosure = { _, _ in - throw TestError.testError + .failure(.uploadFailure(TestError.testError)) } let viewModel = BugReportScreenViewModel(bugReportService: mockService, userID: "@mock.client.com", diff --git a/UnitTests/Sources/LoggingTests.swift b/UnitTests/Sources/LoggingTests.swift index 0ad986ccc..83a67c901 100644 --- a/UnitTests/Sources/LoggingTests.swift +++ b/UnitTests/Sources/LoggingTests.swift @@ -161,6 +161,61 @@ class LoggingTests: XCTestCase { XCTAssertTrue(logFile.lastPathComponent.contains(target)) } + func testLogFileSorting() async throws { + // Given a collection of log files. + XCTAssertTrue(MXLogger.logFiles.isEmpty) + + // When creating new logs. + let logsFileDirectory = URL.appGroupContainerDirectory + for i in 1...5 { + let filename = "console.\(i).log" + try "console".write(to: logsFileDirectory.appending(path: filename), atomically: true, encoding: .utf8) + } + + for i in 1...5 { + let nseFilename = "console-nse.\(i).log" + try "nse".write(to: logsFileDirectory.appending(path: nseFilename), atomically: true, encoding: .utf8) + } + + // Then the logs should be sorted chronologically (newest first) and not alphabetically. + XCTAssertEqual(MXLogger.logFiles.map(\.lastPathComponent), + ["console-nse.5.log", + "console-nse.4.log", + "console-nse.3.log", + "console-nse.2.log", + "console-nse.1.log", + "console.5.log", + "console.4.log", + "console.3.log", + "console.2.log", + "console.1.log"]) + + // When updating the oldest log file. + let currentLogFile = logsFileDirectory.appending(path: "console.1.log") + let fileHandle = try FileHandle(forWritingTo: currentLogFile) + try fileHandle.seekToEnd() + guard let newLineData = "newline".data(using: .utf8) else { + XCTFail("Couldn't create data to write to disk.") + return + } + + try fileHandle.write(contentsOf: newLineData) + try fileHandle.close() + + // Then that file should now be the first log file. + XCTAssertEqual(MXLogger.logFiles.map(\.lastPathComponent), + ["console.1.log", + "console-nse.5.log", + "console-nse.4.log", + "console-nse.3.log", + "console-nse.2.log", + "console-nse.1.log", + "console.5.log", + "console.4.log", + "console.3.log", + "console.2.log"]) + } + func testRoomSummaryContentIsRedacted() throws { // Given a room summary that contains sensitive information let roomName = "Private Conversation" diff --git a/changelog.d/pr-1018.change b/changelog.d/pr-1018.change new file mode 100644 index 000000000..6ed9015e8 --- /dev/null +++ b/changelog.d/pr-1018.change @@ -0,0 +1 @@ +Improve bug report uploads with file size checks and better error handling. \ No newline at end of file