From c27d3b2a967798a5756f044ae33be799b3546408 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Thu, 16 Jun 2022 13:23:56 -0700 Subject: [PATCH 1/8] fix: allow to use age=0 in OLM conditions --- storage/bucket.go | 21 +++++++++++++++++++++ storage/bucket_test.go | 16 ++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/storage/bucket.go b/storage/bucket.go index 87789f3bc29..510d3d77b99 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -610,7 +610,12 @@ const ( // // All configured conditions must be met for the associated action to be taken. type LifecycleCondition struct { + // AllObject is used to select all objects in a bucket by + // setting Age=0. + AllObjects bool + // AgeInDays is the age of the object in days. + // If you want to set AgeInDays to `0` use AllObjects set to true AgeInDays int64 // CreatedBefore is the time the object was created. @@ -628,10 +633,12 @@ type LifecycleCondition struct { // DaysSinceCustomTime is the days elapsed since the CustomTime date of the // object. This condition can only be satisfied if CustomTime has been set. + // Note: Using `0` as the value will not be ignored by the library and not sent to the API. DaysSinceCustomTime int64 // DaysSinceNoncurrentTime is the days elapsed since the noncurrent timestamp // of the object. This condition is relevant only for versioned objects. + // Note: Using `0` as the value will not be ignored by the library and not sent to the API. DaysSinceNoncurrentTime int64 // Liveness specifies the object's liveness. Relevant only for versioned objects @@ -1454,6 +1461,10 @@ func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle { NumNewerVersions: r.Condition.NumNewerVersions, }, } + if r.Condition.AllObjects { + rr.Condition.Age = 0 + rr.Condition.ForceSendFields = []string{"Age"} + } setAgeCondition(r.Condition.AgeInDays, &rr.Condition.Age) @@ -1504,6 +1515,12 @@ func toProtoLifecycle(l Lifecycle) *storagepb.Bucket_Lifecycle { }, } + // TODO: I don't think this is necessary because proto will automatically pass in values present. + // I wonder if all values will be send though by accident. + if r.Condition.AllObjects { + rr.Condition.AgeDays = proto.Int32(0) + } + switch r.Condition.Liveness { case LiveAndArchived: rr.Condition.IsLive = nil @@ -1564,6 +1581,10 @@ func toLifecycle(rl *raw.BucketLifecycle) Lifecycle { } r.Condition.AgeInDays = getAgeCondition(rr.Condition.Age) + if rr.Condition.Age == 0 { + r.Condition.AllObjects = true + } + if rr.Condition.IsLive == nil { r.Condition.Liveness = LiveAndArchived } else if *rr.Condition.IsLive { diff --git a/storage/bucket_test.go b/storage/bucket_test.go index c6b8710fc82..1b31284b651 100644 --- a/storage/bucket_test.go +++ b/storage/bucket_test.go @@ -119,6 +119,13 @@ func TestBucketAttrsToRawBucket(t *testing.T) { Condition: LifecycleCondition{ AgeInDays: 20, }, + }, { + Action: LifecycleAction{ + Type: DeleteAction, + }, + Condition: LifecycleCondition{ + AllObjects: true, + }, }}, }, } @@ -221,6 +228,15 @@ func TestBucketAttrsToRawBucket(t *testing.T) { //Age: 20, }, }, + { + Action: &raw.BucketLifecycleRuleAction{ + Type: DeleteAction, + }, + Condition: &raw.BucketLifecycleRuleCondition{ + Age: 0, + ForceSendFields: []string{"Age"}, + }, + }, }, }, } From bca3ea160f5341caf568390d9533ba2ac70e774f Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Thu, 16 Jun 2022 14:25:10 -0700 Subject: [PATCH 2/8] address feedback --- storage/bucket.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/storage/bucket.go b/storage/bucket.go index 510d3d77b99..263da5642bd 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -611,11 +611,11 @@ const ( // All configured conditions must be met for the associated action to be taken. type LifecycleCondition struct { // AllObject is used to select all objects in a bucket by - // setting Age=0. + // setting AgeInDays to 0. AllObjects bool // AgeInDays is the age of the object in days. - // If you want to set AgeInDays to `0` use AllObjects set to true + // If you want to set AgeInDays to `0` use AllObjects set to `true`. AgeInDays int64 // CreatedBefore is the time the object was created. @@ -1515,8 +1515,7 @@ func toProtoLifecycle(l Lifecycle) *storagepb.Bucket_Lifecycle { }, } - // TODO: I don't think this is necessary because proto will automatically pass in values present. - // I wonder if all values will be send though by accident. + // TODO(#6205): This may not be needed for gRPC if r.Condition.AllObjects { rr.Condition.AgeDays = proto.Int32(0) } @@ -1629,6 +1628,11 @@ func toLifecycleFromProto(rl *storagepb.Bucket_Lifecycle) Lifecycle { }, } + // TODO(#6205): This may not be needed for gRPC + if rr.GetCondition().GetAgeDays() == 0 { + r.Condition.AllObjects = true + } + if rr.GetCondition().IsLive == nil { r.Condition.Liveness = LiveAndArchived } else if rr.GetCondition().GetIsLive() { From d36b7418f546f131a34c50540512ce2ad7f17a32 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Fri, 17 Jun 2022 13:41:37 -0700 Subject: [PATCH 3/8] address feedback on OLM condition documentation --- storage/bucket.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/storage/bucket.go b/storage/bucket.go index 263da5642bd..abc0d882eee 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -633,12 +633,12 @@ type LifecycleCondition struct { // DaysSinceCustomTime is the days elapsed since the CustomTime date of the // object. This condition can only be satisfied if CustomTime has been set. - // Note: Using `0` as the value will not be ignored by the library and not sent to the API. + // Note: Using `0` as the value will be ignored by the library and not sent to the API. DaysSinceCustomTime int64 // DaysSinceNoncurrentTime is the days elapsed since the noncurrent timestamp // of the object. This condition is relevant only for versioned objects. - // Note: Using `0` as the value will not be ignored by the library and not sent to the API. + // Note: Using `0` as the value will be ignored by the library and not sent to the API. DaysSinceNoncurrentTime int64 // Liveness specifies the object's liveness. Relevant only for versioned objects @@ -670,6 +670,7 @@ type LifecycleCondition struct { // If the value is N, this condition is satisfied when there are at least N // versions (including the live version) newer than this version of the // object. + // Note: Using `0` as the value will be ignored by the library and not sent to the API. NumNewerVersions int64 } From e915f3058b41a24d748f755deac80401807c3ab3 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Wed, 24 Aug 2022 15:47:40 -0700 Subject: [PATCH 4/8] fix: remove workaround and update tests --- storage/bucket.go | 46 ++++++++++-------------------------------- storage/bucket_test.go | 41 +++++++++---------------------------- 2 files changed, 21 insertions(+), 66 deletions(-) diff --git a/storage/bucket.go b/storage/bucket.go index abc0d882eee..5f08f982dc8 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -1429,19 +1429,6 @@ func toCORSFromProto(rc []*storagepb.Bucket_Cors) []CORS { return out } -// Used to handle breaking change in Autogen Storage client OLM Age field -// from int64 to *int64 gracefully in the manual client -// TODO(#6240): Method should be removed once breaking change is made and introduced to this client -func setAgeCondition(age int64, ageField interface{}) { - c := reflect.ValueOf(ageField).Elem() - switch c.Kind() { - case reflect.Int64: - c.SetInt(age) - case reflect.Ptr: - c.Set(reflect.ValueOf(&age)) - } -} - func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle { var rl raw.BucketLifecycle if len(l.Rules) == 0 { @@ -1462,13 +1449,16 @@ func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle { NumNewerVersions: r.Condition.NumNewerVersions, }, } + + if r.Condition.AgeInDays > 0 { + rr.Condition.Age = googleapi.Int64(r.Condition.AgeInDays) + } + if r.Condition.AllObjects { - rr.Condition.Age = 0 + rr.Condition.Age = googleapi.Int64(0) rr.Condition.ForceSendFields = []string{"Age"} } - setAgeCondition(r.Condition.AgeInDays, &rr.Condition.Age) - switch r.Condition.Liveness { case LiveAndArchived: rr.Condition.IsLive = nil @@ -1544,21 +1534,6 @@ func toProtoLifecycle(l Lifecycle) *storagepb.Bucket_Lifecycle { return &rl } -// Used to handle breaking change in Autogen Storage client OLM Age field -// from int64 to *int64 gracefully in the manual client -// TODO(#6240): Method should be removed once breaking change is made and introduced to this client -func getAgeCondition(ageField interface{}) int64 { - v := reflect.ValueOf(ageField) - if v.Kind() == reflect.Int64 { - return v.Interface().(int64) - } else if v.Kind() == reflect.Ptr { - if val, ok := v.Interface().(*int64); ok { - return *val - } - } - return 0 -} - func toLifecycle(rl *raw.BucketLifecycle) Lifecycle { var l Lifecycle if rl == nil { @@ -1579,10 +1554,11 @@ func toLifecycle(rl *raw.BucketLifecycle) Lifecycle { NumNewerVersions: rr.Condition.NumNewerVersions, }, } - r.Condition.AgeInDays = getAgeCondition(rr.Condition.Age) - - if rr.Condition.Age == 0 { - r.Condition.AllObjects = true + if rr.Condition.Age != nil { + r.Condition.AgeInDays = *rr.Condition.Age + if rr.Condition.Age == googleapi.Int64(0) { + r.Condition.AllObjects = true + } } if rr.Condition.IsLive == nil { diff --git a/storage/bucket_test.go b/storage/bucket_test.go index 1b31284b651..22d20455c5e 100644 --- a/storage/bucket_test.go +++ b/storage/bucket_test.go @@ -25,7 +25,6 @@ import ( raw "google.golang.org/api/storage/v1" ) -// TODO(#6539): re-enable tests after breaking change is released and AgeInDays is a int64* func TestBucketAttrsToRawBucket(t *testing.T) { t.Skip("TestBucketAttrsToRawBucket skipped: https://github.com/googleapis/google-cloud-go/issues/6539") t.Parallel() @@ -170,7 +169,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) { StorageClass: "NEARLINE", }, Condition: &raw.BucketLifecycleRuleCondition{ - //Age: 10, + Age: googleapi.Int64(10), IsLive: googleapi.Bool(true), CreatedBefore: "2017-01-02", MatchesStorageClass: []string{"STANDARD"}, @@ -206,7 +205,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) { Type: DeleteAction, }, Condition: &raw.BucketLifecycleRuleCondition{ - //Age: 10, + Age: googleapi.Int64(10), MatchesPrefix: []string{"testPrefix"}, MatchesSuffix: []string{"testSuffix"}, NumNewerVersions: 3, @@ -217,6 +216,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) { Type: DeleteAction, }, Condition: &raw.BucketLifecycleRuleCondition{ + Age: googleapi.Int64(0), IsLive: googleapi.Bool(false), }, }, @@ -225,7 +225,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) { Type: AbortIncompleteMPUAction, }, Condition: &raw.BucketLifecycleRuleCondition{ - //Age: 20, + Age: googleapi.Int64(20), }, }, { @@ -233,7 +233,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) { Type: DeleteAction, }, Condition: &raw.BucketLifecycleRuleCondition{ - Age: 0, + Age: googleapi.Int64(0), ForceSendFields: []string{"Age"}, }, }, @@ -366,9 +366,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) { } } -// TODO(#6539): re-enable tests after breaking change is released and AgeInDays is a int64* func TestBucketAttrsToUpdateToRawBucket(t *testing.T) { - t.Skip("TestBucketAttrsToUpdateToRawBucket skipped: https://github.com/googleapis/google-cloud-go/issues/6539") t.Parallel() au := &BucketAttrsToUpdate{ VersioningEnabled: false, @@ -424,12 +422,12 @@ func TestBucketAttrsToUpdateToRawBucket(t *testing.T) { Lifecycle: &raw.BucketLifecycle{ Rule: []*raw.BucketLifecycleRule{ { - Action: &raw.BucketLifecycleRuleAction{Type: "Delete"}, - //Condition: &raw.BucketLifecycleRuleCondition{Age: 30}, + Action: &raw.BucketLifecycleRuleAction{Type: "Delete"}, + Condition: &raw.BucketLifecycleRuleCondition{Age: googleapi.Int64(30)}, }, { - Action: &raw.BucketLifecycleRuleAction{Type: AbortIncompleteMPUAction}, - //Condition: &raw.BucketLifecycleRuleCondition{Age: 13}, + Action: &raw.BucketLifecycleRuleAction{Type: AbortIncompleteMPUAction}, + Condition: &raw.BucketLifecycleRuleCondition{Age: googleapi.Int64(13)}, }, }, }, @@ -580,26 +578,7 @@ func TestBucketAttrsToUpdateToRawBucket(t *testing.T) { } } -func TestAgeConditionBackwardCompat(t *testing.T) { - var ti int64 - var want int64 = 100 - setAgeCondition(want, &ti) - if getAgeCondition(ti) != want { - t.Fatalf("got %v, want %v", getAgeCondition(ti), want) - } - - var tp *int64 - want = 10 - setAgeCondition(want, &tp) - if getAgeCondition(tp) != want { - t.Fatalf("got %v, want %v", getAgeCondition(tp), want) - } - -} - -// TODO(#6539): re-enable tests after breaking change is released and AgeInDays is a int64* func TestNewBucket(t *testing.T) { - t.Skip("TestNewBucket skipped: https://github.com/googleapis/google-cloud-go/issues/6539") labels := map[string]string{"a": "b"} matchClasses := []string{"STANDARD"} aTime := time.Date(2017, 1, 2, 0, 0, 0, 0, time.UTC) @@ -621,7 +600,7 @@ func TestNewBucket(t *testing.T) { StorageClass: "NEARLINE", }, Condition: &raw.BucketLifecycleRuleCondition{ - // Age: 10, + Age: googleapi.Int64(10), IsLive: googleapi.Bool(true), CreatedBefore: "2017-01-02", MatchesStorageClass: matchClasses, From 54f0253144290539c358eb052dde0703aaefde08 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Wed, 24 Aug 2022 15:54:12 -0700 Subject: [PATCH 5/8] unskip one more test path --- storage/bucket_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/storage/bucket_test.go b/storage/bucket_test.go index 22d20455c5e..1a343d5c97b 100644 --- a/storage/bucket_test.go +++ b/storage/bucket_test.go @@ -26,7 +26,6 @@ import ( ) func TestBucketAttrsToRawBucket(t *testing.T) { - t.Skip("TestBucketAttrsToRawBucket skipped: https://github.com/googleapis/google-cloud-go/issues/6539") t.Parallel() attrs := &BucketAttrs{ Name: "name", @@ -216,7 +215,6 @@ func TestBucketAttrsToRawBucket(t *testing.T) { Type: DeleteAction, }, Condition: &raw.BucketLifecycleRuleCondition{ - Age: googleapi.Int64(0), IsLive: googleapi.Bool(false), }, }, From e0b9ef42f7db88df222316a51b51932177214383 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Thu, 25 Aug 2022 13:27:27 -0700 Subject: [PATCH 6/8] address feedback --- storage/bucket.go | 2 +- storage/integration_test.go | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/storage/bucket.go b/storage/bucket.go index 5f08f982dc8..79fefa4be46 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -1556,7 +1556,7 @@ func toLifecycle(rl *raw.BucketLifecycle) Lifecycle { } if rr.Condition.Age != nil { r.Condition.AgeInDays = *rr.Condition.Age - if rr.Condition.Age == googleapi.Int64(0) { + if *rr.Condition.Age == 0 { r.Condition.AllObjects = true } } diff --git a/storage/integration_test.go b/storage/integration_test.go index 924eea65edb..1d7d2d5d266 100644 --- a/storage/integration_test.go +++ b/storage/integration_test.go @@ -309,6 +309,13 @@ func TestIntegration_BucketCreateDelete(t *testing.T) { MatchesSuffix: []string{"testSuffix"}, NumNewerVersions: 3, }, + }, { + Action: LifecycleAction{ + Type: DeleteAction, + }, + Condition: LifecycleCondition{ + AllObjects: true, + }, }}, } @@ -442,6 +449,10 @@ func TestIntegration_BucketLifecycle(t *testing.T) { Action: LifecycleAction{Type: AbortIncompleteMPUAction}, Condition: LifecycleCondition{AgeInDays: 30}, }, + { + Action: LifecycleAction{Type: DeleteAction}, + Condition: LifecycleCondition{AllObjects: true}, + }, }, } From bbe558d99b3c916e074dbfd9f9650a2630049b5d Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Thu, 25 Aug 2022 13:59:26 -0700 Subject: [PATCH 7/8] factor in precedent --- storage/bucket.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/storage/bucket.go b/storage/bucket.go index 79fefa4be46..c4a6c852b73 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -1450,13 +1450,14 @@ func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle { }, } - if r.Condition.AgeInDays > 0 { - rr.Condition.Age = googleapi.Int64(r.Condition.AgeInDays) - } - + // AllObjects takes precedent when both AllObjects and AgeInDays are set + // Rationale: If you've opted into using AllObjects, it makes sense that you + // understand the implications of how this option works with AgeInDays. if r.Condition.AllObjects { rr.Condition.Age = googleapi.Int64(0) rr.Condition.ForceSendFields = []string{"Age"} + } else if r.Condition.AgeInDays > 0 { + rr.Condition.Age = googleapi.Int64(r.Condition.AgeInDays) } switch r.Condition.Liveness { From a938372fcfdd8c3c5c6de6b8fd5a2b06a8b990c9 Mon Sep 17 00:00:00 2001 From: Frank Natividad Date: Thu, 25 Aug 2022 14:06:13 -0700 Subject: [PATCH 8/8] s/AllObject/AllObjects --- storage/bucket.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/bucket.go b/storage/bucket.go index c4a6c852b73..5a1c11eba00 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -610,7 +610,7 @@ const ( // // All configured conditions must be met for the associated action to be taken. type LifecycleCondition struct { - // AllObject is used to select all objects in a bucket by + // AllObjects is used to select all objects in a bucket by // setting AgeInDays to 0. AllObjects bool