Merge pull request #2647 from gofr-dev/fix-file

Author: Umang01-hash

docs/advanced-guide/handling-file/page.md

@@ -186,7 +186,7 @@ func main() {
 		ShareName:   "myshare",
 		// Endpoint is optional, defaults to https://{AccountName}.file.core.windows.net
 		// Endpoint: "https://custom-endpoint.file.core.windows.net",
-	}, app.Logger(), app.Metrics())
+	})
 
 	if err != nil {
 		app.Logger().Fatalf("Failed to initialize Azure File Storage: %v", err)

pkg/gofr/datasource/file/azure/fs.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"time"
 
-	"gofr.dev/pkg/gofr/datasource"
 	"gofr.dev/pkg/gofr/datasource/file"
 )
 
@@ -30,8 +29,9 @@ type Config struct {
 }
 
 // New creates and validates a new Azure File Storage file system.
-// Returns error if connection fails.
-func New(config *Config, logger datasource.Logger, metrics file.StorageMetrics) (file.FileSystemProvider, error) {
+// Returns error if configuration is invalid.
+// Connection will be established when Connect() is called.
+func New(config *Config) (file.FileSystemProvider, error) {
 	if config == nil {
 		return nil, errInvalidConfig
 	}
@@ -54,48 +54,42 @@ func New(config *Config, logger datasource.Logger, metrics file.StorageMetrics)
 		CommonFileSystem: &file.CommonFileSystem{
 			Provider:     adapter,
 			Location:     config.ShareName,
-			Logger:       logger,
-			Metrics:      metrics,
 			ProviderName: "Azure", // Set provider name for observability
 		},
 	}
 
+	return fs, nil
+}
+
+// Connect tries a single immediate connect via provider; on failure it starts a background retry.
+func (f *azureFileSystem) Connect() {
+	if f.CommonFileSystem.IsConnected() {
+		return
+	}
+
 	ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
 	defer cancel()
 
-	if logger != nil {
-		logger.Debugf("Attempting to connect to Azure File Share %s (timeout: %v)", config.ShareName, defaultTimeout)
+	if f.CommonFileSystem.Logger != nil {
+		f.CommonFileSystem.Logger.Debugf("Attempting to connect to Azure File Share %s (timeout: %v)",
+			f.CommonFileSystem.Location, defaultTimeout)
 	}
 
 	// Use CommonFileSystem.Connect for bookkeeping
-	if err := fs.CommonFileSystem.Connect(ctx); err != nil {
-		if logger != nil {
-			logger.Warnf("Azure File Share %s not available, starting background retry: %v", config.ShareName, err)
+	if err := f.CommonFileSystem.Connect(ctx); err != nil {
+		if f.CommonFileSystem.Logger != nil {
+			f.CommonFileSystem.Logger.Warnf("Azure File Share %s not available, starting background retry: %v", f.CommonFileSystem.Location, err)
 		}
 
-		go fs.startRetryConnect()
+		go f.startRetryConnect()
 
-		return fs, nil
+		return
 	}
 
 	// Connected successfully
-	if logger != nil {
-		logger.Debugf("Successfully connected to Azure File Share %s", config.ShareName)
-	}
-
-	return fs, nil
-}
-
-// Connect tries a single immediate connect via provider; on failure it starts a background retry.
-func (f *azureFileSystem) Connect() {
-	if f.CommonFileSystem.IsConnected() {
-		return
+	if f.CommonFileSystem.Logger != nil {
+		f.CommonFileSystem.Logger.Debugf("Successfully connected to Azure File Share %s", f.CommonFileSystem.Location)
 	}
-
-	ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
-	defer cancel()
-
-	_ = f.CommonFileSystem.Connect(ctx)
 }
 
 // startRetryConnect repeatedly calls provider.Connect until success.

pkg/gofr/datasource/file/azure/fs_test.go

@@ -15,7 +15,7 @@ import (
 var errConnectionFailed = errors.New("connection failed")
 
 func TestNew_NilConfig(t *testing.T) {
-	fs, err := New(nil, nil, nil)
+	fs, err := New(nil)
 
 	require.Error(t, err)
 	assert.Nil(t, fs)
@@ -25,7 +25,7 @@ func TestNew_NilConfig(t *testing.T) {
 func TestNew_EmptyShareName(t *testing.T) {
 	config := &Config{ShareName: ""}
 
-	fs, err := New(config, nil, nil)
+	fs, err := New(config)
 
 	require.Error(t, err)
 	assert.Nil(t, fs)
@@ -39,7 +39,7 @@ func TestNew_EmptyAccountName(t *testing.T) {
 		AccountKey:  "testkey",
 	}
 
-	fs, err := New(config, nil, nil)
+	fs, err := New(config)
 
 	require.Error(t, err)
 	assert.Nil(t, fs)
@@ -53,7 +53,7 @@ func TestNew_EmptyAccountKey(t *testing.T) {
 		AccountKey:  "",
 	}
 
-	fs, err := New(config, nil, nil)
+	fs, err := New(config)
 
 	require.Error(t, err)
 	assert.Nil(t, fs)
@@ -73,6 +73,14 @@ func TestNew_ConnectionFailure_StartsRetry(t *testing.T) {
 		ShareName:   "non-existent-share",
 	}
 
+	fs, err := New(config)
+	require.NoError(t, err)
+	require.NotNil(t, fs)
+
+	// Set logger and metrics via UseLogger/UseMetrics
+	fs.UseLogger(mockLogger)
+	fs.UseMetrics(mockMetrics)
+
 	// Expect debug log for connection attempt
 	mockLogger.EXPECT().Debugf(
 		gomock.Any(), // Format string
@@ -99,10 +107,8 @@ func TestNew_ConnectionFailure_StartsRetry(t *testing.T) {
 		gomock.Any(), // Share name
 	).MaxTimes(1)
 
-	fs, err := New(config, mockLogger, mockMetrics)
-
-	require.NoError(t, err)
-	require.NotNil(t, fs)
+	// Now call Connect which will attempt connection and start retry
+	fs.Connect()
 
 	time.Sleep(100 * time.Millisecond)
 
@@ -115,25 +121,36 @@ func TestAzureFileSystem_Connect_AlreadyConnected(t *testing.T) {
 
 	mockLogger := file.NewMockLogger(ctrl)
 	mockMetrics := file.NewMockMetrics(ctrl)
+	mockProvider := file.NewMockStorageProvider(ctrl)
 
 	config := &Config{
 		AccountName: "testaccount",
 		AccountKey:  "testkey",
 		ShareName:   "testshare",
 	}
 
-	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
-	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).AnyTimes()
+	fs, err := New(config)
+	require.NoError(t, err)
+
+	// Set logger and metrics via UseLogger/UseMetrics
+	fs.UseLogger(mockLogger)
+	fs.UseMetrics(mockMetrics)
+
+	// Replace provider with mock for successful connection
+	fs.(*azureFileSystem).CommonFileSystem.Provider = mockProvider
+
+	mockLogger.EXPECT().Debugf("Attempting to connect to Azure File Share %s (timeout: %v)", "testshare", gomock.Any())
 	mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
-	mockLogger.EXPECT().Infof("connected to %s", "testshare").MaxTimes(1)
-	mockLogger.EXPECT().Warnf(gomock.Any(), gomock.Any(), gomock.Any()).MaxTimes(1)
-	mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes()
-	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any()).AnyTimes()
+	mockProvider.EXPECT().Connect(gomock.Any()).Return(nil)
+	mockLogger.EXPECT().Infof("connected to %s", "testshare")
+	mockLogger.EXPECT().Debugf("Successfully connected to Azure File Share %s", "testshare")
+	mockLogger.EXPECT().Debug(gomock.Any())
+	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any())
 
-	fs, err := New(config, mockLogger, mockMetrics)
-	require.NoError(t, err)
+	// First Connect() call - will attempt connection and succeed
+	fs.(*azureFileSystem).Connect()
 
-	// If already connected, Connect() should return immediately
+	// If already connected, Connect() should return immediately (no more calls expected)
 	fs.(*azureFileSystem).Connect()
 
 	fs.(*azureFileSystem).CommonFileSystem.SetDisableRetry(true)
@@ -152,21 +169,35 @@ func TestAzureFileSystem_Connect_NotConnected(t *testing.T) {
 		ShareName:   "testshare",
 	}
 
-	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
-	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).AnyTimes()
+	fs, err := New(config)
+	require.NoError(t, err)
+
+	// Set logger and metrics via UseLogger/UseMetrics
+	fs.UseLogger(mockLogger)
+	fs.UseMetrics(mockMetrics)
+
+	mockLogger.EXPECT().Debugf("Attempting to connect to Azure File Share %s (timeout: %v)", "testshare", gomock.Any())
 	mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
-	mockLogger.EXPECT().Warnf(gomock.Any(), gomock.Any(), gomock.Any()).MaxTimes(1)
-	mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes()
-	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any()).AnyTimes()
+	mockLogger.EXPECT().Debug(gomock.Any())
+	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any())
+	mockLogger.EXPECT().Warnf("Azure File Share %s not available, starting background retry: %v", "testshare", gomock.Any())
+	// Expect logRetryStart from the goroutine (may or may not be called depending on timing)
+	mockLogger.EXPECT().Debugf(
+		"Starting background retry for Azure File Share %s (retry interval: 1 minute)",
+		"testshare",
+	).MaxTimes(1)
 
-	fs, err := New(config, mockLogger, mockMetrics)
-	require.NoError(t, err)
+	// Call Connect - should attempt to connect and start retry
+	fs.(*azureFileSystem).Connect()
+
+	// Give goroutine time to start
+	time.Sleep(50 * time.Millisecond)
 
-	// Mark as not connected
+	// Mark as not connected and disable retry to stop the goroutine
 	fs.(*azureFileSystem).CommonFileSystem.SetDisableRetry(true)
 
-	// Call Connect - should attempt to connect
-	fs.(*azureFileSystem).Connect()
+	// Give goroutine time to check the flag and exit
+	time.Sleep(50 * time.Millisecond)
 }
 
 func TestAzureFileSystem_startRetryConnect(t *testing.T) {
@@ -182,15 +213,22 @@ func TestAzureFileSystem_startRetryConnect(t *testing.T) {
 		ShareName:   "testshare",
 	}
 
+	fs, err := New(config)
+	require.NoError(t, err)
+
+	// Set logger and metrics via UseLogger/UseMetrics
+	fs.UseLogger(mockLogger)
+	fs.UseMetrics(mockMetrics)
+
 	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
 	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).AnyTimes()
 	mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
 	mockLogger.EXPECT().Warnf(gomock.Any(), gomock.Any(), gomock.Any()).MaxTimes(1)
 	mockLogger.EXPECT().Debug(gomock.Any())
 	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any())
 
-	fs, err := New(config, mockLogger, mockMetrics)
-	require.NoError(t, err)
+	// Call Connect which will start retry on failure
+	fs.Connect()
 
 	// Disable retry to stop the goroutine quickly
 	fs.(*azureFileSystem).CommonFileSystem.SetDisableRetry(true)
@@ -212,19 +250,26 @@ func TestAzureFileSystem_startRetryConnect_RetryDisabled(t *testing.T) {
 		ShareName:   "testshare",
 	}
 
+	fs, err := New(config)
+	require.NoError(t, err)
+
+	// Set logger and metrics via UseLogger/UseMetrics
+	fs.UseLogger(mockLogger)
+	fs.UseMetrics(mockMetrics)
+
 	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
 	mockLogger.EXPECT().Debugf(gomock.Any(), gomock.Any()).AnyTimes()
 	mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
 	mockLogger.EXPECT().Warnf(gomock.Any(), gomock.Any(), gomock.Any()).MaxTimes(1)
 	mockLogger.EXPECT().Debug(gomock.Any())
 	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any())
 
-	fs, err := New(config, mockLogger, mockMetrics)
-	require.NoError(t, err)
-
 	// Disable retry immediately - retry loop should exit
 	fs.(*azureFileSystem).CommonFileSystem.SetDisableRetry(true)
 
+	// Call Connect which would start retry, but retry is disabled
+	fs.Connect()
+
 	// Give it a moment to check the retry disabled flag
 	time.Sleep(50 * time.Millisecond)
 }
