From: Jeff Halter Date: Thu, 9 Jun 2022 02:09:38 +0000 (-0700) Subject: Refactor filestore to simplify testing X-Git-Url: https://git.r.bdr.sh/rbdr/mobius/commitdiff_plain/b196a50a44a5c72fc43df39fce4407d05ec8863b?hp=8168522a18e78acff2f6ca2f04b9f9300c6ee7a1 Refactor filestore to simplify testing --- diff --git a/cmd/mobius-hotline-server/main.go b/cmd/mobius-hotline-server/main.go index f18ae9d..60327a8 100644 --- a/cmd/mobius-hotline-server/main.go +++ b/cmd/mobius-hotline-server/main.go @@ -48,9 +48,7 @@ func main() { logger.Fatalw("Configuration directory not found", "path", configDir) } - hotline.FS = &hotline.OSFileStore{} - - srv, err := hotline.NewServer(*configDir, "", *basePort, logger) + srv, err := hotline.NewServer(*configDir, "", *basePort, logger, &hotline.OSFileStore{}) if err != nil { logger.Fatal(err) } diff --git a/hotline/file_store.go b/hotline/file_store.go index ba86c6a..2ba9d7a 100644 --- a/hotline/file_store.go +++ b/hotline/file_store.go @@ -6,8 +6,6 @@ import ( "os" ) -var FS FileStore - type FileStore interface { Mkdir(name string, perm os.FileMode) error Stat(name string) (os.FileInfo, error) diff --git a/hotline/server.go b/hotline/server.go index b1d8849..33888e0 100644 --- a/hotline/server.go +++ b/hotline/server.go @@ -50,6 +50,7 @@ type Server struct { TrackerPassID [4]byte Stats *Stats + FS FileStore // newsReader io.Reader // newsWriter io.WriteCloser @@ -187,7 +188,7 @@ const ( ) // NewServer constructs a new Server from a config dir -func NewServer(configDir, netInterface string, netPort int, logger *zap.SugaredLogger) (*Server, error) { +func NewServer(configDir, netInterface string, netPort int, logger *zap.SugaredLogger, FS FileStore) (*Server, error) { server := Server{ Port: netPort, Accounts: make(map[string]*Account), @@ -201,6 +202,7 @@ func NewServer(configDir, netInterface string, netPort int, logger *zap.SugaredL outbox: make(chan Transaction), Stats: &Stats{StartTime: time.Now()}, ThreadedNews: &ThreadedNews{}, + FS: FS, } var err error @@ -210,7 +212,8 @@ func NewServer(configDir, netInterface string, netPort int, logger *zap.SugaredL return nil, err } - if server.Agreement, err = os.ReadFile(configDir + agreementFile); err != nil { + server.Agreement, err = os.ReadFile(configDir + agreementFile) + if err != nil { return nil, err } @@ -361,7 +364,7 @@ func (s *Server) NewUser(login, name, password string, access []byte) error { } s.Accounts[login] = &account - return FS.WriteFile(s.ConfigDir+"Users/"+login+".yaml", out, 0666) + return s.FS.WriteFile(s.ConfigDir+"Users/"+login+".yaml", out, 0666) } func (s *Server) UpdateUser(login, newLogin, name, password string, access []byte) error { @@ -401,7 +404,7 @@ func (s *Server) DeleteUser(login string) error { delete(s.Accounts, login) - return FS.Remove(s.ConfigDir + "Users/" + login + ".yaml") + return s.FS.Remove(s.ConfigDir + "Users/" + login + ".yaml") } func (s *Server) connectedUsers() []Field { @@ -447,7 +450,7 @@ func (s *Server) loadAccounts(userDir string) error { } for _, file := range matches { - fh, err := FS.Open(file) + fh, err := s.FS.Open(file) if err != nil { return err } @@ -464,7 +467,7 @@ func (s *Server) loadAccounts(userDir string) error { } func (s *Server) loadConfig(path string) error { - fh, err := FS.Open(path) + fh, err := s.FS.Open(path) if err != nil { return err } @@ -714,7 +717,7 @@ func (s *Server) handleFileTransfer(conn io.ReadWriteCloser) error { } } - file, err := FS.Open(fullFilePath) + file, err := s.FS.Open(fullFilePath) if err != nil { return err } @@ -906,7 +909,7 @@ func (s *Server) handleFileTransfer(conn io.ReadWriteCloser) error { return err } - file, err := FS.Open(path) + file, err := s.FS.Open(path) if err != nil { return err } @@ -963,8 +966,8 @@ func (s *Server) handleFileTransfer(conn io.ReadWriteCloser) error { ) // Check if the target folder exists. If not, create it. - if _, err := FS.Stat(dstPath); os.IsNotExist(err) { - if err := FS.Mkdir(dstPath, 0777); err != nil { + if _, err := s.FS.Stat(dstPath); os.IsNotExist(err) { + if err := s.FS.Mkdir(dstPath, 0777); err != nil { return err } } @@ -1077,7 +1080,7 @@ func (s *Server) handleFileTransfer(conn io.ReadWriteCloser) error { filePath := dstPath + "/" + fu.FormattedPath() s.Logger.Infow("Starting file transfer", "path", filePath, "fileNum", i+1, "totalFiles", "zz", "fileSize", binary.BigEndian.Uint32(fileSize)) - newFile, err := FS.Create(filePath + ".incomplete") + newFile, err := s.FS.Create(filePath + ".incomplete") if err != nil { return err } diff --git a/hotline/transaction_handlers.go b/hotline/transaction_handlers.go index f4c5a2f..1b69114 100644 --- a/hotline/transaction_handlers.go +++ b/hotline/transaction_handlers.go @@ -391,7 +391,7 @@ func HandleSetFileInfo(cc *ClientConn, t *Transaction) (res []Transaction, err e fileNewName := t.GetField(fieldFileNewName).Data if fileNewName != nil { - fi, err := FS.Stat(fullFilePath) + fi, err := cc.Server.FS.Stat(fullFilePath) if err != nil { return res, err } @@ -524,14 +524,14 @@ func HandleNewFolder(cc *ClientConn, t *Transaction) (res []Transaction, err err // TODO: check path and folder name lengths - if _, err := FS.Stat(newFolderPath); !os.IsNotExist(err) { + if _, err := cc.Server.FS.Stat(newFolderPath); !os.IsNotExist(err) { msg := fmt.Sprintf("Cannot create folder \"%s\" because there is already a file or folder with that name.", folderName) return []Transaction{cc.NewErrReply(t, msg)}, nil } // TODO: check for disallowed characters to maintain compatibility for original client - if err := FS.Mkdir(newFolderPath, 0777); err != nil { + if err := cc.Server.FS.Mkdir(newFolderPath, 0777); err != nil { msg := fmt.Sprintf("Cannot create folder \"%s\" because an error occurred.", folderName) return []Transaction{cc.NewErrReply(t, msg)}, nil } @@ -1523,7 +1523,7 @@ func HandleUploadFile(cc *ClientConn, t *Transaction) (res []Transaction, err er return res, err } - fileInfo, err := FS.Stat(fullFilePath + incompleteFileSuffix) + fileInfo, err := cc.Server.FS.Stat(fullFilePath + incompleteFileSuffix) if err != nil { return res, err } @@ -1856,7 +1856,7 @@ func HandleMakeAlias(cc *ClientConn, t *Transaction) (res []Transaction, err err cc.Server.Logger.Debugw("Make alias", "src", fullFilePath, "dst", fullNewFilePath) - if err := FS.Symlink(fullFilePath, fullNewFilePath); err != nil { + if err := cc.Server.FS.Symlink(fullFilePath, fullNewFilePath); err != nil { res = append(res, cc.NewErrReply(t, "Error creating alias")) return res, nil } diff --git a/hotline/transaction_handlers_test.go b/hotline/transaction_handlers_test.go index c174822..01ae862 100644 --- a/hotline/transaction_handlers_test.go +++ b/hotline/transaction_handlers_test.go @@ -689,15 +689,13 @@ func TestHandleNewFolder(t *testing.T) { t *Transaction } tests := []struct { - setup func() name string args args wantRes []Transaction wantErr bool }{ { - name: "without required permission", - setup: func() {}, + name: "without required permission", args: args{ cc: &ClientConn{ Account: &Account{ @@ -744,6 +742,12 @@ func TestHandleNewFolder(t *testing.T) { Config: &Config{ FileRoot: "/Files/", }, + FS: func() *MockFileStore { + mfs := &MockFileStore{} + mfs.On("Mkdir", "/Files/aaa/testFolder", fs.FileMode(0777)).Return(nil) + mfs.On("Stat", "/Files/aaa/testFolder").Return(nil, os.ErrNotExist) + return mfs + }(), }, }, t: NewTransaction( @@ -757,12 +761,6 @@ func TestHandleNewFolder(t *testing.T) { }), ), }, - setup: func() { - mfs := &MockFileStore{} - mfs.On("Mkdir", "/Files/aaa/testFolder", fs.FileMode(0777)).Return(nil) - mfs.On("Stat", "/Files/aaa/testFolder").Return(nil, os.ErrNotExist) - FS = mfs - }, wantRes: []Transaction{ { clientID: &[]byte{0, 1}, @@ -792,6 +790,12 @@ func TestHandleNewFolder(t *testing.T) { Config: &Config{ FileRoot: "/Files", }, + FS: func() *MockFileStore { + mfs := &MockFileStore{} + mfs.On("Mkdir", "/Files/testFolder", fs.FileMode(0777)).Return(nil) + mfs.On("Stat", "/Files/testFolder").Return(nil, os.ErrNotExist) + return mfs + }(), }, }, t: NewTransaction( @@ -799,12 +803,6 @@ func TestHandleNewFolder(t *testing.T) { NewField(fieldFileName, []byte("testFolder")), ), }, - setup: func() { - mfs := &MockFileStore{} - mfs.On("Mkdir", "/Files/testFolder", fs.FileMode(0777)).Return(nil) - mfs.On("Stat", "/Files/testFolder").Return(nil, os.ErrNotExist) - FS = mfs - }, wantRes: []Transaction{ { clientID: &[]byte{0, 1}, @@ -834,6 +832,12 @@ func TestHandleNewFolder(t *testing.T) { Config: &Config{ FileRoot: "/Files/", }, + FS: func() *MockFileStore { + mfs := &MockFileStore{} + mfs.On("Mkdir", "/Files/aaa/testFolder", fs.FileMode(0777)).Return(nil) + mfs.On("Stat", "/Files/aaa/testFolder").Return(nil, os.ErrNotExist) + return mfs + }(), }, }, t: NewTransaction( @@ -844,12 +848,6 @@ func TestHandleNewFolder(t *testing.T) { }), ), }, - setup: func() { - mfs := &MockFileStore{} - mfs.On("Mkdir", "/Files/aaa/testFolder", fs.FileMode(0777)).Return(nil) - mfs.On("Stat", "/Files/aaa/testFolder").Return(nil, os.ErrNotExist) - FS = mfs - }, wantRes: []Transaction{}, wantErr: true, }, @@ -870,6 +868,12 @@ func TestHandleNewFolder(t *testing.T) { Config: &Config{ FileRoot: "/Files/", }, + FS: func() *MockFileStore { + mfs := &MockFileStore{} + mfs.On("Mkdir", "/Files/testFolder", fs.FileMode(0777)).Return(nil) + mfs.On("Stat", "/Files/testFolder").Return(nil, os.ErrNotExist) + return mfs + }(), }, }, t: NewTransaction( @@ -877,12 +881,6 @@ func TestHandleNewFolder(t *testing.T) { NewField(fieldFileName, []byte("../../testFolder")), ), }, - setup: func() { - mfs := &MockFileStore{} - mfs.On("Mkdir", "/Files/testFolder", fs.FileMode(0777)).Return(nil) - mfs.On("Stat", "/Files/testFolder").Return(nil, os.ErrNotExist) - FS = mfs - }, wantRes: []Transaction{ { clientID: &[]byte{0, 1}, @@ -911,6 +909,12 @@ func TestHandleNewFolder(t *testing.T) { Config: &Config{ FileRoot: "/Files/", }, + FS: func() *MockFileStore { + mfs := &MockFileStore{} + mfs.On("Mkdir", "/Files/foo/testFolder", fs.FileMode(0777)).Return(nil) + mfs.On("Stat", "/Files/foo/testFolder").Return(nil, os.ErrNotExist) + return mfs + }(), }, }, t: NewTransaction( @@ -927,12 +931,6 @@ func TestHandleNewFolder(t *testing.T) { }), ), }, - setup: func() { - mfs := &MockFileStore{} - mfs.On("Mkdir", "/Files/foo/testFolder", fs.FileMode(0777)).Return(nil) - mfs.On("Stat", "/Files/foo/testFolder").Return(nil, os.ErrNotExist) - FS = mfs - }, wantRes: []Transaction{ { clientID: &[]byte{0, 1}, @@ -947,7 +945,6 @@ func TestHandleNewFolder(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.setup() gotRes, err := HandleNewFolder(tt.args.cc, tt.args.t) if (err != nil) != tt.wantErr { @@ -1078,23 +1075,12 @@ func TestHandleMakeAlias(t *testing.T) { } tests := []struct { name string - setup func() args args wantRes []Transaction wantErr bool }{ { name: "with valid input and required permissions", - setup: func() { - mfs := &MockFileStore{} - path, _ := os.Getwd() - mfs.On( - "Symlink", - path+"/test/config/Files/foo/testFile", - path+"/test/config/Files/bar/testFile", - ).Return(nil) - FS = mfs - }, args: args{ cc: &ClientConn{ Account: &Account{ @@ -1113,6 +1099,16 @@ func TestHandleMakeAlias(t *testing.T) { }(), }, Logger: NewTestLogger(), + FS: func() *MockFileStore { + mfs := &MockFileStore{} + path, _ := os.Getwd() + mfs.On( + "Symlink", + path+"/test/config/Files/foo/testFile", + path+"/test/config/Files/bar/testFile", + ).Return(nil) + return mfs + }(), }, }, t: NewTransaction( @@ -1136,16 +1132,6 @@ func TestHandleMakeAlias(t *testing.T) { }, { name: "when symlink returns an error", - setup: func() { - mfs := &MockFileStore{} - path, _ := os.Getwd() - mfs.On( - "Symlink", - path+"/test/config/Files/foo/testFile", - path+"/test/config/Files/bar/testFile", - ).Return(errors.New("ohno")) - FS = mfs - }, args: args{ cc: &ClientConn{ Account: &Account{ @@ -1164,6 +1150,16 @@ func TestHandleMakeAlias(t *testing.T) { }(), }, Logger: NewTestLogger(), + FS: func() *MockFileStore { + mfs := &MockFileStore{} + path, _ := os.Getwd() + mfs.On( + "Symlink", + path+"/test/config/Files/foo/testFile", + path+"/test/config/Files/bar/testFile", + ).Return(errors.New("ohno")) + return mfs + }(), }, }, t: NewTransaction( @@ -1188,8 +1184,7 @@ func TestHandleMakeAlias(t *testing.T) { wantErr: false, }, { - name: "when user does not have required permission", - setup: func() {}, + name: "when user does not have required permission", args: args{ cc: &ClientConn{ Account: &Account{ @@ -1242,8 +1237,6 @@ func TestHandleMakeAlias(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.setup() - gotRes, err := HandleMakeAlias(tt.args.cc, tt.args.t) if (err != nil) != tt.wantErr { t.Errorf("HandleMakeAlias(%v, %v)", tt.args.cc, tt.args.t) @@ -1400,18 +1393,12 @@ func TestHandleDeleteUser(t *testing.T) { } tests := []struct { name string - setup func() args args wantRes []Transaction wantErr assert.ErrorAssertionFunc }{ { name: "when user exists", - setup: func() { - mfs := &MockFileStore{} - mfs.On("Remove", "Users/testuser.yaml").Return(nil) - FS = mfs - }, args: args{ cc: &ClientConn{ Account: &Account{ @@ -1431,6 +1418,11 @@ func TestHandleDeleteUser(t *testing.T) { Access: &[]byte{1}, }, }, + FS: func() *MockFileStore { + mfs := &MockFileStore{} + mfs.On("Remove", "Users/testuser.yaml").Return(nil) + return mfs + }(), }, }, t: NewTransaction( @@ -1451,8 +1443,7 @@ func TestHandleDeleteUser(t *testing.T) { wantErr: assert.NoError, }, { - name: "when user does not have required permission", - setup: func() {}, + name: "when user does not have required permission", args: args{ cc: &ClientConn{ Account: &Account{ @@ -1488,7 +1479,6 @@ func TestHandleDeleteUser(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - tt.setup() gotRes, err := HandleDeleteUser(tt.args.cc, tt.args.t) if !tt.wantErr(t, err, fmt.Sprintf("HandleDeleteUser(%v, %v)", tt.args.cc, tt.args.t)) { return