add remote resource support #2
Conversation
hahn-kev
force-pushed
the
remote-resources
branch
2 times, most recently
from
5f64c2a to
8f29622
Compare
|
Yeah that's a good idea. Maybe I'll just make an attached file to the Word entity. |
…evant. Update db model snapshot test add remote resources support. add method to upload all pending local resources, and call from SyncWithResourceUpload helper method
hahn-kev
force-pushed
the
remote-resources
branch
from
e7b004c to
034c3dc
Compare
# Conflicts: # src/SIL.Harmony/DataModel.cs # src/SIL.Harmony/Db/CrdtRepository.cs
|
|
||
| namespace SIL.Harmony.Tests.ResourceTests; | ||
|
|
||
| public class RemoteResourcesTests : DataModelTestBase |
There was a problem hiding this comment.
In your tests you always know before hand if you're dealing with a local or remote resource.
I feel like maybe there should be a public Resource GetResource() method where Resource aggregates the local and remote resource looks something like:
Resource
{
Id,
LocalPath: string?,
RemoteId: string?,
IsLocal: bool,
IsRemote: bool,
}
I suppose a Harmony user could implement it themselves. A join is probably trivial if projected tables are enabled.
There was a problem hiding this comment.
yeah that's a good idea
There was a problem hiding this comment.
I think there was a slight misunderstanding.
You added a GetResources() (plural). My suggestion was for looking up a single resource.
I'm thinking about the case where I have a specific Resource ID, but I don't know if it's local or remote (or both), so I'd like to be able to get a CrdtResource just for that one resource.
a GetResource() would also cover up the fact that local and remote resources are queried differently, which would be a small bonus.
Can we do the join in SQL? I guess it doesn't matter for GetResources() (plural), because everything ends up in memory anyway.
Also I wonder if CrdtResource is a misleading name: the local-resources are explicitly non-crdt values, because they're local only. 🤔
There was a problem hiding this comment.
Ahh ok, that makes sense, yeah I can easily make that API (probably on top of the one I just made for now).
As for the name. Yeah that's true, I don't want to use just Resource as the class name as that's way too generic and there's other dotnet classes with the same name so I don't want the confusion. So maybe HarmonyResource?
There was a problem hiding this comment.
Yeah, you're right Resource is heavily used 😆.
I like HarmonyResource except for the fact that it would be the first type to use the word Harmony in it. 🤔
Some more ideas:
UnifiedResourceHybridResource
Hybrid sounds coolest, but I think it almost sounds too cool and fancy.
Personally, I think I'd opt for UnifiedResource.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| await _crdtRepository.AddLocalResource(localResource); | ||
| if (resourceService is not null) | ||
| { | ||
| var uploadResult = await resourceService.UploadResource(localResource.LocalPath); |
There was a problem hiding this comment.
Currently we roll everything back if the upload fails.
But, the local resource is still valid.
I think it would make more sense to follow the same path we would if resourceService was null. I.e. keep the local resource and add a CreateRemoteResourcePendingUploadChange
the purpose of this is to investiage how to connect CRDTs and binary files (called resources here) which can't easily be stored as a CRDT directly.
Basically there's RemoteResources, these a tracked in CRDTs and can be referenced by other objects. They contain a reference to a resource on a remote server (auth left up to the app). Then there's a LocalResource, this represents a resource stored locally. There are 3 states a resource can be in.
if the file is local only then the RemoteResource.RemoteId will be null, indicating it has not yet been uploaded.
if DataModel.GetLocalResource returns null that means the resource has not yet been downloaded.
There are 2 apis to help the application list resources that are pending upload or pending download.
There is also an API to help upload resources before a sync.