@@ -321,7 +366,7 @@ func TestNew_NilLogger(t *testing.T) {
 		ShareName:   "testshare",
 	}
 
-	fs, err := New(config, nil, nil)
+	fs, err := New(config)
 
 	require.NoError(t, err)
 	require.NotNil(t, fs)
@@ -344,11 +389,27 @@ func TestAzureFileSystem_Connect_NotConnected_WithLogger(t *testing.T) {
 		},
 	}
 
+	mockLogger.EXPECT().Debugf("Attempting to connect to Azure File Share %s (timeout: %v)", "testshare", gomock.Any())
 	mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
 	mockLogger.EXPECT().Debug(gomock.Any())
 	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any())
+	mockLogger.EXPECT().Warnf("Azure File Share %s not available, starting background retry: %v", "testshare", gomock.Any())
+	// Expect logRetryStart from the goroutine
+	mockLogger.EXPECT().Debugf(
+		"Starting background retry for Azure File Share %s (retry interval: 1 minute)",
+		"testshare",
+	)
 
 	fs.Connect()
+
+	// Give goroutine time to start
+	time.Sleep(100 * time.Millisecond)
+
+	// Disable retry to stop the goroutine
+	fs.CommonFileSystem.SetDisableRetry(true)
+
+	// Give goroutine time to check the flag and exit
+	time.Sleep(100 * time.Millisecond)
 }
 
 func TestAzureFileSystem_logRetryStart_WithLogger(t *testing.T) {
@@ -617,9 +678,11 @@ func TestAzureFileSystem_Connect_WhenNotConnected(t *testing.T) {
 	// Ensure not connected
 	assert.False(t, fs.CommonFileSystem.IsConnected())
 
+	mockLogger.EXPECT().Debugf("Attempting to connect to Azure File Share %s (timeout: %v)", "testshare", gomock.Any())
 	mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
 	mockProvider.EXPECT().Connect(gomock.Any()).Return(nil)
 	mockLogger.EXPECT().Infof("connected to %s", "testshare")
+	mockLogger.EXPECT().Debugf("Successfully connected to Azure File Share %s", "testshare")
 	mockLogger.EXPECT().Debug(gomock.Any())
 	mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any())