Enhanced metadata #25
Conversation
|
I don't see mentioned |
|
It's coming! I couldn't finish on friday as you came to disturb me! :p |
| var blockBlob = this.container.Value.GetBlockBlobReference(file.Path); | ||
| await blockBlob.UploadFromStreamAsync(data); | ||
| blockBlob.Properties.ContentType = contentType; | ||
| blockBlob.Properties.CacheControl = "max-age=300, must-revalidate"; |
There was a problem hiding this comment.
Maybe there is the chance to address #27 using freshly created FileProperties :)
There was a problem hiding this comment.
Yeah! Saw that, I'm on it ;)
There was a problem hiding this comment.
Great, the more I see this the more I think this is completely fucked up :D
| blockBlob.Properties.CacheControl = "max-age=300, must-revalidate"; | ||
| await blockBlob.SetPropertiesAsync(); | ||
| return new Internal.AzureFileReference(blockBlob); | ||
| return new Internal.AzureFileReference(blockBlob, withMetadata: true); |
There was a problem hiding this comment.
I think we should make metadata loading optional here and add a withMetadata argument. Most save operation won't need them.
There was a problem hiding this comment.
In fact, it is not the loading that we specify here, but the fact that the properties and metadata are loaded :)
There was a problem hiding this comment.
Yes, the GetBlockBlobReference method initializes a reference with an empty dictionary for metadata, and the properties are set just the line before
There was a problem hiding this comment.
I think that since it's not a GetBlobReferenceFromServerAsync, it won't fetch pre-existing Metadata or Properties and one might end up overriding preexisting metadata.
There was a problem hiding this comment.
Oh, you're right, didn't think of using this methods with existing Blobs...
There is already an issue in the current version with the properties being overwritten so.
Maybe we should check it the blob exists, and auto fetch properties in this case?
|
Ready to merge! I need your approbation @sandorfr and @arnaudauroux :) |
| { | ||
| var reference = InternalGetOrCreateAsync(file); | ||
| if (File.Exists(reference.FileSystemPath)) | ||
| var data = File.ReadAllBytes(fileSystemPath); |
There was a problem hiding this comment.
I think we should refrain from using ReadAllBytes kind of API in this library internals as they might be put the memory under high pressure. We should rely on a stream and a buffer using TransformBlock : https://msdn.microsoft.com/en-us/library/system.security.cryptography.hashalgorithm.transformblock(v=vs.110).aspx
There was a problem hiding this comment.
@asiffermann this is not highest priority and might be tracked as an issue if you don't feel like addressing it now.
There was a problem hiding this comment.
I remembered I had the same issue here : https://github.com/geeklearningio/gl-dotnet-storage/blob/develop/src/GeekLearning.Storage.Azure/Internal/AzureFileReference.cs#L68
There was a problem hiding this comment.
Better and simpler: md5.ComputeHash has an override that takes a Stream :)
Issues
Metadata handling
Breaking changes
withMetadataparameterIFileReferencevia aIFilePropertiesinstance. For example, you should now access the Length like this:file.Properties.LengthAddMetadatamethod onIStoreor onIFileReference. You should now modify metadata on theIFilePropertiesdirectly, and then call the methodSavePropertiesAsync