Skip to content

Commit f6f6607

Browse files
Umang01-hashUmang01-hash
authored
Merge branch 'development' into fix-file
2 parents bb2f900 + 924c569 commit f6f6607

File tree

6 files changed

+133
-89
lines changed

6 files changed

+133
-89
lines changed

‎pkg/gofr/datasource/file/common_file.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -360,7 +360,7 @@ func (f *CommonFile) observe(operation string, startTime time.Time, status, mess
360360
Metrics: f.metrics,
361361
Operation: operation,
362362
Location: f.location,
363-
Provider: "GoFr-File",
363+
Provider: "FILE",
364364
StartTime: startTime,
365365
Status: status,
366366
Message: message,

‎pkg/gofr/datasource/file/common_fs.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ func (c *CommonFileSystem) getProviderName() string {
610610
return c.ProviderName
611611
}
612612

613-
return "Common"
613+
return "COMMON"
614614
}
615615

616616
// Observe is a helper method to centralize observability for all operations.

‎pkg/gofr/datasource/file/ftp/fs.go‎

Lines changed: 64 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,12 @@ import (
66
"fmt"
77
"time"
88

9-
"gofr.dev/pkg/gofr/datasource"
109
"gofr.dev/pkg/gofr/datasource/file"
1110
)
1211

1312
var (
14-
errInvalidConfig = errors.New("invalid FTP configuration: host and port are required")
13+
errInvalidConfig = errors.New("invalid FTP configuration: host and port are required")
14+
errInvalidProvider = errors.New("invalid FTP provider")
1515
)
1616

