[MOB-19274][MOB-15354] Use Disk for Storage Services #966
Conversation
Codecov Report
@@ Coverage Diff @@
## migrateOffUserDefaults #966 +/- ##
==========================================================
- Coverage 90.22% 90.01% -0.21%
==========================================================
Files 130 131 +1
Lines 8119 8221 +102
==========================================================
+ Hits 7325 7400 +75
- Misses 794 821 +27 |
AEPLifecycle/Tests/FunctionalTests/LifecycleFunctionalTests.swift
Outdated
Show resolved
Hide resolved
…ng when clearing something which doesn't exist.
|
|
||
| func setAppGroup(_ appGroup: String?) { | ||
| self.appGroup = appGroup | ||
| if let appGroup = appGroup { |
There was a problem hiding this comment.
Should this function also handle setting appGroup to nil (and clearing the associated appGroupUrl?
There was a problem hiding this comment.
also -- why not use get/set semantics on appGroup variable?
There was a problem hiding this comment.
Regardless of how it's done, setting/getting of appGroup needs to be done on queue as well.
There was a problem hiding this comment.
I agree that the get/set semantics would be cleaner, but the get/set function are part of the NamedCollectionProcessing protocol so we would be breaking compatibility by changing it. Or did you mean keeping the setAppGroup and getAppGroup functions and using the computed variable internally to handle the logic?
There was a problem hiding this comment.
Didn't consider that there was a pre-existing protocol -- makes sense, leave as is.
| guard let fileUrl = fileUrl(for: collectionName) else { | ||
| return | ||
| } | ||
| queue.sync { |
There was a problem hiding this comment.
should be async, this function doesn't need to return anything.
| } | ||
|
|
||
| func remove(collectionName: String, key: String) { | ||
| queue.sync { |
| } | ||
|
|
||
| func set(collectionName: String, key: String, value: Any?) { | ||
| guard let fileUrl = fileUrl(for: collectionName) else { |
There was a problem hiding this comment.
fileUrl needs to be called from within queue.sync to maintain ordering.
| private var appGroupUrl: URL? | ||
| private let fileManager = FileManager.default | ||
| private let LOG_TAG = "FileSystemNamedCollection" | ||
| var appGroup: String? |
| import Foundation | ||
|
|
||
| class FileSystemNamedCollection: NamedCollectionProcessing { | ||
| private let queue = DispatchQueue(label: "FileSystemNamedCollection.barrierQueue") |
There was a problem hiding this comment.
nit: isn't really a barrier, just a FIFO queue.
| set(key: key, value: encodedValue) | ||
| guard let encodedValue = try? encoder.encode(value) else {return} | ||
| let encodedString = String(data: encodedValue, encoding: .utf8) | ||
| set(key: key, value: encodedString) |
There was a problem hiding this comment.
in previous code a failure to encode resulted in a nil value set (removal) -- is the change (failing to set and bailing due to the new guard statement) intentional, or was the previous behavior preferred/depended on?
There was a problem hiding this comment.
This is a good point. Not too sure if we were depending on this to be nil in failure scenarios. I think it's a matter of preference. I would prefer to simply return, as setting nil on a value when encoding fails feels odd to me. What do you think @praveek? Should we remove the guard and set nil when encoding fails?
There was a problem hiding this comment.
I too prefer simply returning instead of setting nil value. However, I would like to maintain the same behavior as before since I'm uncertain whether other extensions depend on the previous behavior and want to keep all the changes for this migration only within Core.
| do { | ||
| try fileManager.createDirectory(at: adobeBaseUrl, withIntermediateDirectories: true) | ||
| } catch { | ||
| Log.error(label: adobeDirectory, "Failed to create storage directory with error: \(error)") |
There was a problem hiding this comment.
Can you check if directory exists and exit before attempting to create a directory?
There was a problem hiding this comment.
Apparently Apple doesn't recommend that approach. https://developer.apple.com/documentation/foundation/filemanager/1410277-fileexists
If you see the note towards the bottom which says: "Attempting to predicate behavior based on the current state of the file system or a particular file on the file system is not recommended. Doing so can cause odd behavior or race conditions. It’s far better to attempt an operation (such as loading a file or creating a directory), check for errors, and handle those errors gracefully than it is to try to figure out ahead of time whether the operation will succeed."
I have tested this function, and if the directory exists, it doesn't do anything. So in our case it works just fine.
Phase 1 of migration from UserDefaults as our storage mechanism. Next, we will be working on migrating existing data from UserDefaults to this new Disk storage.