-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Fix SPM install + Add secure #422
base: develop
Are you sure you want to change the base?
Fix SPM install + Add secure #422
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for this PR and for raising the issues presented here. I left some questions and I'll make considerations based on your answers. As for the file protection support, I'll have to think of a tighter solution for it since once those flags are on, a lot of disk behaviors start to affect the save
processing. In the meantime, I'll keep this PR open for now.
@@ -149,7 +149,8 @@ public struct From<O: DynamicObject> { | |||
) | |||
throw CoreStoreError.unknown | |||
} | |||
fetchRequest.entity = parentStack.entityDescription(for: Internals.EntityIdentifier(self.entityClass))! | |||
guard let entity = parentStack.entityDescription(for: Internals.EntityIdentifier(self.entityClass)) else { return } | |||
fetchRequest.entity = entity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What specific use-case does this resolve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was facing an error when debugging, dunno if related to change of the store in a secure way, so I preferred to guard and avoid a strong crash
@@ -40,7 +40,7 @@ let package = Package( | |||
name: "CoreStore", | |||
dependencies: [], | |||
path: "Sources", | |||
exclude: ["CoreStoreBridge.h", "CoreStoreBridge.m"] | |||
exclude: ["CoreStoreBridge.h", "CoreStoreBridge.m", "ObjectPublisher+Reactive.swift", "Info.plist"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these exclusions needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was generating error on compiling if I didn't exclude that
@@ -74,12 +74,13 @@ public final class SQLiteStore: LocalStorage { | |||
|
|||
- Warning: The default SQLite file location for the `LegacySQLiteStore` and `SQLiteStore` are different. If the app was depending on CoreStore's default directories prior to 2.0.0, make sure to use the `SQLiteStore.legacy(...)` factory methods to create the `SQLiteStore` instead of using initializers directly. | |||
*/ | |||
public init() { | |||
public init(secure: Bool = false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather implement this as a separate store, or a subclass of SQLiteStore
. The use of NSFileProtectionType
s have some nuanced effects, and the errors that may occur need to be expressed in CoreStoreError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be good, I had tried a quick approach without too many changes, prob a subclass is even better
This PR aims to fix one issue for SPM install:
Also it introduces and extra init param that will force the sqlite file to be encrypted ( I took as reference #145 )