Cache implementation #68
Conversation
TODO - needs cleanup
aashishpatil-g
requested review from
dconeybe,
maneesht,
ncooke3 and
stephenarosaj
|
/gemini review |
| @available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *) | ||
| struct QueryRequest<Variable: OperationVariable>: OperationRequest, Hashable, Equatable { | ||
| private(set) var operationName: String | ||
| private(set) var variables: Variable? |
There was a problem hiding this comment.
This is minor, but I would think the variables would map to an array of Variable (e.g. [Variable])?
There was a problem hiding this comment.
The Variable is typically a container struct that contains all the variables for that operation.
The Gen SDK creates schema specific Variable container structs that meet OperationVariable protocol requirements (Codable, Equatable, Hashable)
There was a problem hiding this comment.
The container struct could also contain an array for example.
There was a problem hiding this comment.
The fields of the container struct map to the names of the variables of that operation.
| public private(set) var lastError: DataConnectError? | ||
|
|
||
| /// Source of the query results (server, local cache, ...) | ||
| public private(set) var source: DataSource? |
There was a problem hiding this comment.
Why is this mutable again? Is it because the contents of this class will change each time an update is observed?
There was a problem hiding this comment.
They are observed properties that are updated each time results are updated.
Its a class so we end up updating the same instance of the Observed class.
|
|
||
| private let cache: Cache? | ||
|
|
||
| private var ttl: TimeInterval? = 10.0 // |
There was a problem hiding this comment.
Looks like a comment was intended? Also, does this need to be optional?
There was a problem hiding this comment.
I'll fix this now that we have the ttl field finalized. This might need some refactoring.
| case hydrated // JSON data is full hydrated and contains full data in the tree | ||
| case dehydrated // JSON data is dehydrated and only contains refs to actual data objects |
There was a problem hiding this comment.
These docs are sort of self-referencing. What does this concept of (de)hydration mean?
There was a problem hiding this comment.
Its explained in the go/fdc-sdk-caching internal doc (Design tab).
| // QueryRef requesting impacted | ||
| let requestor: (any QueryRefInternal)? | ||
|
|
||
| init(requestor: (any QueryRefInternal)? = nil) { |
There was a problem hiding this comment.
Why would we want to accept nil here?
There was a problem hiding this comment.
The ResultTreeProcessor isn't required to have a requestor and can function without it too. I kept it optional to maintain the decoupling.
| Go down the tree and convert them to entity nodes | ||
| For each Node | ||
| - extract globalID | ||
| - Get the EDO for the globalID |
There was a problem hiding this comment.
EntityDataObject - I need to improve that comment but details are in the go/fdc-sdk-caching doc (Design tab)
| - Get the EDO for the globalID | ||
| - extract scalars and update EDO with scalars | ||
| - for each array | ||
| - recursively process each object (could be scalar or composite) |
There was a problem hiding this comment.
Is the recursion being done by Apple's coders? I'm not noticing the recursive calls in below implementations.
There was a problem hiding this comment.
Yes, I am hooking into the codable process here.
| } | ||
|
|
||
| @available(iOS 15.0, macOS 12.0, tvOS 15.0, watchOS 8.0, *) | ||
| class SQLiteCacheProvider: CacheProvider { |
There was a problem hiding this comment.
All optional suggestions/questions
- Maybe worth generating some unit tests for this class?
- Check whether db storage location is in a location where it will be backed up or not and whether that's either is desired.
- Can/should this be an actor instead? I recall in FirebaseAuth we ran into some issues when mixing async/await with dispatch queues, mainly because queue.sync is blocking. I still have some doubts but generally I'm of the belief that only one cocnucrrency system should be used within an SDK (GCD or async/await).
There was a problem hiding this comment.
Haven't done unit tests - will do these in a later PR. I've tested this functionally for now.
Db storage is in the Documents folder
- Cache gets backed up
- Cache gets deleted when app is deleted
- (I think) if device encryption is enabled, data stays encrypted.
Reg sync ->
You are right about not mixing systems but async but its difficult to use async everywhere here.
Actor or async/await require calls from an async context. The dehydration process calls the cache provider from within the coding process which is sync (encode / init(decoder)) are sync.
The outer cache API is async so we aren't blocking the UI. The queue.sync is selectively used here to protect the access to the db resource.
The way it is used here is essentially converting this class into an actor using the following rules -
- internal dispatch queue is serial
- only public func (entry points) initiate a sync call
- private (internal) func never initiate a sync call but will always check to ensure they are called on queue.
- one public function never calls another public func (avoid deadlock).
|
@aashishpatil-g, left a first pass! |
4 participants
Implement caching in base (core) SDK
For googlers, refer to the doc go/fdc-sdk-caching