feat: Add support for external cache storage #412
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,18 +57,24 @@ protocol FeatureFlagCaching { | |
| func saveCachedData(_ storedItems: StoredItems, cacheKey: String, contextHash: String, lastUpdated: Date, etag: String?) | ||
| } | ||
|
|
||
| extension MobileKey { | ||
| func cacheKey() -> String { | ||
|
There was a problem hiding this comment. What do you think about having the cache define it's own key? I don't see any reason why completely different implementations need to use the same format as this one. There was a problem hiding this comment. I didn't consider this as an important thing to add (trying to make the PR smaller), but this certainly can be done. Ideally, a wider refactoring is needed here. The parameter name
use cacheKey that look different, and are generated differently. The first one usually looks like There was a problem hiding this comment. That's fair. It may be something we add ourselves then after this lands. @tanderson-ld that is something you and I can decide on then if we want this now, later, or not at all. |
||
| let cacheKey: String | ||
| if let bundleId = Bundle.main.bundleIdentifier { | ||
| cacheKey = "\(Util.sha256base64(bundleId)).\(Util.sha256base64(self))" | ||
| } else { | ||
| cacheKey = Util.sha256base64(self) | ||
| } | ||
| return "com.launchdarkly.client.\(cacheKey)" | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Is this extension organizational or is there another reason for adding it? |
||
|
|
||
| final class FeatureFlagCache: FeatureFlagCaching { | ||
| let keyedValueCache: KeyedValueCaching | ||
| let maxCachedContexts: Int | ||
|
|
||
| init(serviceFactory: ClientServiceCreating, mobileKey: MobileKey, maxCachedContexts: Int) { | ||
| self.keyedValueCache = serviceFactory.makeKeyedValueCache(cacheKey: mobileKey.cacheKey()) | ||
| self.maxCachedContexts = maxCachedContexts | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,25 +1,14 @@ | ||
| import Foundation | ||
| import OSLog | ||
| // sourcery: autoMockable | ||
| public protocol KeyedValueCaching { | ||
| func set(_ value: Data, forKey: String) | ||
| func data(forKey: String) -> Data? | ||
| func removeObject(forKey: String) | ||
| func removeAll() | ||
| func keys() -> [String] | ||
| } | ||
|
|
||
| public extension LDConfig { | ||
| typealias CacheFactory = (_ cacheKey: String?, _ logger: OSLog) -> KeyedValueCaching | ||
|
There was a problem hiding this comment. Is there any reason to make the cache key optional? It forces a decision on what a default key would be in your cache implementations which I would prefer to avoid. There was a problem hiding this comment. This was dictated by the There was a problem hiding this comment. I think that's actually in line with my later comment about this conversion being something associated with a specific cache implementation. Our existing implementation could take its cache key, but when doing the conversion bit, it can ignore that key then since it knows it didn't use to use it. Also, at this point, I think we could argue that our cache converter could probably be cleaned up some. I don't think we have to support a v5 -> v9 migration path. So we could probably remove those steps entirely. What do you think @tanderson-ld There was a problem hiding this comment. I agree. Dropping support for converting v5 to remove the optionality of the cacheKey is a good tradeoff. I think these are the pertinent lines. |
||
| } | ||
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.
With caching implementations being as wildly different as they are, it seem like the
convertCacheDatashould be implemented alongside the cache implementation.So I think the cache prototype probably needs a method to retrieve an optional converter. If provided, then we pass in the list of mobile keys, and the max cached contexts option. From there, the implementation is cache specific.
We should also conversion migration and conversion as the same concept. If someone wants to migrate between two completely different storage technologies, they can write a custom cacher to handle that. Seems like it would be straight forward enough.