storage: return file metadata on read #11212
Conversation
product-auto-label
bot
added
the
api: storage
I'd say we can just document that it is unavailable via the JSON API. There are other fields in ReaderObjectAttrs which are only populated sometimes. I need to look at the rest of the code more deeply; just let me know when it's ready for review. |
jshore1296
force-pushed
the
metadata-on-read
branch
from
462536d to
b882d74
Compare
|
Alright, this is ready for review. |
|
@tritone sending a ping in case the previous comment didn't notify you |
storage/client_test.go
Outdated
| @@ -795,6 +795,62 @@ func TestOpenReaderEmulated(t *testing.T) { | |||
| }) | |||
| } | |||
|
|
|||
| func TestOpenReaderEmulated_Metadata(t *testing.T) { | |||
| transportClientTest(skipHTTP("metadata on read not supported by JSON api"), t, func(t *testing.T, ctx context.Context, project, bucket string, client storageClient) { | |||
There was a problem hiding this comment.
I believe the HTTP client should use XML by default, does the test not pass with a default HTTP client?
| t.Fatalf("closing object: %v", err) | ||
| } | ||
| if _, err := veneerClient.Bucket(bucket).Object(want.Name).Update(ctx, ObjectAttrsToUpdate{ | ||
| Metadata: map[string]string{ |
There was a problem hiding this comment.
Let's do at least 2 keys in here just to make sure the decoding logic works correctly.
| @@ -1133,6 +1133,7 @@ func (c *grpcStorageClient) NewRangeReader(ctx context.Context, params *newRange | |||
| CacheControl: obj.GetCacheControl(), | |||
| LastModified: obj.GetUpdateTime().AsTime(), | |||
| Metageneration: obj.GetMetageneration(), | |||
| Metadata: obj.GetMetadata(), | |||
There was a problem hiding this comment.
I was concerned this wouldn't be sufficient because we didn't have it in the unmarshaler for gRPC, but it looks like we did that part already so this should be fine.
There was a problem hiding this comment.
Yeah, I was surprised that was already in there!
storage/integration_test.go
Outdated
| @@ -204,10 +205,10 @@ func initIntegrationTest() func() error { | |||
| if err != nil { | |||
| log.Fatalf("NewStorageControlClient: %v", err) | |||
| } | |||
| if err := client.Bucket(bucketName).Create(ctx, testutil.ProjID(), nil); err != nil { | |||
| if err := client.Bucket(bucketName).Create(ctx, testutil.ProjID(), &BucketAttrs{SoftDeletePolicy: &SoftDeletePolicy{RetentionDuration: 0}}); err != nil { | |||
There was a problem hiding this comment.
I think this should maybe be a separate change in our test harness? Was it impossible to delete the bucket if this wasn't included?
There was a problem hiding this comment.
Oops, this is leftover. My org has a policy set that made this necessary for me to be able to run the tests. I'll remove this before merge.
storage/integration_test.go
Outdated
| @@ -5041,7 +5042,58 @@ func TestIntegration_ReaderAttrs(t *testing.T) { | |||
| Metageneration: attrs.Metageneration, | |||
| CRC32C: crc32c(c), | |||
| } | |||
| if got != want { | |||
| if !reflect.DeepEqual(got, want) { | |||
There was a problem hiding this comment.
Maybe we can use https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff as elsewhere in these tests?
storage/integration_test.go
Outdated
| if err := writeObject(ctx, o, defaultType, c); err != nil { | ||
| t.Errorf("Write for %v failed with %v", o.ObjectName(), err) | ||
| } | ||
| defer func() { |
There was a problem hiding this comment.
Use t.Cleanup to ensure this is run.
storage/integration_test.go
Outdated
| } | ||
| }() | ||
|
|
||
| oa, err := o.Update(ctx, ObjectAttrsToUpdate{Metadata: map[string]string{"Custom-Key": "custom-value"}}) |
There was a problem hiding this comment.
Again, let's use at least 2 keys here.
storage/integration_test.go
Outdated
| Metageneration: attrs.Metageneration, | ||
| CRC32C: crc32c(c), | ||
| } | ||
| if !reflect.DeepEqual(got, want) { |
There was a problem hiding this comment.
Use cmp.Diff and let's only check the Metadata field in this test.
storage/reader.go
Outdated
| @@ -59,6 +59,10 @@ type ReaderObjectAttrs struct { | |||
| // Generation is the generation number of the object's content. | |||
| Generation int64 | |||
|
|
|||
| // Metadata represents user-provided metadata, in key/value pairs. | |||
| // Not supported by the JSON api. Use ObjectHandle.Attrs instead. | |||
There was a problem hiding this comment.
I would rephrase this as follows:
// Metadata represents user-provided metadata, in key/value pairs.
//
// It can be nil if no metadata is present, or if the client uses the JSON
// API for downloads. Only the XML and gRPC APIs support getting
// custom metadata via the Reader; for JSON make a separate call to
// ObjectHandle.Attrs.
storage/retry_conformance_test.go
Outdated
| @@ -586,6 +587,9 @@ func TestRetryConformance(t *testing.T) { | |||
| if host == "" { | |||
| t.Skip("This test must use the testbench emulator; set STORAGE_EMULATOR_HOST to run.") | |||
| } | |||
| if runtime.GOOS == "darwin" { | |||
There was a problem hiding this comment.
Let's remove this from the PR; seems non-relevant to this particular issue.
There was a problem hiding this comment.
👍 there was handling for this in the test script so I moved it in here, good to remove though.
implements #11211
This isn't in a mergeable state quite yet, but I wanted to start the discussion. This is only supported by the XML and gRPC apis. I'm unsure of how to handle the JSON api. Should I just document that the JSON api does not support metadata on read? Or is there a more elegant way to block the user from trying to read it and being surprised by it being empty?