Story #8833

[Keep] Azure SDK update to add metadata to ListBlobs call

Added by Radhika Chippada about 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Keep
Target version:
Start date:
04/28/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Subtasks

Task #9072: Review branch list-blob-metadataResolvedRadhika Chippada


Related issues

Blocked by Arvados - Story #8832: [Keep] Azure SDK requirements analysis - update Azure SDK to include metadata in ListBlobs api callResolved

Blocks Arvados - Feature #8556: [Keep] Implement trash/untrash behavior in azure_blob_volumeResolved05/05/2016

History

#1 Updated by Radhika Chippada about 5 years ago

  • Category set to Keep
  • Assigned To set to Radhika Chippada
  • Target version changed from Arvados Future Sprints to 2016-04-27 sprint

#2 Updated by Radhika Chippada about 5 years ago

  • Status changed from New to In Progress

#3 Updated by Brett Smith about 5 years ago

  • Target version changed from 2016-04-27 sprint to Arvados Future Sprints

#4 Updated by Tom Clegg about 5 years ago

  • Target version changed from Arvados Future Sprints to 2016-05-11 sprint

#5 Updated by Tom Clegg about 5 years ago

  • Subject changed from [Keep] Azure SDK update to add metadata to ListBlobs callĀ  to [Keep] Azure SDK update to add metadata to ListBlobs call
  • Story points set to 0.5

#6 Updated by Tom Clegg about 5 years ago

In queue.go, the metadata looks like this

type QueueMetadataResponse struct {
    ApproximateMessageCount int
    UserDefinedMetadata     map[string]string
}

Can we make the blob metadata work the same way, something like this...?

type Blob struct {
    Name       string            `xml:"Name"`
    Properties BlobProperties    `xml:"Properties"`
    Metadata   map[string]string `xml:"Metadata"`
}

This way callers could say resp.Blobs[0].Metadata["Foo"] instead of iterating over a slice and comparing keys.

#7 Updated by Tom Clegg about 5 years ago

I couldn't find a way to make encoding/xml do this by itself, but if we make a BlobMetadata type that implements xml.Unmarshaler, we can convert it to a map while decoding, so blobs[i].Metadata["Foo"] works out of the box:

https://play.golang.org/p/P1xi66oeJw

#8 Updated by Radhika Chippada about 5 years ago

Tom, thanks for the update. It's more appropriate implementation. I updated the test accordingly.

#9 Updated by Tom Clegg about 5 years ago

This looks better, thanks.

IMO the test case is verbose, making it less obvious what it's doing. How about something like this:

var expectMeta map[string]BlobMetadata

for i := 0; i < 5; i++ {
    name := randString(20)
    c.Assert(cli.putSingleBlockBlob(cnt, name, []byte("Hello, world!")), chk.IsNil)
    c.Assert(cli.SetBlobMetadata(cnt, name, map[string]string{
        "foo": name,
        "bar_BAZ": "waz qux",
    }), chk.IsNil)
    expectMeta[name] = BlobMetadata{
        "Foo": name,
        "Bar_baz": "waz qux",
    }
}

// ...call ListBlobs and copy results into respBlobs...

for name, metadata := range expectMeta {
    c.Check(respBlobs[name].Metadata, chk.DeepEquals, expectMeta[name])
}

I'd also say skip the cli.GetBlobMetadata() call and the associated checks. That stuff is already tested by TestGetAndSetMetadata().

(sort.Strings(blobs) seems superfluous too)

Might be worth writing one blob without metadata, if only for the sake of exercising that case in the decoder.

c.Check(0, chk.Equals, len(respBlobs[nameWithoutMetadata].Metadata))

#10 Updated by Radhika Chippada about 5 years ago

Tom:

  • Updated the test as suggested and added a blob with no metadata
  • While testing I realized that the metadata returned by ListBlobs should match the same behavior as GetBlobMetadata, where the keys are in lowercase. Made this update in blob.go as well.

Thanks.

#11 Updated by Tom Clegg about 5 years ago

golint: blob_test.go:552:12: should omit 2nd value from range; this loop is equivalent to `for name := range ...`

With that fixed, LGTM @ commit:c922124. Should probably squash this into one commit before making a PR, to make it easier for upstream to review...

#12 Updated by Radhika Chippada about 5 years ago

  • Status changed from In Progress to Resolved

Also available in: Atom PDF