From d251c3fe7483408f133c14f269e0e422b2140bc0 Mon Sep 17 00:00:00 2001 From: mplorentz Date: Fri, 7 Jun 2024 11:09:36 -0400 Subject: [PATCH 1/3] Move database cleanup routine into DatabaseCleaner type --- Nos.xcodeproj/project.pbxproj | 10 ++ Nos/Controller/PersistenceController.swift | 130 ++------------------ Nos/NosApp.swift | 2 +- Nos/Service/DatabaseCleaner.swift | 123 ++++++++++++++++++ NosTests/Service/DatabaseCleanerTests.swift | 8 ++ 5 files changed, 153 insertions(+), 120 deletions(-) create mode 100644 Nos/Service/DatabaseCleaner.swift create mode 100644 NosTests/Service/DatabaseCleanerTests.swift diff --git a/Nos.xcodeproj/project.pbxproj b/Nos.xcodeproj/project.pbxproj index 3a1ed9941..27a689c15 100644 --- a/Nos.xcodeproj/project.pbxproj +++ b/Nos.xcodeproj/project.pbxproj @@ -368,6 +368,9 @@ C9BCF1C12AC72020009BDE06 /* UNSWizardChooseNameView.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9BCF1C02AC72020009BDE06 /* UNSWizardChooseNameView.swift */; }; C9BD91892B61BBEF00FDA083 /* bad_contact_list.json in Resources */ = {isa = PBXBuildFile; fileRef = C9BD91882B61BBEF00FDA083 /* bad_contact_list.json */; }; C9BD919B2B61C4FB00FDA083 /* RawNostrID+Random.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9BD919A2B61C4FB00FDA083 /* RawNostrID+Random.swift */; }; + C9C097232C13534800F78EC3 /* DatabaseCleanerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9C097222C13534800F78EC3 /* DatabaseCleanerTests.swift */; }; + C9C097252C13537900F78EC3 /* DatabaseCleaner.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9C097242C13537900F78EC3 /* DatabaseCleaner.swift */; }; + C9C097262C13537900F78EC3 /* DatabaseCleaner.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9C097242C13537900F78EC3 /* DatabaseCleaner.swift */; }; C9C2B77C29E072E400548B4A /* WebSocket+Nos.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9C2B77B29E072E400548B4A /* WebSocket+Nos.swift */; }; C9C2B77D29E072E400548B4A /* WebSocket+Nos.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9C2B77B29E072E400548B4A /* WebSocket+Nos.swift */; }; C9C2B77F29E0731600548B4A /* AsyncTimer.swift in Sources */ = {isa = PBXBuildFile; fileRef = C9C2B77E29E0731600548B4A /* AsyncTimer.swift */; }; @@ -754,6 +757,8 @@ C9BCF1C02AC72020009BDE06 /* UNSWizardChooseNameView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = UNSWizardChooseNameView.swift; sourceTree = ""; }; C9BD91882B61BBEF00FDA083 /* bad_contact_list.json */ = {isa = PBXFileReference; lastKnownFileType = text.json; path = bad_contact_list.json; sourceTree = ""; }; C9BD919A2B61C4FB00FDA083 /* RawNostrID+Random.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "RawNostrID+Random.swift"; sourceTree = ""; }; + C9C097222C13534800F78EC3 /* DatabaseCleanerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DatabaseCleanerTests.swift; sourceTree = ""; }; + C9C097242C13537900F78EC3 /* DatabaseCleaner.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DatabaseCleaner.swift; sourceTree = ""; }; C9C2B77B29E072E400548B4A /* WebSocket+Nos.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "WebSocket+Nos.swift"; sourceTree = ""; }; C9C2B77E29E0731600548B4A /* AsyncTimer.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AsyncTimer.swift; sourceTree = ""; }; C9C2B78129E0735400548B4A /* RelaySubscriptionManager.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RelaySubscriptionManager.swift; sourceTree = ""; }; @@ -961,6 +966,7 @@ 035729B72BE416A6005FEE85 /* Service */ = { isa = PBXGroup; children = ( + C9C097222C13534800F78EC3 /* DatabaseCleanerTests.swift */, 035729B42BE416A6005FEE85 /* DirectMessageWrapperTests.swift */, 035729B52BE416A6005FEE85 /* GiftWrapperTests.swift */, 037975D02C0E341500ADDF37 /* MockFeatureFlags.swift */, @@ -1173,6 +1179,7 @@ C9A0DAF729C92F4500466635 /* UNSAPI.swift */, 032634512C10BB8F00E489B5 /* FileStorage */, C98CA9052B14FA8500929141 /* Relay */, + C9C097242C13537900F78EC3 /* DatabaseCleaner.swift */, ); path = Service; sourceTree = ""; @@ -2011,6 +2018,7 @@ C960C57129F3236200929990 /* LikeButton.swift in Sources */, C97797B9298AA19A0046BD25 /* RelayService.swift in Sources */, C99721CB2AEBED26004EBEAB /* String+Empty.swift in Sources */, + C9C097252C13537900F78EC3 /* DatabaseCleaner.swift in Sources */, C959DB762BD01DF4008F3627 /* GiftWrapper.swift in Sources */, 5B29B5842BEAA0D7008F6008 /* BioSheet.swift in Sources */, C93CA0C329AE3A1E00921183 /* JSONEvent.swift in Sources */, @@ -2130,6 +2138,7 @@ A3B943D7299D6DB700A15A08 /* Follow+CoreDataClass.swift in Sources */, C9ADB13E29929EEF0075E7F8 /* Bech32.swift in Sources */, 5B098DC92BDAF7CF00500A1B /* NoteParserTests+NIP27.swift in Sources */, + C9C097262C13537900F78EC3 /* DatabaseCleaner.swift in Sources */, C9DEC05A2989509B0078B43A /* PersistenceController.swift in Sources */, C9C2B78629E073E300548B4A /* RelaySubscription.swift in Sources */, C973AB662A323167002AED16 /* EventReference+CoreDataProperties.swift in Sources */, @@ -2158,6 +2167,7 @@ 0326347B2C10C57A00E489B5 /* FileStorageAPIClient.swift in Sources */, C9B678DC29EEBF3B00303F33 /* DependencyInjection.swift in Sources */, C9F0BB6D29A503D9000547FC /* Int+Bool.swift in Sources */, + C9C097232C13534800F78EC3 /* DatabaseCleanerTests.swift in Sources */, DC08FF812A7969C5009F87D1 /* UIDevice+Simulator.swift in Sources */, 5BFBB2962BD9D824002E909F /* URLParser.swift in Sources */, C959DB772BD01DF4008F3627 /* GiftWrapper.swift in Sources */, diff --git a/Nos/Controller/PersistenceController.swift b/Nos/Controller/PersistenceController.swift index 397c127b5..07ecda1dd 100644 --- a/Nos/Controller/PersistenceController.swift +++ b/Nos/Controller/PersistenceController.swift @@ -6,6 +6,7 @@ class PersistenceController { @Dependency(\.currentUser) var currentUser @Dependency(\.analytics) var analytics + @Dependency(\.crashReporting) var crashReporting /// Increment this to delete core data on update static let version = 3 @@ -186,130 +187,21 @@ class PersistenceController { UserDefaults.standard.set(newVersion, forKey: Self.versionKey) } - var cleanupTask: Task? - - // swiftlint:disable function_body_length - - /// Deletes unneeded entities from Core Data. - /// The general strategy here is to: - /// - keep some max number of events, delete the others - /// - delete authors outside the user's network - /// - delete any other models that are orphaned by the previous deletions - /// - fix EventReferences whose referencedEvent was deleted by createing a stubbed Event - @MainActor func cleanupEntities() { - // this function was written in a hurry and probably should be refactored and tested thorougly. - guard cleanupTask == nil else { - Log.info("Core Data cleanup task already running. Aborting.") + /// Cleans up uneeded entities from the database. Our local database is really just a cache, and we need to + /// invalidate old items to keep it from growing indefinitely. + func cleanupEntities() async { + guard let authorKey = await currentUser.author?.hexadecimalPublicKey else { return } - guard let authorKey = currentUser.author?.hexadecimalPublicKey else { - return - } - - cleanupTask = Task { - defer { self.cleanupTask = nil } - let context = newBackgroundContext() - let startTime = Date.now - Log.info("Starting Core Data cleanup...") - - Log.info("Database statistics: \(try await Self.databaseStatistics(from: context))") - - // Delete all but the most recent n events - let eventsToKeep = 1000 - let fetchFirstEventToDelete = Event.allEventsRequest() - fetchFirstEventToDelete.sortDescriptors = [NSSortDescriptor(keyPath: \Event.receivedAt, ascending: false)] - fetchFirstEventToDelete.fetchLimit = 1 - fetchFirstEventToDelete.fetchOffset = eventsToKeep - fetchFirstEventToDelete.predicate = NSPredicate(format: "receivedAt != nil") - var deleteBefore = Date.distantPast - try await context.perform { - - guard let currentAuthor = try? Author.find(by: authorKey, context: context) else { - return - } - - if let firstEventToDelete = try context.fetch(fetchFirstEventToDelete).first, - let receivedAt = firstEventToDelete.receivedAt { - deleteBefore = receivedAt - } - - let oldStoryCutoff = Calendar.current.date(byAdding: .day, value: -2, to: .now) ?? .now - - // Delete events older than `deleteBefore` - let oldEventsRequest = NSFetchRequest(entityName: "Event") - oldEventsRequest.sortDescriptors = [NSSortDescriptor(keyPath: \Event.receivedAt, ascending: true)] - let oldEventClause = "(receivedAt <= %@ OR receivedAt == nil)" - let notOwnEventClause = "(author.hexadecimalPublicKey != %@)" - let readStoryClause = "(isRead = 1 AND receivedAt > %@)" - let userReportClause = "(kind == \(EventKind.report.rawValue) AND " + - "authorReferences.@count > 0 AND eventReferences.@count == 0)" - let clauses = "\(oldEventClause) AND" + - "\(notOwnEventClause) AND " + - "NOT \(readStoryClause) AND " + - "NOT \(userReportClause)" - oldEventsRequest.predicate = NSPredicate( - format: clauses, - deleteBefore as CVarArg, - authorKey, - oldStoryCutoff as CVarArg - ) - - let deleteRequests: [NSPersistentStoreRequest] = [ - oldEventsRequest, - Event.expiredRequest(), - EventReference.orphanedRequest(), - AuthorReference.orphanedRequest(), - Author.outOfNetwork(for: currentAuthor), - Follow.orphanedRequest(), - Relay.orphanedRequest(), - NosNotification.oldNotificationsRequest(), - ] - - for request in deleteRequests { - guard let fetchRequest = request as? NSFetchRequest else { - Log.error("Bad fetch request: \(request)") - continue - } - - let batchDeleteRequest = NSBatchDeleteRequest(fetchRequest: fetchRequest) - batchDeleteRequest.resultType = .resultTypeCount - let deleteResult = try context.execute(batchDeleteRequest) as? NSBatchDeleteResult - if let entityName = fetchRequest.entityName { - Log.info("Deleted \(deleteResult?.result ?? 0) of type \(entityName)") - } - } - - // Heal EventReferences - let brokenEventReferencesRequest = NSFetchRequest(entityName: "EventReference") - brokenEventReferencesRequest.sortDescriptors = [ - NSSortDescriptor(keyPath: \EventReference.eventId, ascending: false) - ] - brokenEventReferencesRequest.predicate = NSPredicate(format: "referencedEvent = nil") - let brokenEventReferences = try context.fetch(brokenEventReferencesRequest) - Log.info("Healing \(brokenEventReferences.count) EventReferences") - for eventReference in brokenEventReferences { - guard let eventID = eventReference.eventId else { - Log.error("Found an EventReference with no eventID") - continue - } - let referencedEvent = try Event.findOrCreateStubBy(id: eventID, context: context) - eventReference.referencedEvent = referencedEvent - } - - try context.saveIfNeeded() - context.refreshAllObjects() - } - - let newStatistics = try await Self.databaseStatistics(from: context) - Log.info("Database statistics: \(newStatistics)") - analytics.databaseStatistics(newStatistics) - - let elapsedTime = Date.now.timeIntervalSince1970 - startTime.timeIntervalSince1970 - Log.info("Finished Core Data cleanup in \(elapsedTime) seconds.") + let context = newBackgroundContext() + do { + try await DatabaseCleaner.cleanupEntities(before: Date.now, for: authorKey, in: context) + } catch { + Log.optional(error) + crashReporting.report("Error in database cleanup: \(error.localizedDescription)") } } - // swiftlint:enable function_body_length static func databaseStatistics(from context: NSManagedObjectContext) async throws -> [(String, Int)] { try await context.perform { diff --git a/Nos/NosApp.swift b/Nos/NosApp.swift index a9f990f53..8972a7978 100644 --- a/Nos/NosApp.swift +++ b/Nos/NosApp.swift @@ -34,7 +34,7 @@ struct NosApp: App { fileStorageAPIClient.refreshServerInfo() } .task { - persistenceController.cleanupEntities() + await persistenceController.cleanupEntities() } .onChange(of: scenePhase) { _, newPhase in // TODO: save all contexts, not just the view and background. diff --git a/Nos/Service/DatabaseCleaner.swift b/Nos/Service/DatabaseCleaner.swift new file mode 100644 index 000000000..f8adc2840 --- /dev/null +++ b/Nos/Service/DatabaseCleaner.swift @@ -0,0 +1,123 @@ +import Foundation +import Logger +import CoreData +import Dependencies + +enum DatabaseCleaner { + + // swiftlint:disable function_body_length + + /// Deletes unneeded entities from Core Data. + /// The general strategy here is to: + /// - keep some max number of events, delete the others + /// - delete authors outside the user's network + /// - delete any other models that are orphaned by the previous deletions + /// - fix EventReferences whose referencedEvent was deleted by createing a stubbed Event + static func cleanupEntities( + before date: Date, + for authorKey: RawAuthorID, + in context: NSManagedObjectContext + ) async throws { + // this function was written in a hurry and probably should be refactored and tested thorougly. + @Dependency(\.analytics) var analytics + + let startTime = Date.now + Log.info("Starting Core Data cleanup...") + + Log.info("Database statistics: \(try await PersistenceController.databaseStatistics(from: context))") + + // Delete all but the most recent n events + let eventsToKeep = 1000 + let fetchFirstEventToDelete = Event.allEventsRequest() + fetchFirstEventToDelete.sortDescriptors = [NSSortDescriptor(keyPath: \Event.receivedAt, ascending: false)] + fetchFirstEventToDelete.fetchLimit = 1 + fetchFirstEventToDelete.fetchOffset = eventsToKeep + fetchFirstEventToDelete.predicate = NSPredicate(format: "receivedAt != nil") + var deleteBefore = Date.distantPast + try await context.perform { + + guard let currentAuthor = try? Author.find(by: authorKey, context: context) else { + return + } + + if let firstEventToDelete = try context.fetch(fetchFirstEventToDelete).first, + let receivedAt = firstEventToDelete.receivedAt { + deleteBefore = receivedAt + } + + let oldStoryCutoff = Calendar.current.date(byAdding: .day, value: -2, to: .now) ?? .now + + // Delete events older than `deleteBefore` + let oldEventsRequest = NSFetchRequest(entityName: "Event") + oldEventsRequest.sortDescriptors = [NSSortDescriptor(keyPath: \Event.receivedAt, ascending: true)] + let oldEventClause = "(receivedAt <= %@ OR receivedAt == nil)" + let notOwnEventClause = "(author.hexadecimalPublicKey != %@)" + let readStoryClause = "(isRead = 1 AND receivedAt > %@)" + let userReportClause = "(kind == \(EventKind.report.rawValue) AND " + + "authorReferences.@count > 0 AND eventReferences.@count == 0)" + let clauses = "\(oldEventClause) AND" + + "\(notOwnEventClause) AND " + + "NOT \(readStoryClause) AND " + + "NOT \(userReportClause)" + oldEventsRequest.predicate = NSPredicate( + format: clauses, + deleteBefore as CVarArg, + authorKey, + oldStoryCutoff as CVarArg + ) + + let deleteRequests: [NSPersistentStoreRequest] = [ + oldEventsRequest, + Event.expiredRequest(), + EventReference.orphanedRequest(), + AuthorReference.orphanedRequest(), + Author.outOfNetwork(for: currentAuthor), + Follow.orphanedRequest(), + Relay.orphanedRequest(), + NosNotification.oldNotificationsRequest(), + ] + + for request in deleteRequests { + guard let fetchRequest = request as? NSFetchRequest else { + Log.error("Bad fetch request: \(request)") + continue + } + + let batchDeleteRequest = NSBatchDeleteRequest(fetchRequest: fetchRequest) + batchDeleteRequest.resultType = .resultTypeCount + let deleteResult = try context.execute(batchDeleteRequest) as? NSBatchDeleteResult + if let entityName = fetchRequest.entityName { + Log.info("Deleted \(deleteResult?.result ?? 0) of type \(entityName)") + } + } + + // Heal EventReferences + let brokenEventReferencesRequest = NSFetchRequest(entityName: "EventReference") + brokenEventReferencesRequest.sortDescriptors = [ + NSSortDescriptor(keyPath: \EventReference.eventId, ascending: false) + ] + brokenEventReferencesRequest.predicate = NSPredicate(format: "referencedEvent = nil") + let brokenEventReferences = try context.fetch(brokenEventReferencesRequest) + Log.info("Healing \(brokenEventReferences.count) EventReferences") + for eventReference in brokenEventReferences { + guard let eventID = eventReference.eventId else { + Log.error("Found an EventReference with no eventID") + continue + } + let referencedEvent = try Event.findOrCreateStubBy(id: eventID, context: context) + eventReference.referencedEvent = referencedEvent + } + + try context.saveIfNeeded() + context.refreshAllObjects() + } + + let newStatistics = try await PersistenceController.databaseStatistics(from: context) + Log.info("Database statistics: \(newStatistics)") + analytics.databaseStatistics(newStatistics) + + let elapsedTime = Date.now.timeIntervalSince1970 - startTime.timeIntervalSince1970 + Log.info("Finished Core Data cleanup in \(elapsedTime) seconds.") + } + // swiftlint:enable function_body_length +} diff --git a/NosTests/Service/DatabaseCleanerTests.swift b/NosTests/Service/DatabaseCleanerTests.swift new file mode 100644 index 000000000..7bdee06da --- /dev/null +++ b/NosTests/Service/DatabaseCleanerTests.swift @@ -0,0 +1,8 @@ +import XCTest + +final class DatabaseCleanerTests: CoreDataTestCase { + + func test_emptyDatabase() async throws { + try await DatabaseCleaner.cleanupEntities(before: Date.now, for: KeyFixture.alice.publicKeyHex, in: testContext) + } +} From b2c9c30c065ab91a7151a782da1d03d4e28690e9 Mon Sep 17 00:00:00 2001 From: mplorentz Date: Fri, 7 Jun 2024 11:18:25 -0400 Subject: [PATCH 2/3] Add note about only calling routine once at app launch --- Nos/Controller/PersistenceController.swift | 2 ++ Nos/Service/DatabaseCleaner.swift | 3 +++ 2 files changed, 5 insertions(+) diff --git a/Nos/Controller/PersistenceController.swift b/Nos/Controller/PersistenceController.swift index 07ecda1dd..391997ace 100644 --- a/Nos/Controller/PersistenceController.swift +++ b/Nos/Controller/PersistenceController.swift @@ -189,6 +189,8 @@ class PersistenceController { /// Cleans up uneeded entities from the database. Our local database is really just a cache, and we need to /// invalidate old items to keep it from growing indefinitely. + /// + /// This should only be called once right at app launch. func cleanupEntities() async { guard let authorKey = await currentUser.author?.hexadecimalPublicKey else { return diff --git a/Nos/Service/DatabaseCleaner.swift b/Nos/Service/DatabaseCleaner.swift index f8adc2840..fd2abe6a7 100644 --- a/Nos/Service/DatabaseCleaner.swift +++ b/Nos/Service/DatabaseCleaner.swift @@ -8,6 +8,9 @@ enum DatabaseCleaner { // swiftlint:disable function_body_length /// Deletes unneeded entities from Core Data. + /// + /// This should only be called once right at app launch. + /// /// The general strategy here is to: /// - keep some max number of events, delete the others /// - delete authors outside the user's network From 841f492bd13e36231439329af5cdd0fc83e33de5 Mon Sep 17 00:00:00 2001 From: mplorentz Date: Mon, 10 Jun 2024 10:08:03 -0400 Subject: [PATCH 3/3] Add assertions to DatabaseCleanerTests.test_emptyDatabase() --- NosTests/Service/DatabaseCleanerTests.swift | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/NosTests/Service/DatabaseCleanerTests.swift b/NosTests/Service/DatabaseCleanerTests.swift index 7bdee06da..7a2dc2b07 100644 --- a/NosTests/Service/DatabaseCleanerTests.swift +++ b/NosTests/Service/DatabaseCleanerTests.swift @@ -1,8 +1,20 @@ import XCTest +import CoreData final class DatabaseCleanerTests: CoreDataTestCase { func test_emptyDatabase() async throws { + // Act try await DatabaseCleaner.cleanupEntities(before: Date.now, for: KeyFixture.alice.publicKeyHex, in: testContext) + + // Assert that the database is still empty + let managedObjectModel = try XCTUnwrap(testContext.persistentStoreCoordinator?.managedObjectModel) + let entitiesByName = managedObjectModel.entitiesByName + XCTAssertGreaterThan(entitiesByName.count, 0) // sanity check + + for entityName in entitiesByName.keys { + let fetchRequest = NSFetchRequest(entityName: entityName) + XCTAssertEqual(try testContext.count(for: fetchRequest), 0) + } } }