Task/adopt storage proto #42
Conversation
This is the bare minimum change necessary to get things basically working: - fs/write-file now happens on a different thread - stop using file-conn directly, that should only be touched through a protocol - file-conn/address-full-path doesn't exist anymore, inlined the old behavior, will deprecate in next few commits
Instead of just writing to the filesystem willy-nilly, use the connection's :store.
| [{:keys [fluree/conn]} {:keys [address json] :as _file-meta}] | ||
| (let [{:keys [method local]} (storage/parse-address address)] | ||
| (when (= "file" method) | ||
| (async/<!! (storage/write (:store conn) local json))))) |
There was a problem hiding this comment.
@dpetran This is already merged in a different branch so it's fine to leave as is for now, but we should not be reaching into the connections to write to storage. There is nothing in the connection contract that says it will have an accessible storage, and there's no guarantee that this particular connection will have a file storage implementation versus ipfs or whatever.
In this situation we should have either instantiated a file storage record independent of the connection for this handler to have access to, or add something to the connection api that wrote to storage on this handler's behalf.
There was a problem hiding this comment.
I had thought that the files that are being written here are actually part of the db, so it would be appropriate to reuse whatever storage mechanism the db is using. It just so happens that ledgers in server would always have a file storage method, but I don't think we should actually care what the substrate is.
If we're instantiating a separate storage record, should the storage functionality be its own library? The point of the protocol was to uncouple storage from the specific behavior a connection does on top of it, but it seems like it's not serving that purpose. Reaching into the db dep for the fluree.db.storage protocol to save non-db information feels weird, as opposed to a generic fluree.storage that isn't coupled with the logic on top of it.
I'm really reluctant to add yet another value-specific write method to the connection, that seems like the exact opposite of the direction we want - more coupling instead of less.
There was a problem hiding this comment.
Reaching into the connection object we have with a :store accessor assumes that connections we use later will support that. There's nothing in the connection contract that specifies that. A connection could be a record with a store field, or it could be a map, or some other type of java object that has no idea what to do when you call :store on it.
The purpose of the storage protocol was for us to reuse storage functionality across components that need it. We still need a connection api, and anything we do involving connections needs to use functions and methods defined in that api so that they don't assume internal implementation details of connection objects that may or may not hold in the future.
Update the db dep and update the file saving functionality to use the new store api.