From ca03b0d050aab9b2423ac4d182c9d410a2721749 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 10 Jan 2023 11:00:18 +0000 Subject: [PATCH 1/8] adding component count attribute in objectAttrs and related unit test and integration test --- storage/integration_test.go | 7 +++++++ storage/storage.go | 23 +++++++++++++++++++++++ storage/storage_test.go | 4 ++++ 3 files changed, 34 insertions(+) diff --git a/storage/integration_test.go b/storage/integration_test.go index eb5b27557d20..587ff3a3bda2 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -1633,6 +1633,7 @@ func TestIntegration_Compose(t *testing.T) { t.Errorf("Write for %v failed with %v", obj, err) } compSrcs = append(compSrcs, obj) + wantContents = append(wantContents, c...) defer obj.Delete(ctx) } @@ -1662,6 +1663,9 @@ func TestIntegration_Compose(t *testing.T) { if _, err := c.Run(ctx); err != nil { t.Fatalf("ComposeFrom error: %v", err) } + if c.ComponentCount != int64(len(objects)) { + t.Fatalf("Component Count of dst Object: %v is incorrect", c.ComponentCount) + } checkCompose(compDst, "application/octet-stream") // It should also work if we do. @@ -1671,6 +1675,9 @@ func TestIntegration_Compose(t *testing.T) { if _, err := c.Run(ctx); err != nil { t.Fatalf("ComposeFrom error: %v", err) } + if c.ComponentCount != int64(len(objects)) { + t.Fatalf("Component Count of dst Object: %v is incorrect", c.ComponentCount) + } checkCompose(compDst, "text/json") }) } diff --git a/storage/storage.go b/storage/storage.go index c4a771593249..23393921273f 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1315,6 +1315,27 @@ type ObjectAttrs struct { // later value but not to an earlier one. For more information see // https://cloud.google.com/storage/docs/metadata#custom-time . CustomTime time.Time + + // As of 2015-06-03, the official GCS documentation for this + // property (https://goo.gl/GwD5Dq) says this: + // + // Newly uploaded objects have a component count of 1, and composing a + // sequence of objects creates an object whose component count is equal + // to the sum of component counts in the sequence. + // + // However, in Google-internal bug 21572928 it was clarified that this + // doesn't match the actual implementation, which can be documented as: + // + // Newly uploaded objects do not have a component count. Composing a + // sequence of objects creates an object whose component count is equal + // to the sum of the component counts of the objects in the sequence, + // where objects that do not have a component count are treated as having + // a component count of 1. + // + // This is a much less elegant and convenient rule, so this package emulates + // the officially documented behavior above. That is, it synthesizes a + // component count of 1 for objects that do not have a component count. + ComponentCount int64 } // convertTime converts a time in RFC3339 format to time.Time. @@ -1385,6 +1406,7 @@ func newObject(o *raw.Object) *ObjectAttrs { Updated: convertTime(o.Updated), Etag: o.Etag, CustomTime: convertTime(o.CustomTime), + ComponentCount: o.ComponentCount, } } @@ -1419,6 +1441,7 @@ func newObjectFromProto(o *storagepb.Object) *ObjectAttrs { Deleted: convertProtoTime(o.GetDeleteTime()), Updated: convertProtoTime(o.GetUpdateTime()), CustomTime: convertProtoTime(o.GetCustomTime()), + ComponentCount: int64(o.ComponentCount), } } diff --git a/storage/storage_test.go b/storage/storage_test.go index 30cf209db50f..0414e0de0e7e 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -1747,6 +1747,7 @@ func TestRawObjectToObjectAttrs(t *testing.T) { TimeCreated: "2019-03-31T19:32:10Z", TimeDeleted: "2019-03-31T19:33:39Z", TemporaryHold: true, + ComponentCount: 2, }, want: &ObjectAttrs{ Bucket: "Test", @@ -1763,6 +1764,7 @@ func TestRawObjectToObjectAttrs(t *testing.T) { RetentionExpirationTime: time.Date(2019, 3, 31, 19, 33, 36, 0, time.UTC), Size: 1 << 20, TemporaryHold: true, + ComponentCount: 2, }, }, } @@ -1833,6 +1835,7 @@ func TestProtoObjectToObjectAttrs(t *testing.T) { CreateTime: timestamppb.New(now), DeleteTime: timestamppb.New(now), TemporaryHold: true, + ComponentCount: 2, }, want: &ObjectAttrs{ Bucket: "Test", @@ -1848,6 +1851,7 @@ func TestProtoObjectToObjectAttrs(t *testing.T) { RetentionExpirationTime: now, Size: 1 << 20, TemporaryHold: true, + ComponentCount: 2, }, }, } From 7ec492f6fe1bdf85678dc3beffe981c4717922a7 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 10 Jan 2023 11:03:25 +0000 Subject: [PATCH 2/8] formating the file --- storage/integration_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index 587ff3a3bda2..eec794bcdf32 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -1633,7 +1633,6 @@ func TestIntegration_Compose(t *testing.T) { t.Errorf("Write for %v failed with %v", obj, err) } compSrcs = append(compSrcs, obj) - wantContents = append(wantContents, c...) defer obj.Delete(ctx) } From 6f20478751cf4419c00a0a08ee606092237cdde0 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 10 Jan 2023 11:04:16 +0000 Subject: [PATCH 3/8] formating the file --- storage/integration_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index eec794bcdf32..ebec34e5a0e6 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -1663,7 +1663,7 @@ func TestIntegration_Compose(t *testing.T) { t.Fatalf("ComposeFrom error: %v", err) } if c.ComponentCount != int64(len(objects)) { - t.Fatalf("Component Count of dst Object: %v is incorrect", c.ComponentCount) + t.Fatalf("ComponentCount of dst Object: %v is incorrect", c.ComponentCount) } checkCompose(compDst, "application/octet-stream") @@ -1675,7 +1675,7 @@ func TestIntegration_Compose(t *testing.T) { t.Fatalf("ComposeFrom error: %v", err) } if c.ComponentCount != int64(len(objects)) { - t.Fatalf("Component Count of dst Object: %v is incorrect", c.ComponentCount) + t.Fatalf("ComponentCount of dst Object: %v is incorrect", c.ComponentCount) } checkCompose(compDst, "text/json") }) From 670efdf02d22cde08aa97f76c24d3fedc38e2f2f Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 10 Jan 2023 16:03:06 +0000 Subject: [PATCH 4/8] fixing integration test for compose method --- storage/integration_test.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index ebec34e5a0e6..df459d110492 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -1657,12 +1657,14 @@ func TestIntegration_Compose(t *testing.T) { } // Compose should work even if the user sets no destination attributes. + compDst := b.Object("composed1") c := compDst.ComposerFrom(compSrcs...) - if _, err := c.Run(ctx); err != nil { + attrs, err := c.Run(ctx) + if err != nil { t.Fatalf("ComposeFrom error: %v", err) } - if c.ComponentCount != int64(len(objects)) { + if attrs.ComponentCount != int64(len(objects)) { t.Fatalf("ComponentCount of dst Object: %v is incorrect", c.ComponentCount) } checkCompose(compDst, "application/octet-stream") @@ -1671,10 +1673,11 @@ func TestIntegration_Compose(t *testing.T) { compDst = b.Object("composed2") c = compDst.ComposerFrom(compSrcs...) c.ContentType = "text/json" - if _, err := c.Run(ctx); err != nil { + attrs, err = c.Run(ctx) + if err != nil { t.Fatalf("ComposeFrom error: %v", err) } - if c.ComponentCount != int64(len(objects)) { + if attrs.ComponentCount != int64(len(objects)) { t.Fatalf("ComponentCount of dst Object: %v is incorrect", c.ComponentCount) } checkCompose(compDst, "text/json") From f676964f3d36542665d8808a430d51937bf626a0 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 10 Jan 2023 16:20:20 +0000 Subject: [PATCH 5/8] fixing comments - rephrasing component count descriotion, adding got and want in integration test --- storage/integration_test.go | 4 ++-- storage/storage.go | 22 +++------------------- 2 files changed, 5 insertions(+), 21 deletions(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index df459d110492..fa50ae216699 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -1665,7 +1665,7 @@ func TestIntegration_Compose(t *testing.T) { t.Fatalf("ComposeFrom error: %v", err) } if attrs.ComponentCount != int64(len(objects)) { - t.Fatalf("ComponentCount of dst Object: %v is incorrect", c.ComponentCount) + t.Errorf("mismatching ComponentCount: got %v, want %v", attrs.ComponentCount, int64(len(objects))) } checkCompose(compDst, "application/octet-stream") @@ -1678,7 +1678,7 @@ func TestIntegration_Compose(t *testing.T) { t.Fatalf("ComposeFrom error: %v", err) } if attrs.ComponentCount != int64(len(objects)) { - t.Fatalf("ComponentCount of dst Object: %v is incorrect", c.ComponentCount) + t.Errorf("mismatching ComponentCount: got %v, want %v", attrs.ComponentCount, int64(len(objects))) } checkCompose(compDst, "text/json") }) diff --git a/storage/storage.go b/storage/storage.go index 23393921273f..26206b648ecd 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1316,25 +1316,9 @@ type ObjectAttrs struct { // https://cloud.google.com/storage/docs/metadata#custom-time . CustomTime time.Time - // As of 2015-06-03, the official GCS documentation for this - // property (https://goo.gl/GwD5Dq) says this: - // - // Newly uploaded objects have a component count of 1, and composing a - // sequence of objects creates an object whose component count is equal - // to the sum of component counts in the sequence. - // - // However, in Google-internal bug 21572928 it was clarified that this - // doesn't match the actual implementation, which can be documented as: - // - // Newly uploaded objects do not have a component count. Composing a - // sequence of objects creates an object whose component count is equal - // to the sum of the component counts of the objects in the sequence, - // where objects that do not have a component count are treated as having - // a component count of 1. - // - // This is a much less elegant and convenient rule, so this package emulates - // the officially documented behavior above. That is, it synthesizes a - // component count of 1 for objects that do not have a component count. + // ComponentCount is the number of objects contained within a composite object. + // For non-composite objects, the value will be zero. + // This field is read-only. ComponentCount int64 } From cdb91a76ec87bd60c791ccf3dbf16f69a11b76c3 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Tue, 10 Jan 2023 16:23:50 +0000 Subject: [PATCH 6/8] removing extra line --- storage/integration_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/storage/integration_test.go b/storage/integration_test.go index fa50ae216699..b851fecbcafb 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -1657,7 +1657,6 @@ func TestIntegration_Compose(t *testing.T) { } // Compose should work even if the user sets no destination attributes. - compDst := b.Object("composed1") c := compDst.ComposerFrom(compSrcs...) attrs, err := c.Run(ctx) From bd34f48ca804710c2bcf226cefd294670a3df3ae Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Wed, 11 Jan 2023 05:16:03 +0000 Subject: [PATCH 7/8] fixing presubmit issues --- storage/storage.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/storage/storage.go b/storage/storage.go index 26206b648ecd..e884a87157f9 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1554,6 +1554,7 @@ var attrToFieldMap = map[string]string{ "Updated": "updated", "Etag": "etag", "CustomTime": "customTime", + "ComponentCount": "componentCount", } // attrToProtoFieldMap maps the field names of ObjectAttrs to the underlying field @@ -1585,6 +1586,7 @@ var attrToProtoFieldMap = map[string]string{ "Owner": "owner", "CustomerKeySHA256": "customer_encryption", "CustomTime": "custom_time", + "ComponentCount": "componentCount", // MediaLink was explicitly excluded from the proto as it is an HTTP-ism. // "MediaLink": "mediaLink", } From 235ccb376c98a314c639e19782cbe39c0f5eecc2 Mon Sep 17 00:00:00 2001 From: Tulsi Shah Date: Wed, 11 Jan 2023 15:15:44 +0000 Subject: [PATCH 8/8] fixing comments- ComponentCount to component_count --- storage/storage.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/storage.go b/storage/storage.go index e884a87157f9..7fc3fc4cb9b3 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1586,7 +1586,7 @@ var attrToProtoFieldMap = map[string]string{ "Owner": "owner", "CustomerKeySHA256": "customer_encryption", "CustomTime": "custom_time", - "ComponentCount": "componentCount", + "ComponentCount": "component_count", // MediaLink was explicitly excluded from the proto as it is an HTTP-ism. // "MediaLink": "mediaLink", }