Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

provide missing SetBlobProperties function and add missing properties to GetBlobProperties #374

Merged
merged 3 commits into from
Aug 8, 2016

Conversation

schreibe72
Copy link

Add following properties to GetBlobProperties:

  • Cache-Control
  • Cache-Language

Add function SetBlobProperties to set ContentSettings like:

  • Content-MD5
  • Content-Language
  • Content-Encoding
  • Content-Type
  • Cache-Control

Add Test for SetBlobProperties.

@msftclas
Copy link

msftclas commented Aug 5, 2016

Hi @schreibe72, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

@ahmetb
Copy link
Contributor

ahmetb commented Aug 5, 2016

@schreibe72 thank you for your pull request, please ping me once you signed the CLA and I'll review.

@schreibe72
Copy link
Author

@ahmetalpbalkan I have signed the CLA and send via DocuSign.

@ahmetb
Copy link
Contributor

ahmetb commented Aug 5, 2016

@schreibe72 I guess there is a processing delay in that case, we can come back to that later. I'll leave some comments now.

@@ -122,6 +125,16 @@ type BlobProperties struct {
LeaseStatus string `xml:"LeaseStatus"`
}

// ContentSetting contains various properties of a blob and is an entry
// in SetBlobProperties
type ContentSetting struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this BlobHeaders? (more about this below)

@ahmetb
Copy link
Contributor

ahmetb commented Aug 5, 2016

It looks like Set Blob Properties and Get Blob Properties (+List Blobs) work a bit weird as in "Set" accepts x-ms-* headers and "Get/List" return XML fields named properly.

Therefore it appears like using the same struct both on response deserialization path from 'Get/List' and as headers in 'Set' is going to be a bit challenging.

@ahmetb
Copy link
Contributor

ahmetb commented Aug 5, 2016

@schreibe72 just got this crazy idea. What if we annotate this ContentSetting struct (or better: BlobHeaders or struct) with BOTH "xml" and "header" tags?

This way:

  1. we can embed this struct into type BlobPropertiesResponse and thanks to xml tag, it will deserialize just fine
  2. For SetBlobProperties, func headersFromStruct(v interface{}) (map[..], error) will do the job and read from "header" field tag.
  3. For GetBlobProperties we write a new method called func structFromHeaders(map[string]string, v interface{}) error which looks at the "header" field tag and deserializes the headers in the response of Get Blob Properties into our struct and just return it! This way we don't have to populate its fields manually?

extract struct to map conversion to an extra function headersFromStruct
@@ -590,6 +602,8 @@ func (b BlobStorageClient) GetBlobProperties(container, name string) (*BlobPrope
ContentLength: contentLength,
ContentEncoding: resp.headers.Get("Content-Encoding"),
ContentType: resp.headers.Get("Content-Type"),
CacheControl: resp.headers.Get("Cache-Control"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schreibe72 thanks for making the changes. I think we should somehow embed BlobHeaders struct into BlobProperties struct and have some deserializer that does map-->struct. I feel like it might be a lot of work whereas this patch is probably solving your problem. What do you think?

@schreibe72
Copy link
Author

i would split this in to two pull requests. One is for the new function setBlobProperties, the other for refactoring getBlobProperties.
The deserializer is a bit more tricky, because there are not only string types. The other thing is, that if we change the struct BlobProperties some depending code might break.

@ahmetb
Copy link
Contributor

ahmetb commented Aug 8, 2016

@schreibe72 fair point. Thanks.

@ahmetb ahmetb merged commit 9586c16 into Azure:master Aug 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants