From 9c18f1fb30e69e077a7ca65f9e96ee118e828521 Mon Sep 17 00:00:00 2001 From: James Hillyerd Date: Sat, 10 Mar 2018 18:50:18 -0800 Subject: [PATCH] Large refactor for #69 - makefile: Don't refresh deps automatically, causes double build - storage: Move GetMessage, GetMessages (Mailbox), PurgeMessages to the Store API for #69 - storage: Remove Mailbox.Name method for #69 - test: Create new test package for #79 - test: Implement StoreStub, migrate some tests off MockDataStore for task #80 - rest & webui: update controllers to use new Store methods --- .travis.yml | 1 + Makefile | 4 +- pkg/rest/apiv1_controller.go | 35 ++---------- pkg/rest/apiv1_controller_test.go | 71 ++++-------------------- pkg/server/pop3/handler.go | 7 +-- pkg/server/smtp/handler.go | 7 ++- pkg/storage/file/fstore.go | 49 +++++++++++++---- pkg/storage/file/fstore_test.go | 91 +++++++------------------------ pkg/storage/storage.go | 6 +- pkg/storage/testing.go | 24 ++++++-- pkg/test/storage.go | 47 ++++++++++++++++ pkg/webui/mailbox_controller.go | 45 +++------------ 12 files changed, 160 insertions(+), 227 deletions(-) create mode 100644 pkg/test/storage.go diff --git a/.travis.yml b/.travis.yml index c820ee3..817ab3f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -6,6 +6,7 @@ env: before_script: - go get github.com/golang/lint/golint + - make deps go: - "1.10" diff --git a/Makefile b/Makefile index c5104cc..821c674 100644 --- a/Makefile +++ b/Makefile @@ -19,9 +19,9 @@ clean: deps: go get -t ./... -build: deps $(commands) +build: $(commands) -test: deps +test: go test -race ./... fmt: diff --git a/pkg/rest/apiv1_controller.go b/pkg/rest/apiv1_controller.go index 010bfac..9d7d1c6 100644 --- a/pkg/rest/apiv1_controller.go +++ b/pkg/rest/apiv1_controller.go @@ -24,12 +24,7 @@ func MailboxListV1(w http.ResponseWriter, req *http.Request, ctx *web.Context) ( if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - messages, err := mb.GetMessages() + messages, err := ctx.DataStore.GetMessages(name) if err != nil { // This doesn't indicate empty, likely an IO error return fmt.Errorf("Failed to get messages for %v: %v", name, err) @@ -59,12 +54,7 @@ func MailboxShowV1(w http.ResponseWriter, req *http.Request, ctx *web.Context) ( if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - msg, err := mb.GetMessage(id) + msg, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil @@ -121,13 +111,8 @@ func MailboxPurgeV1(w http.ResponseWriter, req *http.Request, ctx *web.Context) if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } // Delete all messages - err = mb.Purge() + err = ctx.DataStore.PurgeMessages(name) if err != nil { return fmt.Errorf("Mailbox(%q) purge failed: %v", name, err) } @@ -144,12 +129,7 @@ func MailboxSourceV1(w http.ResponseWriter, req *http.Request, ctx *web.Context) if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - message, err := mb.GetMessage(id) + message, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil @@ -178,12 +158,7 @@ func MailboxDeleteV1(w http.ResponseWriter, req *http.Request, ctx *web.Context) if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - message, err := mb.GetMessage(id) + message, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil diff --git a/pkg/rest/apiv1_controller_test.go b/pkg/rest/apiv1_controller_test.go index 9c4e789..a48fd38 100644 --- a/pkg/rest/apiv1_controller_test.go +++ b/pkg/rest/apiv1_controller_test.go @@ -2,14 +2,13 @@ package rest import ( "encoding/json" - "fmt" "io" "net/mail" "os" "testing" "time" - "github.com/jhillyerd/inbucket/pkg/storage" + "github.com/jhillyerd/inbucket/pkg/test" ) const ( @@ -31,7 +30,7 @@ const ( func TestRestMailboxList(t *testing.T) { // Setup - ds := &storage.MockDataStore{} + ds := test.NewStore() logbuf := setupWebServer(ds) // Test invalid mailbox name @@ -45,10 +44,6 @@ func TestRestMailboxList(t *testing.T) { } // Test empty mailbox - emptybox := &storage.MockMailbox{} - ds.On("MailboxFor", "empty").Return(emptybox, nil) - emptybox.On("GetMessages").Return([]storage.Message{}, nil) - w, err = testRestGet(baseURL + "/mailbox/empty") expectCode = 200 if err != nil { @@ -58,30 +53,8 @@ func TestRestMailboxList(t *testing.T) { t.Errorf("Expected code %v, got %v", expectCode, w.Code) } - // Test MailboxFor error - ds.On("MailboxFor", "error").Return(&storage.MockMailbox{}, fmt.Errorf("Internal error")) - w, err = testRestGet(baseURL + "/mailbox/error") - expectCode = 500 - if err != nil { - t.Fatal(err) - } - if w.Code != expectCode { - t.Errorf("Expected code %v, got %v", expectCode, w.Code) - } - - if t.Failed() { - // Wait for handler to finish logging - time.Sleep(2 * time.Second) - // Dump buffered log data if there was a failure - _, _ = io.Copy(os.Stderr, logbuf) - } - - // Test MailboxFor error - error2box := &storage.MockMailbox{} - ds.On("MailboxFor", "error2").Return(error2box, nil) - error2box.On("GetMessages").Return([]storage.Message{}, fmt.Errorf("Internal error 2")) - - w, err = testRestGet(baseURL + "/mailbox/error2") + // Test Mailbox error + w, err = testRestGet(baseURL + "/mailbox/messageserr") expectCode = 500 if err != nil { t.Fatal(err) @@ -107,11 +80,10 @@ func TestRestMailboxList(t *testing.T) { Subject: "subject 2", Date: time.Date(2012, 7, 1, 10, 11, 12, 253, time.FixedZone("PDT", -700)), } - goodbox := &storage.MockMailbox{} - ds.On("MailboxFor", "good").Return(goodbox, nil) msg1 := data1.MockMessage() msg2 := data2.MockMessage() - goodbox.On("GetMessages").Return([]storage.Message{msg1, msg2}, nil) + ds.AddMessage("good", msg1) + ds.AddMessage("good", msg2) // Check return code w, err = testRestGet(baseURL + "/mailbox/good") @@ -130,7 +102,7 @@ func TestRestMailboxList(t *testing.T) { t.Errorf("Failed to decode JSON: %v", err) } if len(result) != 2 { - t.Errorf("Expected 2 results, got %v", len(result)) + t.Fatalf("Expected 2 results, got %v", len(result)) } if errors := data1.CompareToJSONHeaderMap(result[0]); len(errors) > 0 { t.Logf("%v", result[0]) @@ -155,7 +127,7 @@ func TestRestMailboxList(t *testing.T) { func TestRestMessage(t *testing.T) { // Setup - ds := &storage.MockDataStore{} + ds := test.NewStore() logbuf := setupWebServer(ds) // Test invalid mailbox name @@ -169,10 +141,6 @@ func TestRestMessage(t *testing.T) { } // Test requesting a message that does not exist - emptybox := &storage.MockMailbox{} - ds.On("MailboxFor", "empty").Return(emptybox, nil) - emptybox.On("GetMessage", "0001").Return(&storage.MockMessage{}, storage.ErrNotExist) - w, err = testRestGet(baseURL + "/mailbox/empty/0001") expectCode = 404 if err != nil { @@ -182,9 +150,8 @@ func TestRestMessage(t *testing.T) { t.Errorf("Expected code %v, got %v", expectCode, w.Code) } - // Test MailboxFor error - ds.On("MailboxFor", "error").Return(&storage.MockMailbox{}, fmt.Errorf("Internal error")) - w, err = testRestGet(baseURL + "/mailbox/error/0001") + // Test GetMessage error + w, err = testRestGet(baseURL + "/mailbox/messageerr/0001") expectCode = 500 if err != nil { t.Fatal(err) @@ -200,20 +167,6 @@ func TestRestMessage(t *testing.T) { _, _ = io.Copy(os.Stderr, logbuf) } - // Test GetMessage error - error2box := &storage.MockMailbox{} - ds.On("MailboxFor", "error2").Return(error2box, nil) - error2box.On("GetMessage", "0001").Return(&storage.MockMessage{}, fmt.Errorf("Internal error 2")) - - w, err = testRestGet(baseURL + "/mailbox/error2/0001") - expectCode = 500 - if err != nil { - t.Fatal(err) - } - if w.Code != expectCode { - t.Errorf("Expected code %v, got %v", expectCode, w.Code) - } - // Test JSON message headers data1 := &InputMessageData{ Mailbox: "good", @@ -228,10 +181,8 @@ func TestRestMessage(t *testing.T) { Text: "This is some text", HTML: "This is some HTML", } - goodbox := &storage.MockMailbox{} - ds.On("MailboxFor", "good").Return(goodbox, nil) msg1 := data1.MockMessage() - goodbox.On("GetMessage", "0001").Return(msg1, nil) + ds.AddMessage("good", msg1) // Check return code w, err = testRestGet(baseURL + "/mailbox/good/0001") diff --git a/pkg/server/pop3/handler.go b/pkg/server/pop3/handler.go index 98cce65..4f6c4b6 100644 --- a/pkg/server/pop3/handler.go +++ b/pkg/server/pop3/handler.go @@ -513,12 +513,11 @@ func (ses *Session) sendMessageTop(msg storage.Message, lineCount int) { // Load the users mailbox func (ses *Session) loadMailbox() { - var err error - ses.messages, err = ses.mailbox.GetMessages() + m, err := ses.server.dataStore.GetMessages(ses.user) if err != nil { - ses.logError("Failed to load messages for %v", ses.user) + ses.logError("Failed to load messages for %v: %v", ses.user, err) } - + ses.messages = m ses.retainAll() } diff --git a/pkg/server/smtp/handler.go b/pkg/server/smtp/handler.go index a678d3a..3b0660d 100644 --- a/pkg/server/smtp/handler.go +++ b/pkg/server/smtp/handler.go @@ -478,10 +478,15 @@ func (ss *Session) deliverMessage(r recipientDetails, msgBuf [][]byte) (ok bool) ss.logError("Error while closing message for %v: %v", r.mailbox, err) return false } + name, err := stringutil.ParseMailboxName(r.localPart) + if err != nil { + // This parse already succeeded when MailboxFor was called, shouldn't fail here. + return false + } // Broadcast message information broadcast := msghub.Message{ - Mailbox: r.mailbox.Name(), + Mailbox: name, ID: msg.ID(), From: msg.From(), To: msg.To(), diff --git a/pkg/storage/file/fstore.go b/pkg/storage/file/fstore.go index 5588cbb..0a03873 100644 --- a/pkg/storage/file/fstore.go +++ b/pkg/storage/file/fstore.go @@ -81,9 +81,36 @@ func DefaultStore() storage.Store { return New(cfg) } +// GetMessage returns the messages in the named mailbox, or an error. +func (fs *Store) GetMessage(mailbox, id string) (storage.Message, error) { + mb, err := fs.MailboxFor(mailbox) + if err != nil { + return nil, err + } + return mb.(*Mailbox).GetMessage(id) +} + +// GetMessages returns the messages in the named mailbox, or an error. +func (fs *Store) GetMessages(mailbox string) ([]storage.Message, error) { + mb, err := fs.MailboxFor(mailbox) + if err != nil { + return nil, err + } + return mb.(*Mailbox).GetMessages() +} + +// PurgeMessages deletes all messages in the named mailbox, or returns an error. +func (fs *Store) PurgeMessages(name string) error { + mb, err := fs.MailboxFor(name) + if err != nil { + return err + } + return mb.(*Mailbox).Purge() +} + // MailboxFor retrieves the Mailbox object for a specified email address, if the mailbox // does not exist, it will attempt to create it. -func (ds *Store) MailboxFor(emailAddress string) (storage.Mailbox, error) { +func (fs *Store) MailboxFor(emailAddress string) (storage.Mailbox, error) { name, err := stringutil.ParseMailboxName(emailAddress) if err != nil { return nil, err @@ -91,17 +118,17 @@ func (ds *Store) MailboxFor(emailAddress string) (storage.Mailbox, error) { dir := stringutil.HashMailboxName(name) s1 := dir[0:3] s2 := dir[0:6] - path := filepath.Join(ds.mailPath, s1, s2, dir) + path := filepath.Join(fs.mailPath, s1, s2, dir) indexPath := filepath.Join(path, indexFileName) - return &Mailbox{store: ds, name: name, dirName: dir, path: path, + return &Mailbox{store: fs, name: name, dirName: dir, path: path, indexPath: indexPath}, nil } // AllMailboxes returns a slice with all Mailboxes -func (ds *Store) AllMailboxes() ([]storage.Mailbox, error) { +func (fs *Store) AllMailboxes() ([]storage.Mailbox, error) { mailboxes := make([]storage.Mailbox, 0, 100) - infos1, err := ioutil.ReadDir(ds.mailPath) + infos1, err := ioutil.ReadDir(fs.mailPath) if err != nil { return nil, err } @@ -109,7 +136,7 @@ func (ds *Store) AllMailboxes() ([]storage.Mailbox, error) { for _, inf1 := range infos1 { if inf1.IsDir() { l1 := inf1.Name() - infos2, err := ioutil.ReadDir(filepath.Join(ds.mailPath, l1)) + infos2, err := ioutil.ReadDir(filepath.Join(fs.mailPath, l1)) if err != nil { return nil, err } @@ -117,7 +144,7 @@ func (ds *Store) AllMailboxes() ([]storage.Mailbox, error) { for _, inf2 := range infos2 { if inf2.IsDir() { l2 := inf2.Name() - infos3, err := ioutil.ReadDir(filepath.Join(ds.mailPath, l1, l2)) + infos3, err := ioutil.ReadDir(filepath.Join(fs.mailPath, l1, l2)) if err != nil { return nil, err } @@ -125,9 +152,9 @@ func (ds *Store) AllMailboxes() ([]storage.Mailbox, error) { for _, inf3 := range infos3 { if inf3.IsDir() { mbdir := inf3.Name() - mbpath := filepath.Join(ds.mailPath, l1, l2, mbdir) + mbpath := filepath.Join(fs.mailPath, l1, l2, mbdir) idx := filepath.Join(mbpath, indexFileName) - mb := &Mailbox{store: ds, dirName: mbdir, path: mbpath, + mb := &Mailbox{store: fs, dirName: mbdir, path: mbpath, indexPath: idx} mailboxes = append(mailboxes, mb) } @@ -141,13 +168,13 @@ func (ds *Store) AllMailboxes() ([]storage.Mailbox, error) { } // LockFor returns the RWMutex for this mailbox, or an error. -func (ds *Store) LockFor(emailAddress string) (*sync.RWMutex, error) { +func (fs *Store) LockFor(emailAddress string) (*sync.RWMutex, error) { name, err := stringutil.ParseMailboxName(emailAddress) if err != nil { return nil, err } hash := stringutil.HashMailboxName(name) - return ds.hashLock.Get(hash), nil + return fs.hashLock.Get(hash), nil } // Mailbox implements Mailbox, manages the mail for a specific user and diff --git a/pkg/storage/file/fstore_test.go b/pkg/storage/file/fstore_test.go index b02bdd1..26671c4 100644 --- a/pkg/storage/file/fstore_test.go +++ b/pkg/storage/file/fstore_test.go @@ -62,9 +62,7 @@ func TestFSDirStructure(t *testing.T) { assert.True(t, isFile(expect), "Expected %q to be a file", expect) // Delete message - mb, err := ds.MailboxFor(mbName) - assert.Nil(t, err) - msg, err := mb.GetMessage(id1) + msg, err := ds.GetMessage(mbName, id1) assert.Nil(t, err) err = msg.Delete() assert.Nil(t, err) @@ -76,7 +74,7 @@ func TestFSDirStructure(t *testing.T) { assert.True(t, isFile(expect), "Expected %q to be a file", expect) // Delete message - msg, err = mb.GetMessage(id2) + msg, err = ds.GetMessage(mbName, id2) assert.Nil(t, err) err = msg.Delete() assert.Nil(t, err) @@ -137,11 +135,7 @@ func TestFSDeliverMany(t *testing.T) { for i, subj := range subjects { // Check number of messages - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err := mb.GetMessages() + msgs, err := ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -151,11 +145,7 @@ func TestFSDeliverMany(t *testing.T) { deliverMessage(ds, mbName, subj, time.Now()) } - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err := mb.GetMessages() + msgs, err := ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -189,11 +179,7 @@ func TestFSDelete(t *testing.T) { deliverMessage(ds, mbName, subj, time.Now()) } - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err := mb.GetMessages() + msgs, err := ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -205,11 +191,7 @@ func TestFSDelete(t *testing.T) { _ = msgs[3].Delete() // Confirm deletion - mb, err = ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err = mb.GetMessages() + msgs, err = ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -225,11 +207,7 @@ func TestFSDelete(t *testing.T) { // Try appending one more deliverMessage(ds, mbName, "foxtrot", time.Now()) - mb, err = ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err = mb.GetMessages() + msgs, err = ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -263,11 +241,7 @@ func TestFSPurge(t *testing.T) { deliverMessage(ds, mbName, subj, time.Now()) } - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err := mb.GetMessages() + msgs, err := ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -275,15 +249,11 @@ func TestFSPurge(t *testing.T) { len(subjects), len(msgs)) // Purge mailbox - err = mb.Purge() + err = ds.PurgeMessages(mbName) assert.Nil(t, err) // Confirm deletion - mb, err = ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err = mb.GetMessages() + msgs, err = ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -315,12 +285,8 @@ func TestFSSize(t *testing.T) { sentSizes[i] = size } - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } for i, id := range sentIds { - msg, err := mb.GetMessage(id) + msg, err := ds.GetMessage(mbName, id) assert.Nil(t, err) expect := sentSizes[i] @@ -351,17 +317,12 @@ func TestFSMissing(t *testing.T) { sentIds[i] = id } - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - // Delete a message file without removing it from index - msg, err := mb.GetMessage(sentIds[1]) + msg, err := ds.GetMessage(mbName, sentIds[1]) assert.Nil(t, err) fmsg := msg.(*Message) _ = os.Remove(fmsg.rawPath()) - msg, err = mb.GetMessage(sentIds[1]) + msg, err = ds.GetMessage(mbName, sentIds[1]) assert.Nil(t, err) // Try to read parts of message @@ -392,11 +353,7 @@ func TestFSMessageCap(t *testing.T) { t.Logf("Delivered %q", subj) // Check number of messages - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err := mb.GetMessages() + msgs, err := ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -437,11 +394,7 @@ func TestFSNoMessageCap(t *testing.T) { t.Logf("Delivered %q", subj) // Check number of messages - mb, err := ds.MailboxFor(mbName) - if err != nil { - t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err) - } - msgs, err := mb.GetMessages() + msgs, err := ds.GetMessages(mbName) if err != nil { t.Fatalf("Failed to GetMessages for %q: %v", mbName, err) } @@ -467,9 +420,7 @@ func TestGetLatestMessage(t *testing.T) { mbName := "james" // Test empty mailbox - mb, err := ds.MailboxFor(mbName) - assert.Nil(t, err) - msg, err := mb.GetMessage("latest") + msg, err := ds.GetMessage(mbName, "latest") assert.Nil(t, msg) assert.Error(t, err) @@ -480,23 +431,19 @@ func TestGetLatestMessage(t *testing.T) { id2, _ := deliverMessage(ds, mbName, "test 2", time.Now()) // Test get the latest message - mb, err = ds.MailboxFor(mbName) - assert.Nil(t, err) - msg, err = mb.GetMessage("latest") + msg, err = ds.GetMessage(mbName, "latest") assert.Nil(t, err) assert.True(t, msg.ID() == id2, "Expected %q to be equal to %q", msg.ID(), id2) // Deliver test message 3 id3, _ := deliverMessage(ds, mbName, "test 3", time.Now()) - mb, err = ds.MailboxFor(mbName) - assert.Nil(t, err) - msg, err = mb.GetMessage("latest") + msg, err = ds.GetMessage(mbName, "latest") assert.Nil(t, err) assert.True(t, msg.ID() == id3, "Expected %q to be equal to %q", msg.ID(), id3) // Test wrong id - _, err = mb.GetMessage("wrongid") + _, err = ds.GetMessage(mbName, "wrongid") assert.Error(t, err) if t.Failed() { diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index a137f9b..51620ed 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -21,6 +21,9 @@ var ( // Store is an interface to get Mailboxes stored in Inbucket type Store interface { + GetMessage(mailbox string, id string) (Message, error) + GetMessages(mailbox string) ([]Message, error) + PurgeMessages(mailbox string) error MailboxFor(emailAddress string) (Mailbox, error) AllMailboxes() ([]Mailbox, error) // LockFor is a temporary hack to fix #77 until Datastore revamp @@ -30,10 +33,7 @@ type Store interface { // Mailbox is an interface to get and manipulate messages in a DataStore type Mailbox interface { GetMessages() ([]Message, error) - GetMessage(id string) (Message, error) - Purge() error NewMessage() (Message, error) - Name() string String() string } diff --git a/pkg/storage/testing.go b/pkg/storage/testing.go index fc8dbab..8c51b3f 100644 --- a/pkg/storage/testing.go +++ b/pkg/storage/testing.go @@ -15,6 +15,24 @@ type MockDataStore struct { mock.Mock } +// GetMessage mock function +func (m *MockDataStore) GetMessage(name, id string) (Message, error) { + args := m.Called(name, id) + return args.Get(0).(Message), args.Error(1) +} + +// GetMessages mock function +func (m *MockDataStore) GetMessages(name string) ([]Message, error) { + args := m.Called(name) + return args.Get(0).([]Message), args.Error(1) +} + +// PurgeMessages mock function +func (m *MockDataStore) PurgeMessages(name string) error { + args := m.Called(name) + return args.Error(0) +} + // MailboxFor mock function func (m *MockDataStore) MailboxFor(name string) (Mailbox, error) { args := m.Called(name) @@ -61,12 +79,6 @@ func (m *MockMailbox) NewMessage() (Message, error) { return args.Get(0).(Message), args.Error(1) } -// Name mock function -func (m *MockMailbox) Name() string { - args := m.Called() - return args.String(0) -} - // String mock function func (m *MockMailbox) String() string { args := m.Called() diff --git a/pkg/test/storage.go b/pkg/test/storage.go new file mode 100644 index 0000000..29c24ce --- /dev/null +++ b/pkg/test/storage.go @@ -0,0 +1,47 @@ +package test + +import ( + "errors" + + "github.com/jhillyerd/inbucket/pkg/storage" +) + +// StoreStub stubs storage.Store for testing. +type StoreStub struct { + storage.Store + mailboxes map[string][]storage.Message +} + +// NewStore creates a new StoreStub. +func NewStore() *StoreStub { + return &StoreStub{ + mailboxes: make(map[string][]storage.Message), + } +} + +// AddMessage adds a message to the specified mailbox. +func (s *StoreStub) AddMessage(mailbox string, m storage.Message) { + msgs := s.mailboxes[mailbox] + s.mailboxes[mailbox] = append(msgs, m) +} + +// GetMessage gets a message by ID from the specified mailbox. +func (s *StoreStub) GetMessage(mailbox, id string) (storage.Message, error) { + if mailbox == "messageerr" { + return nil, errors.New("internal error") + } + for _, m := range s.mailboxes[mailbox] { + if m.ID() == id { + return m, nil + } + } + return nil, storage.ErrNotExist +} + +// GetMessages gets all the messages for the specified mailbox. +func (s *StoreStub) GetMessages(mailbox string) ([]storage.Message, error) { + if mailbox == "messageserr" { + return nil, errors.New("internal error") + } + return s.mailboxes[mailbox], nil +} diff --git a/pkg/webui/mailbox_controller.go b/pkg/webui/mailbox_controller.go index f9684b9..69b3a3f 100644 --- a/pkg/webui/mailbox_controller.go +++ b/pkg/webui/mailbox_controller.go @@ -72,12 +72,7 @@ func MailboxList(w http.ResponseWriter, req *http.Request, ctx *web.Context) (er if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - messages, err := mb.GetMessages() + messages, err := ctx.DataStore.GetMessages(name) if err != nil { // This doesn't indicate empty, likely an IO error return fmt.Errorf("Failed to get messages for %v: %v", name, err) @@ -99,12 +94,7 @@ func MailboxShow(w http.ResponseWriter, req *http.Request, ctx *web.Context) (er if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - msg, err := mb.GetMessage(id) + msg, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil @@ -148,12 +138,7 @@ func MailboxHTML(w http.ResponseWriter, req *http.Request, ctx *web.Context) (er if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - message, err := mb.GetMessage(id) + message, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil @@ -172,8 +157,7 @@ func MailboxHTML(w http.ResponseWriter, req *http.Request, ctx *web.Context) (er "ctx": ctx, "name": name, "message": message, - // TODO It is not really safe to render, need to sanitize, issue #5 - "body": template.HTML(mime.HTML), + "body": template.HTML(mime.HTML), }) } @@ -185,12 +169,7 @@ func MailboxSource(w http.ResponseWriter, req *http.Request, ctx *web.Context) ( if err != nil { return err } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - message, err := mb.GetMessage(id) + message, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil @@ -231,12 +210,7 @@ func MailboxDownloadAttach(w http.ResponseWriter, req *http.Request, ctx *web.Co http.Redirect(w, req, web.Reverse("RootIndex"), http.StatusSeeOther) return nil } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - message, err := mb.GetMessage(id) + message, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil @@ -284,12 +258,7 @@ func MailboxViewAttach(w http.ResponseWriter, req *http.Request, ctx *web.Contex http.Redirect(w, req, web.Reverse("RootIndex"), http.StatusSeeOther) return nil } - mb, err := ctx.DataStore.MailboxFor(name) - if err != nil { - // This doesn't indicate not found, likely an IO error - return fmt.Errorf("Failed to get mailbox for %q: %v", name, err) - } - message, err := mb.GetMessage(id) + message, err := ctx.DataStore.GetMessage(name, id) if err == storage.ErrNotExist { http.NotFound(w, req) return nil