Refactor handling of setting file expiry policy (#21378)

* Refactor handling of setting file expiry policy Switch to using a struct containing explicit fields instead of an interface to improve discovery and self-documentation. * fix lint * reverse order of fields
Azure · Aug 16, 2023 · 40f7a7f · 40f7a7f
1 parent 8fd0022
commit 40f7a7f
Show file tree

Hide file tree

Showing 6 changed files with 116 additions and 159 deletions.
diff --git a/sdk/storage/azdatalake/file/client.go b/sdk/storage/azdatalake/file/client.go
@@ -10,6 +10,15 @@ import (
 	"bytes"
 	"context"
 	"errors"
+	"io"
+	"net/http"
+	"net/url"
+	"os"
+	"reflect"
+	"strings"
+	"sync"
+	"time"
+
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
@@ -25,13 +34,6 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/path"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/shared"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/sas"
-	"io"
-	"net/http"
-	"net/url"
-	"os"
-	"strings"
-	"sync"
-	"time"
 )
 
 // ClientOptions contains the optional parameters when creating a Client.
@@ -259,9 +261,15 @@ func (f *Client) Rename(ctx context.Context, newName string, options *RenameOpti
 }
 
 // SetExpiry operation sets an expiry time on an existing file (blob2).
-func (f *Client) SetExpiry(ctx context.Context, expiryType SetExpiryType, o *SetExpiryOptions) (SetExpiryResponse, error) {
-	expMode, opts := expiryType.Format(o)
-	resp, err := f.generatedFileClientWithBlob().SetExpiry(ctx, expMode, opts)
+func (f *Client) SetExpiry(ctx context.Context, expiryValues SetExpiryValues, o *SetExpiryOptions) (SetExpiryResponse, error) {
+	if reflect.ValueOf(expiryValues).IsZero() {
+		expiryValues.ExpiryType = SetExpiryTypeNeverExpire
+	}
+	opts := &generated_blob.BlobClientSetExpiryOptions{}
+	if expiryValues.ExpiryType != SetExpiryTypeNeverExpire {
+		opts.ExpiresOn = &expiryValues.ExpiresOn
+	}
+	resp, err := f.generatedFileClientWithBlob().SetExpiry(ctx, expiryValues.ExpiryType, opts)
 	err = exported.ConvertToDFSError(err)
 	return resp, err
 }

diff --git a/sdk/storage/azdatalake/file/client_test.go b/sdk/storage/azdatalake/file/client_test.go
@@ -11,6 +11,15 @@ import (
 	"context"
 	"crypto/md5"
 	"encoding/binary"
+	"hash/crc64"
+	"io"
+	"math/rand"
+	"net/http"
+	"os"
+	"strconv"
+	"testing"
+	"time"
+
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/streaming"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
 	"github.com/Azure/azure-sdk-for-go/sdk/internal/recording"
@@ -21,13 +30,6 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/sas"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
-	"hash/crc64"
-	"io"
-	"math/rand"
-	"net/http"
-	"os"
-	"testing"
-	"time"
 )
 
 var proposedLeaseIDs = []*string{to.Ptr("c820a799-76d7-4ee2-6e15-546f19325c2c"), to.Ptr("326cc5e1-746e-4af8-4811-a50e6629a8ca")}
@@ -472,9 +474,11 @@ func (s *UnrecordedTestSuite) TestCreateFileWithExpiryAbsolute() {
 	defer testcommon.DeleteFilesystem(context.Background(), _require, fsClient)
 
 	expiryTimeAbsolute := time.Now().Add(8 * time.Second)
-	expiry := file.CreationExpiryTypeAbsolute(expiryTimeAbsolute)
 	createFileOpts := &file.CreateOptions{
-		Expiry: expiry,
+		Expiry: file.CreateExpiryValues{
+			ExpiryType: file.CreateExpiryTypeAbsolute,
+			ExpiresOn:  time.Now().Add(8 * time.Second).UTC().Format(http.TimeFormat),
+		},
 	}
 
 	_, err = fsClient.Create(context.Background(), nil)
@@ -505,9 +509,11 @@ func (s *RecordedTestSuite) TestCreateFileWithExpiryRelativeToNow() {
 	_require.NoError(err)
 	defer testcommon.DeleteFilesystem(context.Background(), _require, fsClient)
 
-	expiry := file.CreationExpiryTypeRelativeToNow(8 * time.Second)
 	createFileOpts := &file.CreateOptions{
-		Expiry: expiry,
+		Expiry: file.CreateExpiryValues{
+			ExpiryType: file.CreateExpiryTypeRelativeToNow,
+			ExpiresOn:  strconv.FormatInt((8 * time.Second).Milliseconds(), 10),
+		},
 	}
 
 	_, err = fsClient.Create(context.Background(), nil)
@@ -540,7 +546,9 @@ func (s *RecordedTestSuite) TestCreateFileWithNeverExpire() {
 	defer testcommon.DeleteFilesystem(context.Background(), _require, fsClient)
 
 	createFileOpts := &file.CreateOptions{
-		Expiry: file.CreationExpiryTypeNever{},
+		Expiry: file.CreateExpiryValues{
+			ExpiryType: file.CreateExpiryTypeNeverExpire,
+		},
 	}
 
 	_, err = fsClient.Create(context.Background(), nil)
@@ -2961,6 +2969,7 @@ func (s *RecordedTestSuite) TestFileAppendAndFlushAndDownloadDataWithLeasedFile(
 	_require.Nil(err)
 
 	_, err = rsc.Seek(0, io.SeekStart)
+	_require.NoError(err)
 
 	_, err = srcFClient.AppendData(context.Background(), int64(contentSize), rsc, opts)
 	_require.Nil(err)

diff --git a/sdk/storage/azdatalake/file/constants.go b/sdk/storage/azdatalake/file/constants.go
@@ -9,6 +9,8 @@ package file
 import (
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/exported"
+	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/generated"
+	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/generated_blob"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/path"
 )
 
@@ -50,6 +52,40 @@ func TransferValidationTypeComputeCRC64() TransferValidationType {
 	return exported.TransferValidationTypeComputeCRC64()
 }
 
+// SetExpiryType defines the values for modes of file expiration.
+type SetExpiryType = generated_blob.ExpiryOptions
+
+const (
+	// SetExpiryTypeAbsolute sets the expiration date as an absolute value expressed in RFC1123 format.
+	SetExpiryTypeAbsolute SetExpiryType = generated_blob.ExpiryOptionsAbsolute
+
+	// SetExpiryTypeNeverExpire sets the file to never expire or removes the current expiration date.
+	SetExpiryTypeNeverExpire SetExpiryType = generated_blob.ExpiryOptionsNeverExpire
+
+	// SetExpiryTypeRelativeToCreation sets the expiration date relative to the time of file creation.
+	// The value is expressed as the number of miliseconds to elapse from the time of creation.
+	SetExpiryTypeRelativeToCreation SetExpiryType = generated_blob.ExpiryOptionsRelativeToCreation
+
+	// SetExpiryTypeRelativeToNow sets the expiration date relative to the current time.
+	// The value is expressed as the number of milliseconds to elapse from the present time.
+	SetExpiryTypeRelativeToNow SetExpiryType = generated_blob.ExpiryOptionsRelativeToNow
+)
+
+// CreateExpiryType defines the values for modes of file expiration specified during creation.
+type CreateExpiryType = generated.PathExpiryOptions
+
+const (
+	// CreateExpiryTypeAbsolute sets the expiration date as an absolute value expressed in RFC1123 format.
+	CreateExpiryTypeAbsolute CreateExpiryType = generated.PathExpiryOptionsAbsolute
+
+	// CreateExpiryTypeNeverExpire sets the file to never expire or removes the current expiration date.
+	CreateExpiryTypeNeverExpire CreateExpiryType = generated.PathExpiryOptionsNeverExpire
+
+	// CreateExpiryTypeRelativeToNow sets the expiration date relative to the current time.
+	// The value is expressed as the number of milliseconds to elapse from the present time.
+	CreateExpiryTypeRelativeToNow CreateExpiryType = generated.PathExpiryOptionsRelativeToNow
+)
+
 // StatusType defines values for StatusType
 type StatusType = azdatalake.StatusType
 

diff --git a/sdk/storage/azdatalake/file/models.go b/sdk/storage/azdatalake/file/models.go
@@ -8,16 +8,14 @@ package file
 
 import (
 	"errors"
-	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
+	"io"
+	"reflect"
+
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/blob"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/exported"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/generated"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/path"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/internal/shared"
-	"io"
-	"net/http"
-	"strconv"
-	"time"
 )
 
 const (
@@ -42,7 +40,7 @@ type CreateOptions struct {
 	// HTTPHeaders contains the HTTP headers for path operations.
 	HTTPHeaders *HTTPHeaders
 	// Expiry specifies the type and time of expiry for the file.
-	Expiry CreationExpiryType
+	Expiry CreateExpiryValues
 	// LeaseDuration specifies the duration of the lease, in seconds, or negative one
 	// (-1) for a lease that never expires. A non-infinite lease can be
 	// between 15 and 60 seconds.
@@ -61,6 +59,18 @@ type CreateOptions struct {
 	ACL *string
 }
 
+// CreateExpiryValues describes when a newly created file should expire.
+// A zero-value indicates the file has no expiration date.
+type CreateExpiryValues struct {
+	// ExpiryType indicates how the value of ExpiresOn should be interpreted (absolute, relative to now, etc).
+	ExpiryType CreateExpiryType
+
+	// ExpiresOn contains the time the file should expire.
+	// The value will either be an absolute UTC time in RFC1123 format or an integer expressing a number of milliseconds.
+	// NOTE: when ExpiryType is CreateExpiryTypeNeverExpire, this value is ignored.
+	ExpiresOn string
+}
+
 func (o *CreateOptions) format() (*generated.LeaseAccessConditions, *generated.ModifiedAccessConditions, *generated.PathHTTPHeaders, *generated.PathClientCreateOptions, *generated.CPKInfo) {
 	resource := generated.PathResourceTypeFile
 	createOpts := &generated.PathClientCreateOptions{
@@ -70,13 +80,11 @@ func (o *CreateOptions) format() (*generated.LeaseAccessConditions, *generated.M
 		return nil, nil, nil, createOpts, nil
 	}
 	leaseAccessConditions, modifiedAccessConditions := exported.FormatPathAccessConditions(o.AccessConditions)
-	if o.Expiry == nil {
-		createOpts.ExpiryOptions = nil
-		createOpts.ExpiresOn = nil
-	} else {
-		expOpts, expiresOn := o.Expiry.Format()
-		createOpts.ExpiryOptions = (*generated.PathExpiryOptions)(&expOpts)
-		createOpts.ExpiresOn = expiresOn
+	if !reflect.ValueOf(o.Expiry).IsZero() {
+		createOpts.ExpiryOptions = &o.Expiry.ExpiryType
+		if o.Expiry.ExpiryType != CreateExpiryTypeNeverExpire {
+			createOpts.ExpiresOn = &o.Expiry.ExpiresOn
+		}
 	}
 	createOpts.ACL = o.ACL
 	createOpts.Group = o.Group
@@ -491,65 +499,28 @@ func (o *DownloadFileOptions) format() *blob.DownloadFileOptions {
 	return downloadFileOptions
 }
 
-// CreationExpiryType defines values for Create() ExpiryType
-type CreationExpiryType interface {
-	Format() (generated.ExpiryOptions, *string)
-	notPubliclyImplementable()
-}
-
-// CreationExpiryTypeAbsolute defines the absolute time for the blob expiry
-type CreationExpiryTypeAbsolute time.Time
-
-// CreationExpiryTypeRelativeToNow defines the duration relative to now for the blob expiry
-type CreationExpiryTypeRelativeToNow time.Duration
-
-// CreationExpiryTypeNever defines that the blob will be set to never expire
-type CreationExpiryTypeNever struct {
-	// empty struct since NeverExpire expiry type does not require expiry time
-}
-
-func (e CreationExpiryTypeAbsolute) Format() (generated.ExpiryOptions, *string) {
-	return generated.ExpiryOptionsAbsolute, to.Ptr(time.Time(e).UTC().Format(http.TimeFormat))
+// SetExpiryValues describes when a file should expire.
+// A zero-value indicates the file has no expiration date.
+type SetExpiryValues struct {
+	// ExpiryType indicates how the value of ExpiresOn should be interpreted (absolute, relative to now, etc).
+	ExpiryType SetExpiryType
 
+	// ExpiresOn contains the time the file should expire.
+	// The value will either be an absolute UTC time in RFC1123 format or an integer expressing a number of milliseconds.
+	// NOTE: when ExpiryType is SetExpiryTypeNeverExpire, this value is ignored.
+	ExpiresOn string
 }
 
-func (e CreationExpiryTypeAbsolute) notPubliclyImplementable() {}
-
-func (e CreationExpiryTypeRelativeToNow) Format() (generated.ExpiryOptions, *string) {
-	return generated.ExpiryOptionsRelativeToNow, to.Ptr(strconv.FormatInt(time.Duration(e).Milliseconds(), 10))
-}
-
-func (e CreationExpiryTypeRelativeToNow) notPubliclyImplementable() {}
-
-func (e CreationExpiryTypeNever) Format() (generated.ExpiryOptions, *string) {
-	return generated.ExpiryOptionsNeverExpire, nil
-}
-
-func (e CreationExpiryTypeNever) notPubliclyImplementable() {}
-
 // ACLFailedEntry contains the failed ACL entry (response model).
 type ACLFailedEntry = path.ACLFailedEntry
 
 // SetAccessControlRecursiveResponse contains part of the response data returned by the []OP_AccessControl operations.
 type SetAccessControlRecursiveResponse = generated.SetAccessControlRecursiveResponse
 
-// SetExpiryType defines values for ExpiryType.
-type SetExpiryType = exported.SetExpiryType
-
-// SetExpiryTypeAbsolute defines the absolute time for the expiry.
-type SetExpiryTypeAbsolute = exported.SetExpiryTypeAbsolute
-
-// SetExpiryTypeRelativeToNow defines the duration relative to now for the expiry.
-type SetExpiryTypeRelativeToNow = exported.SetExpiryTypeRelativeToNow
-
-// SetExpiryTypeRelativeToCreation defines the duration relative to creation for the expiry.
-type SetExpiryTypeRelativeToCreation = exported.SetExpiryTypeRelativeToCreation
-
-// SetExpiryTypeNever defines that will be set to never expire.
-type SetExpiryTypeNever = exported.SetExpiryTypeNever
-
 // SetExpiryOptions contains the optional parameters for the Client.SetExpiry method.
-type SetExpiryOptions = exported.SetExpiryOptions
+type SetExpiryOptions struct {
+	// placeholder for future options
+}
 
 type HTTPRange = exported.HTTPRange
 

diff --git a/sdk/storage/azdatalake/filesystem/client_test.go b/sdk/storage/azdatalake/filesystem/client_test.go
@@ -8,6 +8,11 @@ package filesystem_test
 
 import (
 	"context"
+	"strconv"
+	"strings"
+	"testing"
+	"time"
+
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/to"
 	"github.com/Azure/azure-sdk-for-go/sdk/internal/recording"
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/datalakeerror"
@@ -17,10 +22,6 @@ import (
 	"github.com/Azure/azure-sdk-for-go/sdk/storage/azdatalake/sas"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
-	"strconv"
-	"strings"
-	"testing"
-	"time"
 )
 
 func Test(t *testing.T) {
@@ -100,6 +101,7 @@ func (s *RecordedTestSuite) TestCreateFilesystemWithOptions() {
 	_require.Nil(err)
 
 	props, err := fsClient.GetProperties(context.Background(), nil)
+	_require.NoError(err)
 	_require.NotNil(props.Metadata)
 	_require.Equal(*props.PublicAccess, filesystem.Filesystem)
 }
@@ -123,6 +125,7 @@ func (s *RecordedTestSuite) TestCreateFilesystemWithFileAccess() {
 	_, err = fsClient.Create(context.Background(), &opts)
 	_require.Nil(err)
 	props, err := fsClient.GetProperties(context.Background(), nil)
+	_require.NoError(err)
 	_require.NotNil(props.Metadata)
 	_require.Equal(*props.PublicAccess, filesystem.File)
 }
@@ -146,6 +149,7 @@ func (s *RecordedTestSuite) TestCreateFilesystemEmptyMetadata() {
 	_require.Nil(err)
 
 	props, err := fsClient.GetProperties(context.Background(), nil)
+	_require.NoError(err)
 	_require.Nil(props.Metadata)
 	_require.Equal(*props.PublicAccess, filesystem.Filesystem)