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

Log messages/crashes in CoreDataManager in Storage framework #2366

Merged
merged 14 commits into from
Jun 3, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
16 changes: 16 additions & 0 deletions Storage/Storage.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,11 @@
028296F3237D404F00E84012 /* ProductVariation+CoreDataProperties.swift in Sources */ = {isa = PBXBuildFile; fileRef = 028296EF237D404F00E84012 /* ProductVariation+CoreDataProperties.swift */; };
028296F4237D404F00E84012 /* Attribute+CoreDataClass.swift in Sources */ = {isa = PBXBuildFile; fileRef = 028296F0237D404F00E84012 /* Attribute+CoreDataClass.swift */; };
028296F5237D404F00E84012 /* Attribute+CoreDataProperties.swift in Sources */ = {isa = PBXBuildFile; fileRef = 028296F1237D404F00E84012 /* Attribute+CoreDataProperties.swift */; };
02A098272480D160002F8C7A /* MockCrashLogger.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02A098262480D160002F8C7A /* MockCrashLogger.swift */; };
02D45649231CFB27008CF0A9 /* StatsVersionBannerVisibility.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02D45648231CFB27008CF0A9 /* StatsVersionBannerVisibility.swift */; };
02DA64172313C26400284168 /* StatsVersionBySite.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02DA64162313C26400284168 /* StatsVersionBySite.swift */; };
02DA64192313C2AA00284168 /* StatsVersion.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02DA64182313C2AA00284168 /* StatsVersion.swift */; };
02EAB6D72480A86D00FD873C /* CrashLogger.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02EAB6D62480A86D00FD873C /* CrashLogger.swift */; };
26577519243D808B003168A5 /* WooCommerceModelV26toV27.xcmappingmodel in Sources */ = {isa = PBXBuildFile; fileRef = 26577518243D808B003168A5 /* WooCommerceModelV26toV27.xcmappingmodel */; };
450106892399AC7400E24722 /* TaxClass+CoreDataClass.swift in Sources */ = {isa = PBXBuildFile; fileRef = 450106882399AC7400E24722 /* TaxClass+CoreDataClass.swift */; };
4501068B2399AC9B00E24722 /* TaxClass+CoreDataProperties.swift in Sources */ = {isa = PBXBuildFile; fileRef = 4501068A2399AC9B00E24722 /* TaxClass+CoreDataProperties.swift */; };
Expand Down Expand Up @@ -156,11 +158,13 @@
028296F0237D404F00E84012 /* Attribute+CoreDataClass.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Attribute+CoreDataClass.swift"; sourceTree = "<group>"; };
028296F1237D404F00E84012 /* Attribute+CoreDataProperties.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "Attribute+CoreDataProperties.swift"; sourceTree = "<group>"; };
028F00652331605000E6C283 /* Model 20.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "Model 20.xcdatamodel"; sourceTree = "<group>"; };
02A098262480D160002F8C7A /* MockCrashLogger.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockCrashLogger.swift; sourceTree = "<group>"; };
02A9F16A22F9873600EE36EA /* Model 19.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "Model 19.xcdatamodel"; sourceTree = "<group>"; };
02D45648231CFB27008CF0A9 /* StatsVersionBannerVisibility.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StatsVersionBannerVisibility.swift; sourceTree = "<group>"; };
02DA64162313C26400284168 /* StatsVersionBySite.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StatsVersionBySite.swift; sourceTree = "<group>"; };
02DA64182313C2AA00284168 /* StatsVersion.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = StatsVersion.swift; sourceTree = "<group>"; };
02E4F5E223CD552F003B0010 /* Model 26.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "Model 26.xcdatamodel"; sourceTree = "<group>"; };
02EAB6D62480A86D00FD873C /* CrashLogger.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CrashLogger.swift; sourceTree = "<group>"; };
262CD20224317C2F00932241 /* Model 27.xcdatamodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcdatamodel; path = "Model 27.xcdatamodel"; sourceTree = "<group>"; };
26577518243D808B003168A5 /* WooCommerceModelV26toV27.xcmappingmodel */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcmappingmodel; path = WooCommerceModelV26toV27.xcmappingmodel; sourceTree = "<group>"; };
450106882399AC7400E24722 /* TaxClass+CoreDataClass.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TaxClass+CoreDataClass.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -326,6 +330,14 @@
/* End PBXFrameworksBuildPhase section */

