From 6382a536f840216127fec789384fbad63c3b00e7 Mon Sep 17 00:00:00 2001 From: Anton Korotkov Date: Sun, 31 Mar 2024 18:01:50 +0300 Subject: [PATCH 1/2] refactor configuration_manager.go by extracting Downloader and S3ManagerBuilder interfaces; add unit tests --- cmd/backup/main.go | 2 +- go.mod | 1 + go.sum | 2 + pkg/service/configuration_manager.go | 110 ++++++++++----- pkg/service/configuration_manager_s3.go | 15 ++- pkg/service/configuration_manager_test.go | 157 ++++++++++++++++++++++ 6 files changed, 253 insertions(+), 34 deletions(-) create mode 100644 pkg/service/configuration_manager_test.go diff --git a/cmd/backup/main.go b/cmd/backup/main.go index d8625be1..f2940cbd 100644 --- a/cmd/backup/main.go +++ b/cmd/backup/main.go @@ -53,7 +53,7 @@ func run() int { rootCmd.Flags().BoolVarP(&remote, "remote", "r", false, "use remote config file") rootCmd.RunE = func(_ *cobra.Command, _ []string) error { - manager, err := service.NewConfigurationManager(configFile, remote) + manager, err := service.NewConfigManagerBuilder().NewConfigManager(configFile, remote) if err != nil { return err } diff --git a/go.mod b/go.mod index 8ed8dbbf..ffb54a7f 100644 --- a/go.mod +++ b/go.mod @@ -53,6 +53,7 @@ require ( github.com/prometheus/procfs v0.12.0 // indirect github.com/rogpeppe/go-internal v1.11.0 // indirect github.com/spf13/pflag v1.0.5 // indirect + github.com/stretchr/objx v0.5.2 // indirect github.com/swaggo/files/v2 v2.0.0 // indirect github.com/yuin/gopher-lua v1.1.1 // indirect golang.org/x/net v0.20.0 // indirect diff --git a/go.sum b/go.sum index 18abebc6..b8df589e 100644 --- a/go.sum +++ b/go.sum @@ -111,6 +111,8 @@ github.com/spf13/cobra v1.8.0/go.mod h1:WXLWApfZ71AjXPya3WOlMsY9yMs7YeiHhFVlvLyh github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA= github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI= github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= diff --git a/pkg/service/configuration_manager.go b/pkg/service/configuration_manager.go index c83b6b3d..185c3bab 100644 --- a/pkg/service/configuration_manager.go +++ b/pkg/service/configuration_manager.go @@ -2,6 +2,7 @@ package service import ( "bytes" + "fmt" "net/http" "net/url" "os" @@ -15,38 +16,88 @@ type ConfigurationManager interface { WriteConfiguration(config *model.Config) error } -func NewConfigurationManager(configFile string, remote bool) (ConfigurationManager, error) { - uri, err := url.Parse(configFile) +type Downloader interface { + Download(string) ([]byte, error) +} + +type HTTPDownloader struct{} + +func (h HTTPDownloader) Download(url string) ([]byte, error) { + resp, err := http.Get(url) if err != nil { return nil, err } + defer resp.Body.Close() + buf := new(bytes.Buffer) + _, err = buf.ReadFrom(resp.Body) + if err != nil { + return nil, err + } + return buf.Bytes(), nil +} - isDownload := uri.Scheme == "http" || uri.Scheme == "https" +type FileReader struct{} - if remote { - return remoteConfigurationManager(configFile, isDownload) +func (f FileReader) Download(url string) ([]byte, error) { + return os.ReadFile(url) +} + +type ConfigManagerBuilder struct { + http Downloader + file Downloader + s3Builder S3ManagerBuilder +} + +func NewConfigManagerBuilder() *ConfigManagerBuilder { + return &ConfigManagerBuilder{ + http: HTTPDownloader{}, + file: FileReader{}, + s3Builder: S3ManagerBuilderImpl{}, } - if isDownload { - return NewHTTPConfigurationManager(configFile), nil +} + +func (b *ConfigManagerBuilder) NewConfigManager(configFile string, remote bool) (ConfigurationManager, error) { + configStorage, err := b.makeConfigStorage(configFile, remote) + if err != nil { + return nil, err + } + + switch configStorage.Type { + case model.S3: + return b.s3Builder.NewS3ConfigurationManager(configStorage) + case model.Local: + return newLocalConfigurationManager(configStorage) + default: + return nil, fmt.Errorf("unknown type %d", configStorage.Type) } - return NewFileConfigurationManager(configFile), nil } -func remoteConfigurationManager(configFile string, isDownload bool) (ConfigurationManager, error) { - var buf []byte - var err error +func newLocalConfigurationManager(configStorage *model.Storage) (ConfigurationManager, error) { + isHttp, err := isDownload(*configStorage.Path) + if err != nil { + return nil, err + } + if isHttp { + return NewHTTPConfigurationManager(*configStorage.Path), nil + } + return NewFileConfigurationManager(*configStorage.Path), nil +} - if isDownload { - buf, err = download(configFile) - } else { - buf, err = os.ReadFile(configFile) +func (b *ConfigManagerBuilder) makeConfigStorage(configUri string, remote bool) (*model.Storage, error) { + if !remote { + return &model.Storage{ + Type: model.Local, + Path: &configUri, + }, nil } + + content, err := b.loadFileContent(configUri) if err != nil { return nil, err } configStorage := &model.Storage{} - err = yaml.Unmarshal(buf, configStorage) + err = yaml.Unmarshal(content, configStorage) if err != nil { return nil, err } @@ -55,27 +106,26 @@ func remoteConfigurationManager(configFile string, isDownload bool) (Configurati if err != nil { return nil, err } - - switch configStorage.Type { - case model.S3: - return NewS3ConfigurationManager(configStorage) - default: - return NewFileConfigurationManager(*configStorage.Path), nil - } + return configStorage, nil } -func download(url string) ([]byte, error) { - resp, err := http.Get(url) +func (b *ConfigManagerBuilder) loadFileContent(configFile string) ([]byte, error) { + isDownload, err := isDownload(configFile) if err != nil { return nil, err } - defer resp.Body.Close() + if isDownload { + return b.http.Download(configFile) + } else { + return b.file.Download(configFile) + } +} - buf := new(bytes.Buffer) - _, err = buf.ReadFrom(resp.Body) +func isDownload(configFile string) (bool, error) { + uri, err := url.Parse(configFile) if err != nil { - return nil, err + return false, err } - return buf.Bytes(), nil + return uri.Scheme == "http" || uri.Scheme == "https", nil } diff --git a/pkg/service/configuration_manager_s3.go b/pkg/service/configuration_manager_s3.go index 38089880..beb059d3 100644 --- a/pkg/service/configuration_manager_s3.go +++ b/pkg/service/configuration_manager_s3.go @@ -6,7 +6,7 @@ import ( "github.com/aerospike/backup/pkg/model" ) -// FileConfigurationManager implements the ConfigurationManager interface, +// S3ConfigurationManager implements the ConfigurationManager interface, // performing I/O operations on AWS S3. type S3ConfigurationManager struct { *S3Context @@ -14,8 +14,17 @@ type S3ConfigurationManager struct { var _ ConfigurationManager = (*S3ConfigurationManager)(nil) -// NewS3ConfigurationManager returns a new S3ConfigurationManager. -func NewS3ConfigurationManager(configStorage *model.Storage) (ConfigurationManager, error) { +// S3ManagerBuilder defines the interface for building S3ConfigurationManager. +type S3ManagerBuilder interface { + // NewS3ConfigurationManager returns a new S3ConfigurationManager. + NewS3ConfigurationManager(configStorage *model.Storage) (ConfigurationManager, error) +} + +type S3ManagerBuilderImpl struct{} + +var _ S3ManagerBuilder = &S3ManagerBuilderImpl{} + +func (builder S3ManagerBuilderImpl) NewS3ConfigurationManager(configStorage *model.Storage) (ConfigurationManager, error) { s3Context, err := NewS3Context(configStorage) if err != nil { return nil, err diff --git a/pkg/service/configuration_manager_test.go b/pkg/service/configuration_manager_test.go new file mode 100644 index 00000000..c681ce42 --- /dev/null +++ b/pkg/service/configuration_manager_test.go @@ -0,0 +1,157 @@ +package service + +import ( + "errors" + "reflect" + "testing" + + "github.com/aerospike/backup/pkg/model" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +type MockDownloader struct { + mock.Mock +} + +func (m *MockDownloader) Download(configFile string) ([]byte, error) { + args := m.Called(configFile) + return args.Get(0).([]byte), args.Error(1) +} + +type MockS3Builder struct { +} + +func (m *MockS3Builder) NewS3ConfigurationManager(storage *model.Storage) (ConfigurationManager, error) { + if storage.Type == model.S3 { + return &S3ConfigurationManager{}, nil + } + return nil, errors.New("wrong type") +} + +func TestConfigManagerBuilder_NewConfigManager(t *testing.T) { + mockLocal := new(MockDownloader) + mockHttp := new(MockDownloader) + + tests := []struct { + name string + configFile string + remote bool + setMock func() + expectError bool + expectedType reflect.Type + }{ + // Configuration file is passed straight to the service. + { + name: "local non-remote", + configFile: "config.yaml", + remote: false, + setMock: func() {}, + expectError: false, + expectedType: reflect.TypeOf(&FileConfigurationManager{}), + }, + { + name: "http non-remote", + configFile: "https://example.com/config.yaml", + remote: false, + setMock: func() {}, + expectError: false, + expectedType: reflect.TypeOf(&HTTPConfigurationManager{}), + }, + // Open/download remote config file, and based on it's content open/download backup config. + { + name: "local remote local configuration", + configFile: "/path/to/remote.yaml", + remote: true, + setMock: func() { + mockLocal. + On("Download", "/path/to/remote.yaml"). + Return([]byte("path: config.yaml"), nil) + }, + expectError: false, + expectedType: reflect.TypeOf(&FileConfigurationManager{}), + }, + { + name: "local remote http configuration", + configFile: "/path/to/remote.yaml", + remote: true, + setMock: func() { + mockLocal. + On("Download", "/path/to/remote.yaml"). + Return([]byte("path: https://example.com/config.yaml"), nil) + }, + expectError: false, + expectedType: reflect.TypeOf(&HTTPConfigurationManager{}), + }, + { + name: "http remote local configuration", + configFile: "https://example.com/config.yaml", + remote: true, + setMock: func() { + mockHttp. + On("Download", "https://example.com/config.yaml"). + Return([]byte("path: config.yaml"), nil) + }, + expectError: false, + expectedType: reflect.TypeOf(&FileConfigurationManager{}), + }, + { + name: "http remote http", + configFile: "http://path/to/remote.yaml", + remote: true, + setMock: func() { + mockHttp. + On("Download", "http://path/to/remote.yaml"). + Return([]byte("path: https://example.com/config.yaml"), nil) + }, + expectError: false, + expectedType: reflect.TypeOf(&HTTPConfigurationManager{}), + }, + // S3 configuration, we can read it from local file or by http. + { + name: "local s3", + configFile: "config.yaml", + remote: true, + setMock: func() { + mockLocal. + On("Download", "config.yaml"). + Return([]byte("type: 1\npath: s3://bucket/config.yaml\ns3-region: europe"), nil) + }, + expectError: false, + expectedType: reflect.TypeOf(&S3ConfigurationManager{}), + }, + { + name: "http s3", + configFile: "https://example.com/config.yaml", + remote: true, + setMock: func() { + mockHttp. + On("Download", "https://example.com/config.yaml"). + Return([]byte("type: 1\npath: s3://bucket/config.yaml\ns3-region: europe"), nil) + }, + expectError: false, + expectedType: reflect.TypeOf(&S3ConfigurationManager{}), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockLocal = new(MockDownloader) + mockHttp = new(MockDownloader) + builder := &ConfigManagerBuilder{ + http: mockHttp, + file: mockLocal, + s3Builder: &MockS3Builder{}, + } + tt.setMock() + config, err := builder.NewConfigManager(tt.configFile, tt.remote) + if tt.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + configType := reflect.TypeOf(config) + require.Equal(t, tt.expectedType.String(), configType.String()) + }) + } +} From 4919d6b3bea7427affb46c207d9f56efc4064500 Mon Sep 17 00:00:00 2001 From: Anton Korotkov Date: Mon, 1 Apr 2024 09:36:16 +0300 Subject: [PATCH 2/2] rename for better readability --- pkg/service/configuration_manager.go | 28 +++++++++++------------ pkg/service/configuration_manager_test.go | 14 ++++++------ 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/pkg/service/configuration_manager.go b/pkg/service/configuration_manager.go index 185c3bab..afad8aaf 100644 --- a/pkg/service/configuration_manager.go +++ b/pkg/service/configuration_manager.go @@ -16,13 +16,13 @@ type ConfigurationManager interface { WriteConfiguration(config *model.Config) error } -type Downloader interface { - Download(string) ([]byte, error) +type Reader interface { + read(string) ([]byte, error) } -type HTTPDownloader struct{} +type HTTPReader struct{} -func (h HTTPDownloader) Download(url string) ([]byte, error) { +func (h HTTPReader) read(url string) ([]byte, error) { resp, err := http.Get(url) if err != nil { return nil, err @@ -38,19 +38,19 @@ func (h HTTPDownloader) Download(url string) ([]byte, error) { type FileReader struct{} -func (f FileReader) Download(url string) ([]byte, error) { +func (f FileReader) read(url string) ([]byte, error) { return os.ReadFile(url) } type ConfigManagerBuilder struct { - http Downloader - file Downloader + http Reader + file Reader s3Builder S3ManagerBuilder } func NewConfigManagerBuilder() *ConfigManagerBuilder { return &ConfigManagerBuilder{ - http: HTTPDownloader{}, + http: HTTPReader{}, file: FileReader{}, s3Builder: S3ManagerBuilderImpl{}, } @@ -73,7 +73,7 @@ func (b *ConfigManagerBuilder) NewConfigManager(configFile string, remote bool) } func newLocalConfigurationManager(configStorage *model.Storage) (ConfigurationManager, error) { - isHttp, err := isDownload(*configStorage.Path) + isHttp, err := isHttpPath(*configStorage.Path) if err != nil { return nil, err } @@ -110,19 +110,19 @@ func (b *ConfigManagerBuilder) makeConfigStorage(configUri string, remote bool) } func (b *ConfigManagerBuilder) loadFileContent(configFile string) ([]byte, error) { - isDownload, err := isDownload(configFile) + isDownload, err := isHttpPath(configFile) if err != nil { return nil, err } if isDownload { - return b.http.Download(configFile) + return b.http.read(configFile) } else { - return b.file.Download(configFile) + return b.file.read(configFile) } } -func isDownload(configFile string) (bool, error) { - uri, err := url.Parse(configFile) +func isHttpPath(path string) (bool, error) { + uri, err := url.Parse(path) if err != nil { return false, err } diff --git a/pkg/service/configuration_manager_test.go b/pkg/service/configuration_manager_test.go index c681ce42..7c94bb69 100644 --- a/pkg/service/configuration_manager_test.go +++ b/pkg/service/configuration_manager_test.go @@ -14,7 +14,7 @@ type MockDownloader struct { mock.Mock } -func (m *MockDownloader) Download(configFile string) ([]byte, error) { +func (m *MockDownloader) read(configFile string) ([]byte, error) { args := m.Called(configFile) return args.Get(0).([]byte), args.Error(1) } @@ -65,7 +65,7 @@ func TestConfigManagerBuilder_NewConfigManager(t *testing.T) { remote: true, setMock: func() { mockLocal. - On("Download", "/path/to/remote.yaml"). + On("read", "/path/to/remote.yaml"). Return([]byte("path: config.yaml"), nil) }, expectError: false, @@ -77,7 +77,7 @@ func TestConfigManagerBuilder_NewConfigManager(t *testing.T) { remote: true, setMock: func() { mockLocal. - On("Download", "/path/to/remote.yaml"). + On("read", "/path/to/remote.yaml"). Return([]byte("path: https://example.com/config.yaml"), nil) }, expectError: false, @@ -89,7 +89,7 @@ func TestConfigManagerBuilder_NewConfigManager(t *testing.T) { remote: true, setMock: func() { mockHttp. - On("Download", "https://example.com/config.yaml"). + On("read", "https://example.com/config.yaml"). Return([]byte("path: config.yaml"), nil) }, expectError: false, @@ -101,7 +101,7 @@ func TestConfigManagerBuilder_NewConfigManager(t *testing.T) { remote: true, setMock: func() { mockHttp. - On("Download", "http://path/to/remote.yaml"). + On("read", "http://path/to/remote.yaml"). Return([]byte("path: https://example.com/config.yaml"), nil) }, expectError: false, @@ -114,7 +114,7 @@ func TestConfigManagerBuilder_NewConfigManager(t *testing.T) { remote: true, setMock: func() { mockLocal. - On("Download", "config.yaml"). + On("read", "config.yaml"). Return([]byte("type: 1\npath: s3://bucket/config.yaml\ns3-region: europe"), nil) }, expectError: false, @@ -126,7 +126,7 @@ func TestConfigManagerBuilder_NewConfigManager(t *testing.T) { remote: true, setMock: func() { mockHttp. - On("Download", "https://example.com/config.yaml"). + On("read", "https://example.com/config.yaml"). Return([]byte("type: 1\npath: s3://bucket/config.yaml\ns3-region: europe"), nil) }, expectError: false,