Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(storage): allow for Age int64* type and int64 type #6230

Merged
merged 10 commits into from
Jun 22, 2022
28 changes: 27 additions & 1 deletion storage/bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -1503,6 +1503,17 @@ func toCORSFromProto(rc []*storagepb.Bucket_Cors) []CORS {
return out
}

// Handle ptr or int64
func setAgeCondition(age int64, cond interface{}) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a note about why this is needed and a TODO/issue to remove in a future release.

c := reflect.ValueOf(cond).Elem().FieldByName("Age")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would limit the amount of reflection needed by just passing in a pointer to the field directly -- do a recursive call if needed to go one more level in.

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 {
Expand All @@ -1525,6 +1536,8 @@ func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle {
},
}

setAgeCondition(r.Condition.AgeInDays, rr.Condition)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setAgeCondition(r.Condition.AgeInDays, rr.Condition)
setAgeCondition(r.Condition.AgeInDays, &rr.Condition)


switch r.Condition.Liveness {
case LiveAndArchived:
rr.Condition.IsLive = nil
Expand Down Expand Up @@ -1595,6 +1608,19 @@ func toProtoLifecycle(l Lifecycle) *storagepb.Bucket_Lifecycle {
return &rl
}

// Handle ptr or int64
func getAgeCondition(cond interface{}) int64 {
v := reflect.ValueOf(cond).Elem().FieldByName("Age")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this one, I would just pass in the field directly.

if v.Kind() == reflect.Int64 {
return v.Interface().(int64)
} else if v.Kind() == reflect.Pointer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointer got added in a recent Go release; use reflect.Ptr here instead. (This is why the CI fails.)

if v.Interface() != nil {
return *(v.Interface().(*int64))
}
}
return 0
}

func toLifecycle(rl *raw.BucketLifecycle) Lifecycle {
var l Lifecycle
if rl == nil {
Expand All @@ -1607,7 +1633,6 @@ func toLifecycle(rl *raw.BucketLifecycle) Lifecycle {
StorageClass: rr.Action.StorageClass,
},
Condition: LifecycleCondition{
AgeInDays: rr.Condition.Age,
DaysSinceCustomTime: rr.Condition.DaysSinceCustomTime,
DaysSinceNoncurrentTime: rr.Condition.DaysSinceNoncurrentTime,
MatchesPrefix: rr.Condition.MatchesPrefix,
Expand All @@ -1616,6 +1641,7 @@ func toLifecycle(rl *raw.BucketLifecycle) Lifecycle {
NumNewerVersions: rr.Condition.NumNewerVersions,
},
}
r.Condition.AgeInDays = getAgeCondition(rr.Condition)

if rr.Condition.IsLive == nil {
r.Condition.Liveness = LiveAndArchived
Expand Down
24 changes: 24 additions & 0 deletions storage/bucket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,30 @@ func TestBucketAttrsToUpdateToRawBucket(t *testing.T) {
}
}

func TestAgeConditionBackwardCompat(t *testing.T) {
type testPointer struct {
Age *int64
}

type testInt64 struct {
Age int64
}

var tp testPointer
var want int64 = 10
setAgeCondition(want, &tp)
if getAgeCondition(&tp) != want {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if getAgeCondition(&tp) != want {
if getAgeCondition(tp) != want {

t.Fatalf("got %v, want 10", *tp.Age)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
t.Fatalf("got %v, want 10", *tp.Age)
t.Fatalf("got %v, want %v", *tp.Age, want)

}

var ti testInt64
want = 100
setAgeCondition(100, &ti)
if getAgeCondition(&ti) != want {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if getAgeCondition(&ti) != want {
if getAgeCondition(ti) != want {

t.Fatalf("got %v, want %v", ti.Age, want)
}
}

func TestCallBuilders(t *testing.T) {
rc, err := raw.NewService(context.Background())
if err != nil {
Expand Down