1717
const defaultTimeout = 10 * time.Second
@@ -22,9 +22,9 @@ type fileSystem struct {
2222

2323
// New creates and validates a new FTP file system.
2424
// Returns error if connection fails or configuration is invalid.
25-
func New(config *Config, logger datasource.Logger, metrics file.StorageMetrics) (file.FileSystemProvider, error) {
26-
if config == nil || config.Host == "" || config.Port <= 0 {
27-
return nil, errInvalidConfig
25+
func New(config *Config) file.FileSystemProvider {
26+
if config == nil {
27+
config = &Config{}
2828
}
2929

3030
// Set default dial timeout if not specified
@@ -34,37 +34,37 @@ func New(config *Config, logger datasource.Logger, metrics file.StorageMetrics)
3434

3535
adapter := &storageAdapter{cfg: config}
3636

37-
location := fmt.Sprintf("%s:%d", config.Host, config.Port)
38-
if config.RemoteDir != "" && config.RemoteDir != "/" {
39-
location = fmt.Sprintf("%s:%d%s", config.Host, config.Port, config.RemoteDir)
40-
}
37+
location := buildLocation(config)
4138

4239
fs := &fileSystem{
4340
CommonFileSystem: &file.CommonFileSystem{
4441
Provider: adapter,
4542
Location: location,
46-
Logger: logger,
47-
Metrics: metrics,
4843
ProviderName: "FTP",
4944
},
5045
}
5146

52-
ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
53-
defer cancel()
47+
return fs
48+
}
5449

55-
// Attempt initial connection via CommonFileSystem.Connect
56-
if err := fs.CommonFileSystem.Connect(ctx); err != nil {
57-
if logger != nil {
58-
logger.Warnf("FTP server %s not available, starting background retry: %v", config.Host, err)
59-
}
50+
// buildLocation creates the location string for metrics/logging.
51+
func buildLocation(config *Config) string {
52+
if config.Host == "" {
53+
return "ftp://unconfigured"
54+
}
55+
56+
port := config.Port
57+
if port == 0 {
58+
port = 21 // Default FTP port
59+
}
6060

61-
go fs.startRetryConnect()
61+
location := fmt.Sprintf("%s:%d", config.Host, port)
6262

63-
return fs, nil
63+
if config.RemoteDir != "" && config.RemoteDir != "/" {
64+
location = fmt.Sprintf("%s:%d%s", config.Host, port, config.RemoteDir)
6465
}
6566

66-
// Connected successfully
67-
return fs, nil
67+
return location
6868
}
6969

7070
// Connect tries a single immediate connect via provider; on failure it starts a background retry.
@@ -73,10 +73,36 @@ func (f *fileSystem) Connect() {
7373
return
7474
}
7575

76+
// Validate configuration before attempting connection
77+
if err := f.validateConfig(); err != nil {
78+
if f.CommonFileSystem.Logger != nil {
79+
f.CommonFileSystem.Logger.Errorf("Invalid FTP configuration: %v", err)
80+
}
81+
82+
return
83+
}
84+
7685
ctx, cancel := context.WithTimeout(context.Background(), defaultTimeout)
7786
defer cancel()
7887

79-
_ = f.CommonFileSystem.Connect(ctx)
88+
err := f.CommonFileSystem.Connect(ctx)
89+
if err != nil {
90+
// Log warning if logger is available
91+
if f.CommonFileSystem.Logger != nil {
92+
f.CommonFileSystem.Logger.Warnf("FTP server %s not available, starting background retry: %v",
93+
f.CommonFileSystem.Location, err)
94+
}
95+
96+
// Start background retry
97+
go f.startRetryConnect()
98+
99+
return
100+
}
101+
102+
// Connected successfully
103+
if f.CommonFileSystem.Logger != nil {
104+
f.CommonFileSystem.Logger.Infof("FTP connection established to server %s", f.CommonFileSystem.Location)
105+
}
80106
}
81107

82108
// startRetryConnect retries connection every 30 seconds until success.
@@ -114,3 +140,18 @@ func (f *fileSystem) startRetryConnect() {
114140
}
115141
}
116142
}
143+
144+
// validateConfig checks if the configuration is valid.
145+
func (f *fileSystem) validateConfig() error {
146+
adapter, ok := f.CommonFileSystem.Provider.(*storageAdapter)
147+
if !ok {
148+
return errInvalidProvider
149+
}
150+
151+
cfg := adapter.cfg
152+
if cfg == nil || cfg.Host == "" || cfg.Port <= 0 {
153+
return errInvalidConfig
154+
}
155+
156+
return nil
157+
}

‎pkg/gofr/datasource/file/ftp/fs_test.go‎

Lines changed: 53 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -11,41 +11,30 @@ import (
1111
)
1212

1313
func TestNew_NilConfig(t *testing.T) {
14-
fs, err := New(nil, nil, nil)
14+
fs := New(nil)
1515

16-
require.Error(t, err)
17-
assert.Nil(t, fs)
18-
assert.ErrorIs(t, err, errInvalidConfig)
16+
require.NotNil(t, fs)
17+
assert.Equal(t, "ftp://unconfigured", fs.(*fileSystem).CommonFileSystem.Location)
1918
}
2019

2120
func TestNew_EmptyHost(t *testing.T) {
2221
config := &Config{Host: "", Port: 2121}
2322

24-
fs, err := New(config, nil, nil)
25-
26-
require.Error(t, err)
27-
assert.Nil(t, fs)
28-
assert.ErrorIs(t, err, errInvalidConfig)
29-
}
30-
31-
func TestNew_InvalidPort_Negative(t *testing.T) {
32-
config := &Config{Host: "localhost", Port: -1}
23+
fs := New(config)
3324

34-
fs, err := New(config, nil, nil)
25+
require.NotNil(t, fs)
3526

36-
require.Error(t, err)
37-
assert.Nil(t, fs)
38-
assert.ErrorIs(t, err, errInvalidConfig)
27+
assert.Equal(t, "ftp://unconfigured", fs.(*fileSystem).CommonFileSystem.Location)
3928
}
4029

4130
func TestNew_PortZero(t *testing.T) {
4231
config := &Config{Host: "localhost", Port: 0}
4332

44-
fs, err := New(config, nil, nil)
33+
fs := New(config)
4534

46-
require.Error(t, err)
47-
assert.Nil(t, fs)
48-
assert.ErrorIs(t, err, errInvalidConfig)
35+
require.NotNil(t, fs)
36+
// Port 0 will default to 21 in buildLocation
37+
assert.Equal(t, "localhost:21", fs.(*fileSystem).CommonFileSystem.Location)
4938
}
5039

5140
func TestNew_ConnectionFailure_StartsRetry(t *testing.T) {
@@ -62,6 +51,13 @@ func TestNew_ConnectionFailure_StartsRetry(t *testing.T) {
6251
Password: "testpass",
6352
}
6453

54+
fs := New(config)
55+
require.NotNil(t, fs)
56+
57+
// Inject logger and metrics (mimicking AddFileStore behavior)
58+
fs.UseLogger(mockLogger)
59+
fs.UseMetrics(mockMetrics)
60+
6561
mockLogger.EXPECT().Warnf(
6662
"FTP server %s not available, starting background retry: %v",
6763
gomock.Any(),
@@ -72,10 +68,7 @@ func TestNew_ConnectionFailure_StartsRetry(t *testing.T) {
7268
mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes()
7369
mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any()).AnyTimes()
7470

75-
fs, err := New(config, mockLogger, mockMetrics)
76-
77-
require.NoError(t, err)
78-
require.NotNil(t, fs)
71+
fs.Connect()
7972

8073
time.Sleep(100 * time.Millisecond)
8174

@@ -96,6 +89,15 @@ func TestNew_Success(t *testing.T) {
9689
Password: "testpass",
9790
}
9891

92+
fs := New(config)
93+
require.NotNil(t, fs)
94+
95+
assert.Equal(t, "localhost:2121", fs.(*fileSystem).CommonFileSystem.Location)
96+
97+
// Inject logger and metrics
98+
fs.UseLogger(mockLogger)
99+
fs.UseMetrics(mockMetrics)
100+
99101
mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
100102

101103
mockLogger.EXPECT().Infof("connected to %s", "localhost:2121").MaxTimes(1)
@@ -108,12 +110,7 @@ func TestNew_Success(t *testing.T) {
108110
mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes()
109111
mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any()).AnyTimes()
110112

111-
fs, err := New(config, mockLogger, mockMetrics)
112-
113-
require.NoError(t, err)
114-
require.NotNil(t, fs)
115-
116-
assert.Equal(t, "localhost:2121", fs.(*fileSystem).CommonFileSystem.Location)
113+
fs.Connect()
117114
}
118115

119116
func TestNew_WithRemoteDir(t *testing.T) {
@@ -131,6 +128,16 @@ func TestNew_WithRemoteDir(t *testing.T) {
131128
RemoteDir: "/uploads",
132129
}
133130

131+
fs := New(config)
132+
133+
require.NotNil(t, fs)
134+
135+
assert.Equal(t, "localhost:2121/uploads", fs.(*fileSystem).CommonFileSystem.Location)
136+
137+
// Inject logger and metrics
138+
fs.UseLogger(mockLogger)
139+
fs.UseMetrics(mockMetrics)
140+
134141
mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
135142

136143
mockLogger.EXPECT().Infof("connected to %s", "localhost:2121/uploads").MaxTimes(1)
@@ -143,12 +150,7 @@ func TestNew_WithRemoteDir(t *testing.T) {
143150
mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes()
144151
mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any()).AnyTimes()
145152

146-
fs, err := New(config, mockLogger, mockMetrics)
147-
148-
require.NoError(t, err)
149-
require.NotNil(t, fs)
150-
151-
assert.Equal(t, "localhost:2121/uploads", fs.(*fileSystem).CommonFileSystem.Location)
153+
fs.Connect()
152154
}
153155

154156
func TestNew_WithRootRemoteDir(t *testing.T) {
@@ -166,6 +168,15 @@ func TestNew_WithRootRemoteDir(t *testing.T) {
166168
RemoteDir: "/",
167169
}
168170

171+
fs := New(config)
172+
require.NotNil(t, fs)
173+
174+
assert.Equal(t, "localhost:2121", fs.(*fileSystem).CommonFileSystem.Location)
175+
176+
// Inject logger and metrics
177+
fs.UseLogger(mockLogger)
178+
fs.UseMetrics(mockMetrics)
179+
169180
mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
170181

171182
mockLogger.EXPECT().Infof("connected to %s", "localhost:2121").MaxTimes(1)
@@ -178,21 +189,13 @@ func TestNew_WithRootRemoteDir(t *testing.T) {
178189
mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes()
179190
mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any()).AnyTimes()
180191

181-
fs, err := New(config, mockLogger, mockMetrics)
182-
183-
require.NoError(t, err)
184-
require.NotNil(t, fs)
185-
186-
assert.Equal(t, "localhost:2121", fs.(*fileSystem).CommonFileSystem.Location)
192+
fs.Connect()
187193
}
188194

189195
func TestNew_WithCustomDialTimeout(t *testing.T) {
190196
ctrl := gomock.NewController(t)
191197
defer ctrl.Finish()
192198

193-
mockLogger := file.NewMockLogger(ctrl)
194-
mockMetrics := file.NewMockMetrics(ctrl)
195-
196199
config := &Config{
197200
Host: "localhost",
198201
Port: 2121,
@@ -201,22 +204,12 @@ func TestNew_WithCustomDialTimeout(t *testing.T) {
201204
DialTimeout: 3 * time.Second,
202205
}
203206

204-
mockMetrics.EXPECT().NewHistogram(file.AppFileStats, gomock.Any(), gomock.Any())
205-
206-
mockLogger.EXPECT().Infof("connected to %s", "localhost:2121").MaxTimes(1)
207-
mockLogger.EXPECT().Warnf(
208-
"FTP server %s not available, starting background retry: %v",
209-
gomock.Any(),
210-
gomock.Any(),
211-
).MaxTimes(1)
212-
213-
mockLogger.EXPECT().Debug(gomock.Any()).AnyTimes()
214-
mockMetrics.EXPECT().RecordHistogram(gomock.Any(), file.AppFileStats, gomock.Any(), gomock.Any()).AnyTimes()
215-
216-
fs, err := New(config, mockLogger, mockMetrics)
207+
fs := New(config)
217208

218-
require.NoError(t, err)
219209
require.NotNil(t, fs)
210+
211+
adapter := fs.(*fileSystem).CommonFileSystem.Provider.(*storageAdapter)
212+
assert.Equal(t, 3*time.Second, adapter.cfg.DialTimeout)
220213
}
221214

222215
func TestConnect_AlreadyConnected(t *testing.T) {

‎pkg/gofr/datasource/file/ftp/storage_adapter.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,8 @@ var (
2929
errFailedToDeleteObject = errors.New("failed to delete object")
3030
errFailedToListObjects = errors.New("failed to list objects")
3131
errFailedToListDirectory = errors.New("failed to list directory")
32-
errWriterAlreadyClosed = errors.New("writer already closed") // Add this
32+
errWriterAlreadyClosed = errors.New("writer already closed")
33+
errFTPConfigInvalid = errors.New("invalid FTP configuration: host and port are required")
3334
)
3435

3536
// Config represents the FTP configuration.
@@ -59,6 +60,10 @@ func (s *storageAdapter) Connect(_ context.Context) error {
5960
return errFTPConfigNil
6061
}
6162

63+
if s.cfg.Host == "" || s.cfg.Port <= 0 {
64+
return errFTPConfigInvalid
65+
}
66+
6267
// Set default timeout if not specified
6368
dialTimeout := s.cfg.DialTimeout
6469
if dialTimeout == 0 {

0 commit comments

Comments
 (0)