feat(storage): return file metadata on read by jshore1296 · Pull Request #11212 · googleapis/google-cloud-go

jshore1296 · 2024-12-03T17:48:36Z

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?

tritone · 2024-12-04T23:10:35Z

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?

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 · 2024-12-05T19:28:07Z

Alright, this is ready for review.

jshore1296 · 2024-12-10T14:38:44Z

@tritone sending a ping in case the previous comment didn't notify you

tritone

A few comments; overall looks good. Thanks for adding tests!

tritone · 2024-12-10T16:09:00Z

 }

+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) {


I believe the HTTP client should use XML by default, does the test not pass with a default HTTP client?

right you are.

tritone · 2024-12-10T16:09:57Z

+			t.Fatalf("closing object: %v", err)
+		}
+		if _, err := veneerClient.Bucket(bucket).Object(want.Name).Update(ctx, ObjectAttrsToUpdate{
+			Metadata: map[string]string{


Let's do at least 2 keys in here just to make sure the decoding logic works correctly.

tritone · 2024-12-10T16:11:06Z

 			CacheControl:    obj.GetCacheControl(),
 			LastModified:    obj.GetUpdateTime().AsTime(),
 			Metageneration:  obj.GetMetageneration(),
+			Metadata:        obj.GetMetadata(),


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.

Yeah, I was surprised that was already in there!

tritone · 2024-12-10T16:12:16Z

 			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 {


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?

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.

tritone · 2024-12-10T16:14:35Z

 			CRC32C:          crc32c(c),
 		}
-		if got != want {
+		if !reflect.DeepEqual(got, want) {


Maybe we can use https://pkg.go.dev/github.com/google/go-cmp/cmp#Diff as elsewhere in these tests?

tritone · 2024-12-10T16:15:50Z

+		if err := writeObject(ctx, o, defaultType, c); err != nil {
+			t.Errorf("Write for %v failed with %v", o.ObjectName(), err)
+		}
+		defer func() {


Use t.Cleanup to ensure this is run.

tritone · 2024-12-10T16:16:07Z

+			}
+		}()
+
+		oa, err := o.Update(ctx, ObjectAttrsToUpdate{Metadata: map[string]string{"Custom-Key": "custom-value"}})


Again, let's use at least 2 keys here.

tritone · 2024-12-10T16:17:11Z

+			Metageneration:  attrs.Metageneration,
+			CRC32C:          crc32c(c),
+		}
+		if !reflect.DeepEqual(got, want) {


Use cmp.Diff and let's only check the Metadata field in this test.

tritone · 2024-12-10T16:24:42Z

 	Generation int64

+	// Metadata represents user-provided metadata, in key/value pairs.
+	// Not supported by the JSON api.  Use ObjectHandle.Attrs instead.


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.

tritone · 2024-12-10T16:26:45Z

 	if host == "" {
 		t.Skip("This test must use the testbench emulator; set STORAGE_EMULATOR_HOST to run.")
 	}
+	if runtime.GOOS == "darwin" {


Let's remove this from the PR; seems non-relevant to this particular issue.

👍 there was handling for this in the test script so I moved it in here, good to remove though.

tritone

One more minor nit; otherwise looks good. Thanks for your contribution!

tritone · 2024-12-13T22:16:13Z

+		expectedMetadata := map[string]string{
+			"Custom-Metadata-Key": "custom-metadata-value",
+		}
+		if !reflect.DeepEqual(rd.Attrs.Metadata, expectedMetadata) {


One more place to switch to cmp.Diff

Old habits die hard, haha.

tritone · 2024-12-19T02:11:16Z

Okay so looks like there are some presubmit issues:

Check for formatting; you can fix this with gofmt.
Technically this is a breaking change to Reader because, with an exported map field, ReaderObjectAttrs becomes a non-comparable type.

(2) is kind of an annoying check (who is comparing storage.Reader using comparison operators?) but I think we should fix it. Perhaps we can make a GetMetadata() method on Reader rather than making it a field of ReaderObjectAttrs? Or maybe we could change the Metadata field to a pointer to a map; I think pointers are good to go.

Sorry for not flagging that sooner and LMK if that is sufficient clarity on what needs to change...

Adding a map field to Attrs would have made the object uncomparable

jshore1296 · 2024-12-19T16:39:05Z

makes sense @tritone . Pushed the updates, hopefully I caught all the formatting stuff.

tritone · 2024-12-19T16:57:56Z

 type Reader struct {
-	Attrs              ReaderObjectAttrs
+	Attrs          ReaderObjectAttrs
+	objectMetadata map[string]string


Looks like even a non-exported map here makes the comparable check unhappy.

Let's make this a pointer, and you can keep the method to return the map.

to preserve API compatibility - keeps the Reader struct comparable

jshore1296 requested review from a team December 3, 2024 17:48

product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Dec 3, 2024

storage: Return file Metadata on file read

b882d74

jshore1296 force-pushed the metadata-on-read branch from 462536d to b882d74 Compare December 5, 2024 19:21

tritone reviewed Dec 10, 2024

View reviewed changes

review comments

a0f8328

tritone approved these changes Dec 13, 2024

View reviewed changes

review comments

fe81f33

tritone changed the title ~~storage: return file metadata on read~~ feat(storage): return file metadata on read Dec 16, 2024

Merge branch 'main' into metadata-on-read

fbfb3bc

jshore1296 added 2 commits December 19, 2024 09:14

gofmt

5d770bf

Access metadata through function, rather than field

2d624fa

Adding a map field to Attrs would have made the object uncomparable

Merge branch 'main' into metadata-on-read

686325a

tritone added the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 19, 2024

kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Dec 19, 2024

tritone reviewed Dec 19, 2024

View reviewed changes

Store metadata as a pointer

f047e07

to preserve API compatibility - keeps the Reader struct comparable

tritone added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 19, 2024

kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 19, 2024

tritone merged commit d49263b into googleapis:main Dec 19, 2024

release-please Bot mentioned this pull request Dec 19, 2024

chore(main): release storage 1.49.0 #11273

Merged

jshore1296 deleted the metadata-on-read branch December 19, 2024 19:13

tritone mentioned this pull request Dec 20, 2024

storage: return file metadata on read #11211

Closed

release-please Bot mentioned this pull request Mar 12, 2025

chore(main): release storage 2.0.0 #11814

Closed

release-please Bot mentioned this pull request Mar 12, 2025

chore(main): release storage 1.52.0 #11815

Merged

Uh oh!

Conversation

jshore1296 commented Dec 3, 2024

Uh oh!

tritone commented Dec 4, 2024

Uh oh!

jshore1296 commented Dec 5, 2024

Uh oh!

jshore1296 commented Dec 10, 2024

Uh oh!

tritone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tritone left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tritone commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jshore1296 commented Dec 19, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tritone commented Dec 19, 2024 •

edited

Loading