/* Begin PBXGroup section */
02A098252480D155002F8C7A /* Mocks */ = {
isa = PBXGroup;
children = (
02A098262480D160002F8C7A /* MockCrashLogger.swift */,
);
path = Mocks;
sourceTree = "<group>";
};
4DD3D0BDDE216A90FC1335D7 /* Pods */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -402,6 +414,7 @@
B52B0F7820AA287C00477698 /* StorageManagerType.swift */,
B505F6DF20BEEA8100BB1B69 /* StorageType.swift */,
D87F61522265AA230031A13B /* FileStorage.swift */,
02EAB6D62480A86D00FD873C /* CrashLogger.swift */,
);
path = Protocols;
sourceTree = "<group>";
Expand Down Expand Up @@ -454,6 +467,7 @@
isa = PBXGroup;
children = (
D87F61582265AED70031A13B /* Mock data */,
02A098252480D155002F8C7A /* Mocks */,
B54CA5C520A4BFC800F38CD1 /* Tools */,
B52B0F7D20AA2F1200477698 /* Extensions */,
B59E11DC20A9F1E6004121A4 /* CoreData */,
Expand Down Expand Up @@ -814,6 +828,7 @@
B54CA5BD20A4BD3B00F38CD1 /* NSManagedObjectContext+Storage.swift in Sources */,
933A27302222344D00C2143A /* Logging.swift in Sources */,
747453A52242C85E00E0B5EE /* ProductDefaultAttribute+CoreDataClass.swift in Sources */,
02EAB6D72480A86D00FD873C /* CrashLogger.swift in Sources */,
746A9D21214078080013F6FF /* TopEarnerStats+CoreDataClass.swift in Sources */,
74B7D6AD20F90CBB002667AC /* OrderNote+CoreDataClass.swift in Sources */,
B52B0F7920AA287C00477698 /* StorageManagerType.swift in Sources */,
Expand Down Expand Up @@ -892,6 +907,7 @@
9302E3AC220E1CE900DA5018 /* CoreDataIterativeMigratorTests.swift in Sources */,
B59E11E020A9F5E6004121A4 /* Constants.swift in Sources */,
B54CA5C320A4BF6900F38CD1 /* NSManagedObjectContextStorageTests.swift in Sources */,
02A098272480D160002F8C7A /* MockCrashLogger.swift in Sources */,
B54CA5C720A4BFDC00F38CD1 /* DummyStack.swift in Sources */,
B54CA5C220A4BF6900F38CD1 /* NSManagedObjectStorageTests.swift in Sources */,
D87F61572265AD980031A13B /* FileStorageTests.swift in Sources */,
Expand Down
36 changes: 30 additions & 6 deletions Storage/Storage/CoreData/CoreDataManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,17 @@ public class CoreDataManager: StorageManagerType {
///
public let name: String

private let crashLogger: CrashLogger

/// Designated Initializer.
///
/// - Parameter name: Identifier to be used for: [database, data model, container].
///
/// - Important: This should *match* with your actual Data Model file!.
///
public init(name: String) {
public init(name: String, crashLogger: CrashLogger) {
self.name = name
self.crashLogger = crashLogger
}


Expand All @@ -37,21 +39,38 @@ public class CoreDataManager: StorageManagerType {
container.persistentStoreDescriptions = [storeDescription]

container.loadPersistentStores { [weak self] (storeDescription, error) in
shiki marked this conversation as resolved.
Show resolved Hide resolved
guard let `self` = self, let error = error else {
guard let `self` = self, let errorLoadingPersistentStores = error else {
shiki marked this conversation as resolved.
Show resolved Hide resolved
return
}

DDLogError("⛔️ [CoreDataManager] loadPersistentStore failed. Attempting to recover... \(error)")
DDLogError("⛔️ [CoreDataManager] loadPersistentStore failed. Attempting to recover... \(errorLoadingPersistentStores)")

/// Backup the old Store
///
do {
let sourceURL = self.storeURL
let backupURL = sourceURL.appendingPathExtension("~")
try FileManager.default.copyItem(at: sourceURL, to: backupURL)
try FileManager.default.removeItem(at: sourceURL)
} catch {
fatalError("☠️ [CoreDataManager] Cannot backup Store: \(error)")
let message = "☠️ [CoreDataManager] Cannot backup Store: \(error)"
self.crashLogger.logMessage(message,
properties: ["loadPersistentStoresError": errorLoadingPersistentStores,
"backupError": error],
shiki marked this conversation as resolved.
Show resolved Hide resolved
level: .fatal)
fatalError(message)
}

/// Remove the old Store
Copy link
Member

Choose a reason for hiding this comment

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

good idea to separate the removeItem: from the copyItem: 👍

///
do {
try FileManager.default.removeItem(at: self.storeURL)
} catch {
let message = "☠️ [CoreDataManager] Cannot remove Store: \(error)"
self.crashLogger.logMessage(message,
properties: ["loadPersistentStoresError": errorLoadingPersistentStores,
"removeStoreError": error],
level: .fatal)
fatalError(message)
}

/// Retry!
Expand All @@ -61,7 +80,12 @@ public class CoreDataManager: StorageManagerType {
return
}

fatalError("☠️ [CoreDataManager] Recovery Failed! \(error) [\(error.userInfo)]")
let message = "☠️ [CoreDataManager] Recovery Failed! \(error) [\(error.userInfo)]"
self?.crashLogger.logMessage(message,
properties: ["loadPersistentStoresError": errorLoadingPersistentStores,
"retryError": error],
level: .fatal)
fatalError(message)
}
}

Expand Down
22 changes: 22 additions & 0 deletions Storage/Storage/Protocols/CrashLogger.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
//import Foundation
shiki marked this conversation as resolved.
Show resolved Hide resolved

/// The level of severity, that is currently based on `SentrySeverity`.
public enum SeverityLevel {
case fatal
case error
case warning
case info
case debug
}

/// Logs crashes or messages at a given severity level.
public protocol CrashLogger {
/**
Writes a message to the Crash Logging system.
- Parameters:
- message: The message
- properties: A dictionary containing additional information about this message
- level: The level of severity to report
*/
func logMessage(_ message: String, properties: [String: Any]?, level: SeverityLevel)
}
12 changes: 6 additions & 6 deletions Storage/StorageTests/CoreData/CoreDataManagerTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,23 +10,23 @@ class CoreDataManagerTests: XCTestCase {
/// Verifies that the Data Model URL contains the ContextIdentifier String.
///
func testModelUrlMapsToDataModelWithContextIdentifier() {
let manager = CoreDataManager(name: "WooCommerce")
let manager = CoreDataManager(name: "WooCommerce", crashLogger: MockCrashLogger())
XCTAssertEqual(manager.modelURL.lastPathComponent, "WooCommerce.momd")
XCTAssertNoThrow(manager.managedModel)
}

/// Verifies that the Store URL contains the ContextIdentifier string.
///
func testStorageUrlMapsToSqliteFileWithContextIdentifier() {
let manager = CoreDataManager(name: "WooCommerce")
let manager = CoreDataManager(name: "WooCommerce", crashLogger: MockCrashLogger())
XCTAssertEqual(manager.storeURL.lastPathComponent, "WooCommerce.sqlite")
XCTAssertEqual(manager.storeDescription.url?.lastPathComponent, "WooCommerce.sqlite")
}

/// Verifies that the PersistentContainer properly loads the sqlite database.
///
func testPersistentContainerLoadsExpectedDataModelAndSqliteDatabase() {
let manager = CoreDataManager(name: "WooCommerce")
let manager = CoreDataManager(name: "WooCommerce", crashLogger: MockCrashLogger())
let container = manager.persistentContainer

XCTAssertEqual(container.managedObjectModel, manager.managedModel)
Expand All @@ -36,14 +36,14 @@ class CoreDataManagerTests: XCTestCase {
/// Verifies that the ContextManager's viewContext matches the PersistenContainer.viewContext
///
func testViewContextPropertyReturnsPersistentContainerMainContext() {
let manager = CoreDataManager(name: "WooCommerce")
let manager = CoreDataManager(name: "WooCommerce", crashLogger: MockCrashLogger())
XCTAssertEqual(manager.viewStorage as? NSManagedObjectContext, manager.persistentContainer.viewContext)
}

/// Verifies that performBackgroundTask effectively runs received closure in BG.
///
func testPerformTaskInBackgroundEffectivelyRunsReceivedClosureInBackgroundThread() {
let manager = CoreDataManager(name: "WooCommerce")
let manager = CoreDataManager(name: "WooCommerce", crashLogger: MockCrashLogger())
let expectation = self.expectation(description: "Background")

manager.performBackgroundTask { (_) in
Expand All @@ -57,7 +57,7 @@ class CoreDataManagerTests: XCTestCase {
/// Verifies that derived context is instantiated correctly.
///
func testDerivedStorageIsInstantiatedCorrectly() {
let manager = CoreDataManager(name: "WooCommerce")
let manager = CoreDataManager(name: "WooCommerce", crashLogger: MockCrashLogger())
let viewContext = (manager.viewStorage as? NSManagedObjectContext)
let derivedContext = (manager.newDerivedStorage() as? NSManagedObjectContext)

Expand Down
7 changes: 7 additions & 0 deletions Storage/StorageTests/Mocks/MockCrashLogger.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import Storage

struct MockCrashLogger: CrashLogger {
func logMessage(_ message: String, properties: [String: Any]?, level: SeverityLevel) {
// no-op
}
}
2 changes: 1 addition & 1 deletion WooCommerce/Classes/ServiceLocator/ServiceLocator.swift
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ final class ServiceLocator {

/// CoreData Stack
///
private static var _storageManager = CoreDataManager(name: WooConstants.databaseStackName)
private static var _storageManager = CoreDataManager(name: WooConstants.databaseStackName, crashLogger: DefaultCrashLogger())

/// Cocoalumberjack DDLog
///
Expand Down
26 changes: 26 additions & 0 deletions WooCommerce/Classes/Tools/Logging/DefaultCrashLogger.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import AutomatticTracks
import Sentry
import Storage

final class DefaultCrashLogger: CrashLogger {
shiki marked this conversation as resolved.
Show resolved Hide resolved
shiki marked this conversation as resolved.
Show resolved Hide resolved
func logMessage(_ message: String, properties: [String: Any]?, level: SeverityLevel) {
CrashLogging.logMessage(message, properties: properties, level: SentrySeverity(level: level))
}
}

private extension SentrySeverity {
init(level: SeverityLevel) {
switch level {
case .fatal:
self = .fatal
case .error:
self = .error
case .warning:
self = .warning
case .info:
self = .info
case .debug:
self = .debug
}
}
}
4 changes: 4 additions & 0 deletions WooCommerce/WooCommerce.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@
02EA6BF82435E80600FFF90A /* ImageDownloader.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02EA6BF72435E80600FFF90A /* ImageDownloader.swift */; };
02EA6BFA2435E92600FFF90A /* KingfisherImageDownloader+ImageDownloadable.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02EA6BF92435E92600FFF90A /* KingfisherImageDownloader+ImageDownloadable.swift */; };
02EA6BFC2435EC3500FFF90A /* MockImageDownloader.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02EA6BFB2435EC3500FFF90A /* MockImageDownloader.swift */; };
02EAB6D92480AA4900FD873C /* DefaultCrashLogger.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02EAB6D82480AA4900FD873C /* DefaultCrashLogger.swift */; };
02EEB5C42424AFAA00B8A701 /* TextFieldTableViewCell.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02EEB5C22424AFAA00B8A701 /* TextFieldTableViewCell.swift */; };
02EEB5C52424AFAA00B8A701 /* TextFieldTableViewCell.xib in Resources */ = {isa = PBXBuildFile; fileRef = 02EEB5C32424AFAA00B8A701 /* TextFieldTableViewCell.xib */; };
02F49ADA23BF356E00FA0BFA /* TitleAndTextFieldTableViewCell.ViewModel+State.swift in Sources */ = {isa = PBXBuildFile; fileRef = 02F49AD923BF356E00FA0BFA /* TitleAndTextFieldTableViewCell.ViewModel+State.swift */; };
Expand Down Expand Up @@ -1096,6 +1097,7 @@
02EA6BF72435E80600FFF90A /* ImageDownloader.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ImageDownloader.swift; sourceTree = "<group>"; };
02EA6BF92435E92600FFF90A /* KingfisherImageDownloader+ImageDownloadable.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "KingfisherImageDownloader+ImageDownloadable.swift"; sourceTree = "<group>"; };
02EA6BFB2435EC3500FFF90A /* MockImageDownloader.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = MockImageDownloader.swift; sourceTree = "<group>"; };
02EAB6D82480AA4900FD873C /* DefaultCrashLogger.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = DefaultCrashLogger.swift; sourceTree = "<group>"; };
02EEB5C22424AFAA00B8A701 /* TextFieldTableViewCell.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = TextFieldTableViewCell.swift; sourceTree = "<group>"; };
02EEB5C32424AFAA00B8A701 /* TextFieldTableViewCell.xib */ = {isa = PBXFileReference; lastKnownFileType = file.xib; path = TextFieldTableViewCell.xib; sourceTree = "<group>"; };
02F49AD923BF356E00FA0BFA /* TitleAndTextFieldTableViewCell.ViewModel+State.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = "TitleAndTextFieldTableViewCell.ViewModel+State.swift"; sourceTree = "<group>"; };
Expand Down Expand Up @@ -3102,6 +3104,7 @@
children = (
933A27362222354600C2143A /* Logging.swift */,
B5DB01B42114AB2D00A4F797 /* CrashLogging.swift */,
02EAB6D82480AA4900FD873C /* DefaultCrashLogger.swift */,
);
path = Logging;
sourceTree = "<group>";
Expand Down Expand Up @@ -4748,6 +4751,7 @@
B517EA1D218B41F200730EC4 /* String+Woo.swift in Sources */,
02564A8C246CE38E00D6DB2A /* SwappableSubviewContainerView.swift in Sources */,
024DF31223742B18006658FE /* AztecItalicFormatBarCommand.swift in Sources */,
02EAB6D92480AA4900FD873C /* DefaultCrashLogger.swift in Sources */,
D83F5933225B2EB900626E75 /* ManualTrackingViewController.swift in Sources */,
B57C744A20F5649300EEFC87 /* EmptyStoresTableViewCell.swift in Sources */,
027D67D1245ADDF40036B8DB /* FilterTypeViewModel+Helpers.swift in Sources */,
Expand Down
Loading