search

2020IP / TwentyTwenty.Storage

A cross-cloud storage abstraction
Other
100 stars 26 forks source link

Suggestion: store metadata file for local file provider with custom metadata #33

Closed nover closed 3 years ago

nover commented 3 years ago

We're currently in the process of migrating to this library from a home-grown abstraction layer for storage providers, and we're currently relying on being able to store custom metadata for both cloud and local stores (to be truly independent from cloud support), however, I see that the UpdateBlobPropertiesAsync for the local provider simply returns a completed task, effectively making it a no-op.

What I suggest is the following:

Once this method is called, store a containerName + blobName .meta (or similar) where this custom information is stored. It could be in json format or what-not.

When calling the GetBlobDescriptor and similar methods that return meta-data about the object, then check if a .meta file exists and merge the content that is "automatically guessed" with the custom data stored in the .meta file.

This would allow us to not maintain additional code when using the local provider.

And finally, would you accept an MR with the suggested changes?

ericgreenmix commented 3 years ago

I think that makes sense. Would definitely accept a PR with this work. Make sure that the move/delete/etc endpoint also handle that extra meta file as well. I would probably put a -meta or -descriptor in the file name and use the appropriate extension for the file format .json

nover commented 3 years ago

@ericgreenmix I'm also noticing that the Metadata:dictionary<string,string> is not returned by any of the GetBlobDescriptor methods on the storage providers - is that by design or an omission?

I'm asking because our use-case is setting some of this custom metadata, and we expect to be able to get it back and use it.

edit

I see the BlobDescriptor already contains a Metadata property that I could return the information in. Now it's a question of whether such an update (across all the providers) belong in an PR for this issue.