Project

General

Profile

Actions

Idea #8833

closed

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

Added by Radhika Chippada about 8 years ago. Updated almost 8 years ago.

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

Subtasks 1 (0 open1 closed)

Task #9072: Review branch list-blob-metadataResolvedRadhika Chippada04/28/2016Actions

Related issues

Blocked by Arvados - Idea #8832: [Keep] Azure SDK requirements analysis - update Azure SDK to include metadata in ListBlobs api callResolvedRadhika ChippadaActions
Blocks Arvados - Feature #8556: [Keep] Implement trash/untrash behavior in azure_blob_volumeResolvedRadhika Chippada05/05/2016Actions
Actions #1

Updated by Radhika Chippada almost 8 years ago

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

Updated by Radhika Chippada almost 8 years ago

  • Status changed from New to In Progress
Actions #3

Updated by Brett Smith almost 8 years ago

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

Updated by Tom Clegg almost 8 years ago

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

Updated by Tom Clegg almost 8 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
Actions #6

Updated by Tom Clegg almost 8 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.

Actions #7

Updated by Tom Clegg almost 8 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

Actions #8

Updated by Radhika Chippada almost 8 years ago

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

Actions #9

Updated by Tom Clegg almost 8 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))

Actions #10

Updated by Radhika Chippada almost 8 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.

Actions #11

Updated by Tom Clegg almost 8 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...

Actions #12

Updated by Radhika Chippada almost 8 years ago

  • Status changed from In Progress to Resolved
Actions

Also available in: Atom PDF