Skip to content

Commit

Permalink
In case Pending Deletion flag is nil, populate it with 'false' value (#…
Browse files Browse the repository at this point in the history
…2997)

Task/Issue URL: https://app.asana.com/0/856498667320406/1207582863974542/f
Tech Design URL:
CC:

Description:

Check if DB sate is unexpected and fix it if needed.

Steps to test this PR:

See tests to validate how state is fixed.

Ensure Bookmarks are still working after the check, and if migration passes.
  • Loading branch information
bwaresiak authored Jun 27, 2024
1 parent c032c18 commit afe414a
Show file tree
Hide file tree
Showing 5 changed files with 236 additions and 3 deletions.
83 changes: 83 additions & 0 deletions Core/BookmarksStateRepair.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
//
// BookmarksStateRepair.swift
// DuckDuckGo
//
// 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 CoreData
import Bookmarks
import Persistence

public class BookmarksStateRepair {

enum Constants {
static let pendingDeletionRepaired = "stateRepair_pendingDeletionRepaired"
}

public enum RepairStatus: Equatable {
case alreadyPerformed
case noBrokenData
case dataRepaired
case repairError(Error)

public static func == (lhs: BookmarksStateRepair.RepairStatus, rhs: BookmarksStateRepair.RepairStatus) -> Bool {
switch (lhs, rhs) {
case (.alreadyPerformed, .alreadyPerformed), (.noBrokenData, .noBrokenData), (.dataRepaired, .dataRepaired), (.repairError, .repairError):
return true
default:
return false
}
}
}

let keyValueStore: KeyValueStoring

public init(keyValueStore: KeyValueStoring) {
self.keyValueStore = keyValueStore
}

public func validateAndRepairPendingDeletionState(in context: NSManagedObjectContext) -> RepairStatus {

guard keyValueStore.object(forKey: Constants.pendingDeletionRepaired) == nil else {
return .alreadyPerformed
}

do {
let fr = BookmarkEntity.fetchRequest()
fr.predicate = NSPredicate(format: "%K == nil", #keyPath(BookmarkEntity.isPendingDeletion))

let result = try context.fetch(fr)

if !result.isEmpty {
for obj in result {
obj.setValue(false, forKey: #keyPath(BookmarkEntity.isPendingDeletion))
}

try context.save()

keyValueStore.set(true, forKey: Constants.pendingDeletionRepaired)
return .dataRepaired
} else {
keyValueStore.set(true, forKey: Constants.pendingDeletionRepaired)
return .noBrokenData
}
} catch {
return .repairError(error)
}
}

}
8 changes: 7 additions & 1 deletion Core/PixelEvent.swift
Original file line number Diff line number Diff line change
Expand Up @@ -504,7 +504,10 @@ extension Pixel {
case debugBookmarksStructureLost
case debugBookmarksInvalidRoots
case debugBookmarksValidationFailed


case debugBookmarksPendingDeletionFixed
case debugBookmarksPendingDeletionRepairError

case debugCannotClearObservationsDatabase
case debugWebsiteDataStoresNotClearedMultiple
case debugWebsiteDataStoresNotClearedOne
Expand Down Expand Up @@ -1183,6 +1186,9 @@ extension Pixel.Event {
case .debugBookmarksInvalidRoots: return "m_d_bookmarks_invalid_roots"
case .debugBookmarksValidationFailed: return "m_d_bookmarks_validation_failed"

case .debugBookmarksPendingDeletionFixed: return "m_debug_bookmarks_pending_deletion_fixed"
case .debugBookmarksPendingDeletionRepairError: return "m_debug_bookmarks_pending_deletion_repair_error"

case .debugCannotClearObservationsDatabase: return "m_d_cannot_clear_observations_database"
case .debugWebsiteDataStoresNotClearedMultiple: return "m_d_wkwebsitedatastoresnotcleared_multiple"
case .debugWebsiteDataStoresNotClearedOne: return "m_d_wkwebsitedatastoresnotcleared_one"
Expand Down
10 changes: 9 additions & 1 deletion DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,8 @@
9833913727AC400800DAF119 /* AppTrackerDataSetProvider.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9833913627AC400800DAF119 /* AppTrackerDataSetProvider.swift */; };
9838059F2228208E00385F1A /* PositiveFeedbackViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9838059E2228208E00385F1A /* PositiveFeedbackViewController.swift */; };
983BD6B52B34760600AAC78E /* MockPrivacyConfiguration.swift in Sources */ = {isa = PBXBuildFile; fileRef = C1B0F6412AB08BE9001EAF05 /* MockPrivacyConfiguration.swift */; };
983C52E42C2C050B007B5747 /* BookmarksStateRepair.swift in Sources */ = {isa = PBXBuildFile; fileRef = 983C52E32C2C050B007B5747 /* BookmarksStateRepair.swift */; };
983C52E72C2C0ACB007B5747 /* BookmarkStateRepairTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 983C52E52C2C0ABA007B5747 /* BookmarkStateRepairTests.swift */; };
983D71B12A286E810072E26D /* SyncDebugViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 983D71B02A286E810072E26D /* SyncDebugViewController.swift */; };
983EABB8236198F6003948D1 /* DatabaseMigration.swift in Sources */ = {isa = PBXBuildFile; fileRef = 983EABB7236198F6003948D1 /* DatabaseMigration.swift */; };
984147A824F0259000362052 /* Onboarding.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = 984147AA24F0259000362052 /* Onboarding.storyboard */; };
Expand Down Expand Up @@ -1648,6 +1650,8 @@
983A4B8F251EABEA00F3EDF1 /* et */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = et; path = et.lproj/InfoPlist.strings; sourceTree = "<group>"; };
983A4B90251EABEA00F3EDF1 /* et */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = et; path = et.lproj/Localizable.strings; sourceTree = "<group>"; };
983A4B91251EABEA00F3EDF1 /* et */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = et; path = et.lproj/InfoPlist.strings; sourceTree = "<group>"; };
983C52E32C2C050B007B5747 /* BookmarksStateRepair.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarksStateRepair.swift; sourceTree = "<group>"; };
983C52E52C2C0ABA007B5747 /* BookmarkStateRepairTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = BookmarkStateRepairTests.swift; sourceTree = "<group>"; };
983D71B02A286E810072E26D /* SyncDebugViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SyncDebugViewController.swift; sourceTree = "<group>"; };
983E1349251EABF200149BD9 /* fi */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = fi; path = fi.lproj/InfoPlist.strings; sourceTree = "<group>"; };
983E134A251EABF200149BD9 /* fi */ = {isa = PBXFileReference; lastKnownFileType = text.plist.strings; name = fi; path = fi.lproj/InfoPlist.strings; sourceTree = "<group>"; };
Expand Down Expand Up @@ -4239,11 +4243,12 @@
C14882DD27F20D7300D59F0C /* Bookmarks */ = {
isa = PBXGroup;
children = (
C14882DE27F20D7E00D59F0C /* ImportExport */,
987130BD294AAB8200AB05E0 /* BSK */,
98AAF8E3292EB46000DBDF06 /* BookmarksMigrationTests.swift */,
98983095255B5019003339A2 /* BookmarksCachingSearchTests.swift */,
98629D322C21BDEB001E6031 /* BookmarksStateValidationTests.swift */,
C14882DE27F20D7E00D59F0C /* ImportExport */,
983C52E52C2C0ABA007B5747 /* BookmarkStateRepairTests.swift */,
);
name = Bookmarks;
sourceTree = "<group>";
Expand Down Expand Up @@ -5289,6 +5294,7 @@
9856A1982933D2EB00ACB44F /* BookmarksModelsErrorHandling.swift */,
379E877329E97C8D001C8BB0 /* BookmarksCleanupErrorHandling.swift */,
98629D302C21765A001E6031 /* BookmarksStateValidation.swift */,
983C52E32C2C050B007B5747 /* BookmarksStateRepair.swift */,
C14882D627F2010700D59F0C /* ImportExport */,
F1CE42A81ECA0A660074A8DF /* LegacyStore */,
);
Expand Down Expand Up @@ -6844,6 +6850,7 @@
9FA5E44E2BF1B16400BDEF02 /* SubscriptionContainerViewModelTests.swift in Sources */,
85BA58581F34F72F00C6E8CA /* AppUserDefaultsTests.swift in Sources */,
F1134EBC1F40D45700B73467 /* MockStatisticsStore.swift in Sources */,
983C52E72C2C0ACB007B5747 /* BookmarkStateRepairTests.swift in Sources */,
31C138AC27A403CB00FFD4B2 /* DownloadManagerTests.swift in Sources */,
EEFE9C732A603CE9005B0A26 /* NetworkProtectionStatusViewModelTests.swift in Sources */,
F13B4BF91F18CA0600814661 /* TabsModelTests.swift in Sources */,
Expand Down Expand Up @@ -7138,6 +7145,7 @@
1E4DCF4C27B6A4CB00961E25 /* URLFileExtension.swift in Sources */,
EE50053029C3BA0800AE0773 /* InternalUserStore.swift in Sources */,
F1D477CB1F2149C40031ED49 /* Type.swift in Sources */,
983C52E42C2C050B007B5747 /* BookmarksStateRepair.swift in Sources */,
1E05D1D629C46EBB00BF9A1F /* DailyPixel.swift in Sources */,
1CB7B82123CEA1F800AA24EA /* DateExtension.swift in Sources */,
379E877429E97C8D001C8BB0 /* BookmarksCleanupErrorHandling.swift in Sources */,
Expand Down
20 changes: 19 additions & 1 deletion DuckDuckGo/BookmarksDatabaseSetup.swift
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ struct BookmarksDatabaseSetup {
let contextForValidation = bookmarksDatabase.makeContext(concurrencyType: .privateQueueConcurrencyType)
contextForValidation.performAndWait {
validator.validateBookmarksStructure(context: contextForValidation)
repairDeletedFlag(context: contextForValidation)
}

if migrationHappened {
Expand All @@ -84,7 +85,24 @@ struct BookmarksDatabaseSetup {

return migrationHappened
}


private func repairDeletedFlag(context: NSManagedObjectContext) {
let stateRepair = BookmarksStateRepair(keyValueStore: UserDefaults.app)
let status = stateRepair.validateAndRepairPendingDeletionState(in: context)
switch status {
case .alreadyPerformed, .noBrokenData:
break
case .dataRepaired:
Pixel.fire(pixel: .debugBookmarksPendingDeletionFixed)
case .repairError(let underlyingError):
let processedErrors = CoreDataErrorsParser.parse(error: underlyingError as NSError)

DailyPixel.fireDailyAndCount(pixel: .debugBookmarksPendingDeletionRepairError,
withAdditionalParameters: processedErrors.errorPixelParameters,
includedParameters: [.appVersion])
}
}

private func migrateToFormFactorSpecificFavorites(_ context: NSManagedObjectContext, _ oldFavoritesOrder: [String]?) -> Bool {
do {
BookmarkFormFactorFavoritesMigration.migrateToFormFactorSpecificFavorites(byCopyingExistingTo: .mobile,
Expand Down
118 changes: 118 additions & 0 deletions DuckDuckGoTests/BookmarkStateRepairTests.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
//
// BookmarkStateRepairTests.swift
// DuckDuckGo
//
// 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 XCTest
import CoreData
import Bookmarks
import TestUtils
@testable import Core
@testable import DuckDuckGo

class BookmarkStateRepairTests: XCTestCase {

let dbStack = MockBookmarksDatabase.make(prepareFolderStructure: false)

let mockKeyValueStore = MockKeyValueStore()

override func setUp() async throws {
try await super.setUp()

let containerLocation = MockBookmarksDatabase.tempDBDir()
try FileManager.default.createDirectory(at: containerLocation, withIntermediateDirectories: true)
}

override func tearDown() async throws {
try await super.tearDown()

try dbStack.tearDown(deleteStores: true)
}

func testWhenThereIsNoIssueThenThereAreNoChanges() {
prepareValidStructure()

let testContext = dbStack.makeContext(concurrencyType: .privateQueueConcurrencyType)
testContext.performAndWait {

let repair = BookmarksStateRepair(keyValueStore: mockKeyValueStore)

XCTAssertEqual(repair.validateAndRepairPendingDeletionState(in: testContext), .noBrokenData)
XCTAssertEqual(repair.validateAndRepairPendingDeletionState(in: testContext), .alreadyPerformed)
}
}

func testWhenPendingDeletionIsNilThenItIsFixed() {
prepareBrokenStructure()

let testContext = dbStack.makeContext(concurrencyType: .privateQueueConcurrencyType)
testContext.performAndWait {

let repair = BookmarksStateRepair(keyValueStore: mockKeyValueStore)

XCTAssertEqual(repair.validateAndRepairPendingDeletionState(in: testContext), .dataRepaired)

mockKeyValueStore.removeObject(forKey: BookmarksStateRepair.Constants.pendingDeletionRepaired)
XCTAssertEqual(repair.validateAndRepairPendingDeletionState(in: testContext), .noBrokenData)
XCTAssertEqual(repair.validateAndRepairPendingDeletionState(in: testContext), .alreadyPerformed)
}
}

private func prepareStructure(_ block: (NSManagedObjectContext) -> Void) {
let context = dbStack.makeContext(concurrencyType: .privateQueueConcurrencyType)

context.performAndWait {
BookmarkUtils.prepareFoldersStructure(in: context)
block(context)
do {
try context.save()
} catch {
XCTFail("Could not save context")
}
}
}

private func prepareValidStructure() {
prepareStructure { context in
guard let root = BookmarkUtils.fetchRootFolder(context) else {
XCTFail("Root missing")
return
}

let bookmarkA = BookmarkEntity.makeBookmark(title: "A", url: "A", parent: root, context: context)
let bookmarkB = BookmarkEntity.makeBookmark(title: "B", url: "B", parent: root, context: context)
let bookmarkC = BookmarkEntity.makeBookmark(title: "C", url: "C", parent: root, context: context)
}
}

private func prepareBrokenStructure() {
prepareStructure { context in
guard let root = BookmarkUtils.fetchRootFolder(context) else {
XCTFail("Root missing")
return
}

let bookmarkA = BookmarkEntity.makeBookmark(title: "A", url: "A", parent: root, context: context)
let bookmarkB = BookmarkEntity.makeBookmark(title: "B", url: "B", parent: root, context: context)
let bookmarkC = BookmarkEntity.makeBookmark(title: "C", url: "C", parent: root, context: context)

bookmarkA.setValue(nil, forKey: #keyPath(BookmarkEntity.isPendingDeletion))
bookmarkB.setValue(nil, forKey: #keyPath(BookmarkEntity.isPendingDeletion))
}
}

}

0 comments on commit afe414a

Please sign in to comment.