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

Fix SPM install + Add secure #422

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

matteocrippa
Copy link

This PR aims to fix one issue for SPM install:

  • fix warning for Info.plist as unhandled resource
  • remove the Combine extension because raise error and make the lib not working at all

Also it introduces and extra init param that will force the sqlite file to be encrypted ( I took as reference #145 )

Copy link
Owner

@JohnEstropia JohnEstropia left a 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
Copy link
Owner

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?

Copy link
Author

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"]
Copy link
Owner

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?

Copy link
Author

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) {
Copy link
Owner

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 NSFileProtectionTypes have some nuanced effects, and the errors that may occur need to be expressed in CoreStoreError.

Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants