Update head

[JoeMatt]

User description

What does this PR do?

Where should the reviewer start?

How should this be manually tested?

Any background context you want to provide?

What are the relevant tickets?

Screenshots (if appropriate)

Questions

---

PR Type

enhancement, bug_fix

---

Description

• Refactored RecordController for improved error handling and logging using OSLog.
• Enhanced SyncRecordsOperation with new operations for seeding and repairing records.
• Introduced RepairRecordOperation and RepairRecordsOperation for fixing mismatched record hashes.
• Added SeedRecordControllerOperation to ensure initial sync setup.
• Updated Core Data model to version 3 with new attributes and relationships.
• Improved error handling across various operations with detailed logging.
• Updated platform version to iOS 14 and GoogleSignIn dependency to version 5.0.

---

Changes walkthrough 📝

	Relevant files
Enhancement	13 files RecordController.swift Refactor RecordController for improved error handling and logging Harmony/Core Data/RecordController.swift Introduced harmonyPersistentStore for better store management. Added logging using OSLog for error handling and debugging. Refactored seeding logic and migration handling. Improved error handling and logging for database operations. +263/-118 SyncRecordsOperation.swift Enhance SyncRecordsOperation with new operations and logging Harmony/Operations/SyncRecordsOperation.swift Removed changeToken from initializer. Added new operations for seeding and repairing records. Enhanced logging for operation results. Improved error handling and progress tracking. +102/-55 DownloadRecordOperation.swift Improve DownloadRecordOperation with metadata flag and file verification Harmony/Operations/Download/DownloadRecordOperation.swift Added downloadRecordMetadataOnly flag. Implemented verification of remote files to remove duplicates. Enhanced logging for download operations. +147/-28 RepairRecordOperation.swift Add RepairRecordOperation for fixing mismatched record hashes Harmony/Operations/Repair/RepairRecordOperation.swift Introduced new operation to repair records with mismatched hashes. Handles records with missing local versions. +152/-0  SyncCoordinator.swift Refactor SyncCoordinator for better error logging and authentication Harmony/SyncCoordinator.swift Made managedAccount internal. Improved error logging using OSLog. Refactored authentication and sync operations. +62/-36  Errors.swift Extend error handling with new cases and descriptions        Harmony/Types/Errors.swift Added new error cases for database and service errors. Enhanced error handling with detailed failure descriptions. +12/-0    FinishDownloadingRecordsOperation.swift Enhance FinishDownloadingRecordsOperation with metadata handling Harmony/Operations/Download/FinishDownloadingRecordsOperation.swift Added handling for metadata-only records. Improved error logging for file operations. +25/-16  LocalRecord.swift Update LocalRecord with metadata flag and public modification date Harmony/Model/Core Data/LocalRecord.swift Made modificationDate publicly accessible. Introduced isMetadataOnly property. Improved logging for object resolution errors. +25/-10  SeedRecordControllerOperation.swift Add SeedRecordControllerOperation for initial sync setup  Harmony/Operations/Misc./SeedRecordControllerOperation.swift Added new operation to seed the record controller. Ensures initial sync setup is complete. +70/-0    RemoteFile.swift Enhance RemoteFile with ID and Values structs                        Harmony/Model/Core Data/RemoteFile.swift Introduced ID and Values structs for better file management. Added convenience initializer for RemoteFile. +35/-1    OSLog+Harmony.swift Introduce OSLog extensions for centralized logging              Harmony/Extensions/OSLog+Harmony.swift Added logging categories for migration and sync. Provided a centralized logging setup for Harmony. +25/-0    RepairRecordsOperation.swift Add RepairRecordsOperation for batch record repair              Harmony/Operations/Repair/RepairRecordsOperation.swift Added batch operation for repairing records. Uses new predicate for identifying records needing repair. +25/-0    contents Update Core Data model to version 3 with new attributes    Harmony/Model/Core Data/Harmony.xcdatamodeld/Harmony 3.xcdatamodel/contents Updated Core Data model to version 3. Added new attributes and relationships for records. +98/-0
Dependencies	1 files Harmony.podspec Update podspec for iOS 14 and GoogleSignIn 5.0                      Harmony.podspec Updated platform version to iOS 14.0. Updated GoogleSignIn dependency to version 5.0. +2/-2
Additional files (token-limit)	1 files xcmapping.xml ...                                                                                                            Harmony/Model/Core Data/Migrations/Harmony2ToHarmony3.xcmappingmodel/xcmapping.xml ... +285/-1

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[codiumai-pr-agent-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review Potential Performance Issue The new migrateLocalRecordURIsIfNeeded method performs complex operations and database queries that may impact performance, especially for large datasets. Consider optimizing or batching these operations. Possible Bug The new verifyRemoteFiles method attempts to fix duplicate file entries, but it may not handle all edge cases correctly. Thorough testing is needed to ensure this doesn't introduce new issues. Code Complexity The main method has been significantly expanded with new operations and error handling. This increased complexity may make the code harder to maintain and debug.

[codiumai-pr-agent-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Category	Suggestion	Score
Performance	Use Set operations to efficiently update the remoteFiles set Consider using a more efficient approach to update the remoteFiles set by using Set operations instead of iterating through each file individually and manually adding/removing them. Harmony/Core Data/MergePolicy.swift [94-111] -var remoteFilesByID = [RemoteFile.ID: RemoteFile]() +let expectedRemoteFiles = Set(databaseObject.remoteFiles.filter { expectedRemoteFileIDs.contains($0.fileID) }) +let filesToDelete = databaseObject.remoteFiles.subtracting(expectedRemoteFiles) -for remoteFile in databaseObject.remoteFiles -{ - if expectedRemoteFileIDs.contains(remoteFile.fileID), !remoteFilesByID.keys.contains(remoteFile.fileID) - { - // File is expected, and there is not another file with same identifier, so we can keep it. - remoteFilesByID[remoteFile.fileID] = remoteFile - } - else - { - // Set localRecord to nil for all databaseObject.remoteFiles that are duplicates or not in expectedRemoteFileIDs so that they will be deleted. - remoteFile.localRecord = nil - remoteFile.managedObjectContext?.delete(remoteFile) - } +for remoteFile in filesToDelete { + remoteFile.localRecord = nil + remoteFile.managedObjectContext?.delete(remoteFile) } -databaseObject.remoteFiles = Set(remoteFilesByID.values) +databaseObject.remoteFiles = expectedRemoteFiles Apply this suggestion Suggestion importance[1-10]: 9 Why: This suggestion significantly optimizes the code by using Set operations, which are more efficient than manual iteration and manipulation, thus improving performance and maintainability.	9
	Optimize the fetch request predicate for better performance with large datasets Consider using a more efficient approach to fetch records with specific URIs. Instead of using an array of predicates, you can use a single predicate with the 'IN' operator, which is more performant for large datasets. Harmony/Core Data/RecordController.swift [621] -fetchRequest.predicate = NSPredicate(format: "%K IN %@", #keyPath(LocalRecord.recordedObjectURI), recordedObjectURIs) +fetchRequest.predicate = NSPredicate(format: "%K IN %@", #keyPath(LocalRecord.recordedObjectURI), recordedObjectURIs as NSArray) Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion to use 'NSArray' with the 'IN' operator in the predicate is a valid optimization for handling large datasets, improving performance by reducing the complexity of the predicate evaluation.	8
	Use a more efficient data structure for storing and accessing remote files Consider using a more efficient data structure for remoteFiles. Instead of using a Set, you could use a dictionary with the file identifier as the key for faster lookups and to avoid potential duplicates. Harmony/Operations/Download/DownloadRecordOperation.swift [302] -var remoteFiles = Set<RemoteFile>() +var remoteFiles = [String: RemoteFile]() Apply this suggestion Suggestion importance[1-10]: 7 Why: Using a dictionary instead of a set for storing remote files can improve lookup efficiency and help manage duplicates, which is beneficial for performance, especially when handling large datasets.	7
	Simplify the logic for checking and updating remote files Consider using a more efficient approach to update the remoteFiles set by using a Set operation instead of iterating through each file individually. Harmony/Operations/Upload/UploadRecordOperation.swift [177-192] -if let remoteFile = remoteFilesByIdentifier[file.identifier] -{ - // There is already an uploaded file with this identifier, so compare hashes. +if let remoteFile = remoteFilesByIdentifier[file.identifier], remoteFile.sha1Hash == hash { + // Hash is the same, so don't upload file. + self.progress.completedUnitCount += 1 - guard remoteFile.sha1Hash != hash else { - // Hash is the same, so don't upload file. - self.progress.completedUnitCount += 1 - - self.managedObjectContext.performAndWait { - let remoteFile = remoteFile.in(self.managedObjectContext) - remoteFiles.insert(remoteFile) - } - - continue + self.managedObjectContext.performAndWait { + let remoteFile = remoteFile.in(self.managedObjectContext) + remoteFiles.insert(remoteFile) } + + continue } Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion streamlines the code by combining conditions, which enhances readability and slightly improves performance by reducing unnecessary checks.	7
Maintainability	Refactor the complex method into smaller, more focused methods to improve readability and maintainability The verifyRemoteFiles method is quite complex and performs multiple operations. Consider breaking it down into smaller, more focused methods to improve readability and maintainability. Harmony/Operations/Download/DownloadRecordOperation.swift [295-391] -func verifyRemoteFiles(for localRecord: LocalRecord, completion: @escaping (Result<Void, RecordError>) -> Void) -{ - // Due to a previous bug, some records may have been uploaded with duplicate file entries. - // To handle this, we check all downloaded records to see if they contain duplicate files, - // and if they do we choose the "correct" version by comparing against all file versions, - // then finally we remove the "wrong" duplicates. +func verifyRemoteFiles(for localRecord: LocalRecord, completion: @escaping (Result<Void, RecordError>) -> Void) { + let remoteFilesGroupedByRemoteID = Dictionary(grouping: localRecord.remoteFiles) { $0.identifier } + let dispatchGroup = DispatchGroup() var remoteFiles = Set<RemoteFile>() var errors = [FileError]() - let dispatchGroup = DispatchGroup() + for (_, duplicateFiles) in remoteFilesGroupedByRemoteID { + handleDuplicateFiles(duplicateFiles, dispatchGroup: dispatchGroup, remoteFiles: &remoteFiles, errors: &errors) + } - // Group all files with same identifier together so we can fix duplicate entries. - let remoteFilesGroupedByRemoteID = Dictionary(grouping: localRecord.remoteFiles) { $0.identifier } // remoteIdentifier is NOT guaranteed to be case-sensitive (e.g. Dropbox) + dispatchGroup.notify(queue: .global()) { + self.finalizeVerification(localRecord: localRecord, remoteFiles: remoteFiles, errors: errors, completion: completion) + } +} + +private func handleDuplicateFiles(_ duplicateFiles: [RemoteFile], dispatchGroup: DispatchGroup, remoteFiles: inout Set<RemoteFile>, errors: inout [FileError]) { + guard let remoteFile = duplicateFiles.first else { return } - for (_, duplicateFiles) in remoteFilesGroupedByRemoteID - { - // Ignore groups with 0 items (if there are any). - guard let remoteFile = duplicateFiles.first else { continue } - - if duplicateFiles.count == 1 - { - // No duplicates, so return file directly. - remoteFiles.insert(remoteFile) - } - else - { - // Duplicates, so determine which one is correct by comparing against all remote file versions. - - dispatchGroup.enter() - - let operation = ServiceOperation<[Version], FileError>(coordinator: self.coordinator) { (completionHandler) in - return self.managedObjectContext.performAndWait { - // Assume all files with same identifier also have same remoteIdentifier. - return self.service.fetchVersions(for: remoteFile, completionHandler: completionHandler) - } - } - operation.resultHandler = { (result) in - self.managedObjectContext.perform { - do - { - let versions = try result.get() - let sortedVersionIDs = NSOrderedSet(array: versions.sorted { $0.date < $1.date }.map { $0.identifier }) - - // Remove all files that don't match any remote versions. - let filteredFiles = duplicateFiles.filter { sortedVersionIDs.contains($0.versionIdentifier) } - - let sortedFiles = filteredFiles.sorted { (fileA, fileB) in - let indexA = sortedVersionIDs.index(of: fileA.versionIdentifier) - let indexB = sortedVersionIDs.index(of: fileB.versionIdentifier) - return indexA < indexB - } - - // If multiple valid files remain, choose the newest one. - guard let newestFile = sortedFiles.last else { throw FileError.doesNotExist(remoteFile.identifier) } - remoteFiles.insert(newestFile) - } - catch - { - errors.append(FileError(remoteFile.identifier, error)) - } - - dispatchGroup.leave() - } - } - - self.operationQueue.addOperation(operation) + if duplicateFiles.count == 1 { + remoteFiles.insert(remoteFile) + } else { + dispatchGroup.enter() + fetchAndProcessVersions(for: remoteFile, duplicateFiles: duplicateFiles, remoteFiles: &remoteFiles, errors: &errors, dispatchGroup: dispatchGroup) + } +} + +private func fetchAndProcessVersions(for remoteFile: RemoteFile, duplicateFiles: [RemoteFile], remoteFiles: inout Set<RemoteFile>, errors: inout [FileError], dispatchGroup: DispatchGroup) { + let operation = ServiceOperation<[Version], FileError>(coordinator: self.coordinator) { (completionHandler) in + self.managedObjectContext.performAndWait { + self.service.fetchVersions(for: remoteFile, completionHandler: completionHandler) } } - dispatchGroup.notify(queue: .global()) { - self.managedObjectContext.perform { - if !errors.isEmpty - { - completion(.failure(.filesFailed(self.record, errors))) - } - else - { - if remoteFiles != localRecord.remoteFiles - { - // Only update if values have changed, just to be safe, since this method is just a bug fix. - - for remoteFile in localRecord.remoteFiles where !remoteFiles.contains(remoteFile) - { - // Remove duplicate remote file. - remoteFile.localRecord = nil - remoteFile.managedObjectContext?.delete(remoteFile) - } - - localRecord.remoteFiles = remoteFiles - } - - completion(.success) - } + operation.resultHandler = { [weak self] (result) in + self?.managedObjectContext.perform { + self?.processVersions(result: result, duplicateFiles: duplicateFiles, remoteFiles: &remoteFiles, errors: &errors) + dispatchGroup.leave() + } + } + + self.operationQueue.addOperation(operation) +} + +private func processVersions(result: Result<[Version], FileError>, duplicateFiles: [RemoteFile], remoteFiles: inout Set<RemoteFile>, errors: inout [FileError]) { + do { + let versions = try result.get() + let sortedVersionIDs = NSOrderedSet(array: versions.sorted { $0.date < $1.date }.map { $0.identifier }) + + let filteredFiles = duplicateFiles.filter { sortedVersionIDs.contains($0.versionIdentifier) } + + let sortedFiles = filteredFiles.sorted { (fileA, fileB) in + let indexA = sortedVersionIDs.index(of: fileA.versionIdentifier) + let indexB = sortedVersionIDs.index(of: fileB.versionIdentifier) + return indexA < indexB + } + + guard let newestFile = sortedFiles.last else { throw FileError.doesNotExist(duplicateFiles.first!.identifier) } + remoteFiles.insert(newestFile) + } catch { + errors.append(FileError(duplicateFiles.first!.identifier, error)) + } +} + +private func finalizeVerification(localRecord: LocalRecord, remoteFiles: Set<RemoteFile>, errors: [FileError], completion: @escaping (Result<Void, RecordError>) -> Void) { + self.managedObjectContext.perform { + if !errors.isEmpty { + completion(.failure(.filesFailed(self.record, errors))) + } else { + self.updateLocalRecord(localRecord, with: remoteFiles) + completion(.success) } } } +private func updateLocalRecord(_ localRecord: LocalRecord, with remoteFiles: Set<RemoteFile>) { + if remoteFiles != localRecord.remoteFiles { + for remoteFile in localRecord.remoteFiles where !remoteFiles.contains(remoteFile) { + remoteFile.localRecord = nil + remoteFile.managedObjectContext?.delete(remoteFile) + } + + localRecord.remoteFiles = remoteFiles + } +} + Apply this suggestion Suggestion importance[1-10]: 9 Why: Breaking down the complex 'verifyRemoteFiles' method into smaller methods significantly enhances code readability and maintainability, making it easier to understand and modify in the future.	9
	Rename the function to better describe its purpose within the operation Consider using a more descriptive name for the prepare() function. The current name is too generic and doesn't clearly indicate its purpose within the context of the operation. Harmony/Operations/SyncRecordsOperation.swift [162-197] -func prepare() +func prepareAndMigrateRecords() { guard databaseError == nil else { finish() return } if isPreviouslySeeded && !self.isSeeded { // Previously seeded, but not anymore, so re-assign. isPreviouslySeeded = false // Prevent infinite recursion if setIsSeeded() fails. self.setIsSeeded(true) { result in switch result { case .failure(let error): databaseError = error case .success: break } - prepare() + prepareAndMigrateRecords() } } else { self.migrateLocalRecordURIsIfNeeded { result in switch result { case .failure(let error): databaseError = error case .success: break } finish() } } } Apply this suggestion Suggestion importance[1-10]: 6 Why: Renaming the 'prepare' function to 'prepareAndMigrateRecords' provides better context and understanding of its functionality, improving code readability and maintainability.	6
	Use more descriptive variable names to improve code readability Consider using a more descriptive variable name for the predicate in the updateRecordsMetadataPredicate computed property. This can improve code readability and maintainability. Harmony/Extensions/ManagedRecord+Predicates.swift [99-102] -let predicate = NSPredicate(format: "(%K & %i) != 0", #keyPath(ManagedRecord._flags), RecordFlags.pendingMetadataUpdate.rawValue) +let pendingMetadataUpdatePredicate = NSPredicate(format: "(%K & %i) != 0", #keyPath(ManagedRecord._flags), RecordFlags.pendingMetadataUpdate.rawValue) -let compoundPredicate = NSCompoundPredicate(andPredicateWithSubpredicates: [predicate, self.syncableRecordsPredicate]) +let compoundPredicate = NSCompoundPredicate(andPredicateWithSubpredicates: [pendingMetadataUpdatePredicate, self.syncableRecordsPredicate]) return compoundPredicate Apply this suggestion Suggestion importance[1-10]: 6 Why: Using a more descriptive variable name enhances code readability and maintainability, though it is a minor improvement.	6
Enhancement	Simplify authentication logic by using a guard statement for the presentingViewController Consider using a guard statement to handle the case where presentingViewController is nil, instead of nesting the entire authentication logic inside an if-let statement. This can improve readability and reduce nesting. Harmony/SyncCoordinator.swift [210-223] -if let presentingViewController -{ - // Extra precaution to ensure we are signed out before signing-in with another account. - guard !self.isAuthenticated else { - self.deauthenticate { result in - switch result - { - case .failure(let error): completionHandler(.failure(AuthenticationError(error))) - case .success: self.authenticate(presentingViewController: presentingViewController, completionHandler: completionHandler) - } - } - return - } +guard let presentingViewController else { + self.service.authenticateInBackground(completionHandler: completionHandler) + return } +// Extra precaution to ensure we are signed out before signing-in with another account. +guard !self.isAuthenticated else { + self.deauthenticate { result in + switch result + { + case .failure(let error): completionHandler(.failure(AuthenticationError(error))) + case .success: self.authenticate(presentingViewController: presentingViewController, completionHandler: completionHandler) + } + } + return +} + Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion improves code readability by reducing nesting, making the logic easier to follow. It effectively handles the case where presentingViewController is nil, which is a common Swift practice.	8
	Use a more specific error type for file deletion failures to improve error handling Consider using a more specific error type or creating a custom error for file deletion failures. This can provide more context and make error handling more robust. Harmony/Operations/Delete/DeleteRecordsOperation.swift [62] -Logger.sync.error("Failed to delete file \(file.identifier, privacy: .public) at \(file.fileURL, privacy: .public). \(error.localizedDescription, privacy: .public)") +enum FileDeletionError: Error { + case deletionFailed(fileIdentifier: String, url: URL, underlyingError: Error) +} +let deletionError = FileDeletionError.deletionFailed(fileIdentifier: file.identifier, url: file.fileURL, underlyingError: error) +Logger.sync.error("Failed to delete file: \(deletionError)") + Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion to use a specific error type for file deletion failures adds significant value by providing more context and improving error handling robustness.	8
	Use string interpolation for log messages to improve readability and performance Consider using string interpolation for the log messages instead of concatenating strings. This can improve readability and performance. Harmony/Operations/Misc./FetchRemoteRecordsOperation.swift [49-50] -Logger.sync.info("Fetched changed remote records:\n\(records.count)") +Logger.sync.info("Fetched changed remote records: \(records.count)") Logger.sync.debug("Remote Records:\n\(outputText, privacy: .public)") Apply this suggestion Suggestion importance[1-10]: 5 Why: The suggestion to use string interpolation is correct and improves readability slightly, but the performance gain is negligible in this context.	5
Best practice	Use a guard statement to handle potential nil values more explicitly Consider using a guard statement to unwrap record.localMetadata instead of using the nil-coalescing operator. This can make the code more readable and handle the case where localMetadata is nil more explicitly. Harmony/Operations/Metadata/UpdateRecordMetadataOperation.swift [18-21] -var metadata = record.localMetadata ?? [:] +guard var metadata = record.localMetadata else { + metadata = [:] +} metadata[.recordedObjectType] = record.recordID.type metadata[.recordedObjectIdentifier] = record.recordID.identifier self.metadata = metadata Apply this suggestion Suggestion importance[1-10]: 7 Why: The suggestion to use a guard statement improves code readability by explicitly handling the nil case, making the code more robust and easier to understand.	7
	Ensure progress completion is always executed, regardless of operation success or failure Consider moving the progress completion check outside of the success case to ensure it's always executed, even in case of failure. Harmony/Operations/ServiceOperation.swift [76-87] do { _ = try result.get() - - if let progress = self.taskProgress - { - // Ensure progress is completed. - progress.completedUnitCount = progress.totalUnitCount - } - self.result = result self.finish() } +catch +{ + // Handle error case +} +if let progress = self.taskProgress +{ + // Ensure progress is completed. + progress.completedUnitCount = progress.totalUnitCount +} + Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion ensures that progress completion is handled consistently, which is a good practice for maintaining accurate progress tracking, although it doesn't address a critical issue.	6