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

@@ -6,6 +6,7 @@ env:
 
 before_script:
   - go get github.com/golang/lint/golint
+  - make deps
 
 go:
   - "1.10"

Makefile

@@ -19,9 +19,9 @@ clean:
 deps:
 	go get -t ./...
 
-build: deps $(commands)
+build: $(commands)
 
-test: deps
+test:
 	go test -race ./...
 
 fmt:

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)
+	messages, err := ctx.DataStore.GetMessages(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()
 	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)
+	msg, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	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)
+	message, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	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)
+	message, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	if err == storage.ErrNotExist {
 		http.NotFound(w, req)
 		return nil

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
+	// Test Mailbox error
-	ds.On("MailboxFor", "error").Return(&storage.MockMailbox{}, fmt.Errorf("Internal error"))
+	w, err = testRestGet(baseURL + "/mailbox/messageserr")
-	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")
 	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
+	// Test GetMessage error
-	ds.On("MailboxFor", "error").Return(&storage.MockMailbox{}, fmt.Errorf("Internal error"))
+	w, err = testRestGet(baseURL + "/mailbox/messageerr/0001")
-	w, err = testRestGet(baseURL + "/mailbox/error/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")

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
+	m, err := ses.server.dataStore.GetMessages(ses.user)
-	ses.messages, err = ses.mailbox.GetMessages()
 	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()
 }
 

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(),

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

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)
+	msg, err := ds.GetMessage(mbName, id1)
-	assert.Nil(t, err)
-	msg, err := mb.GetMessage(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)
+		msgs, err := ds.GetMessages(mbName)
-		if err != nil {
-			t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-		}
-		msgs, err := mb.GetMessages()
 		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)
+	msgs, err := ds.GetMessages(mbName)
-	if err != nil {
-		t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-	}
-	msgs, err := mb.GetMessages()
 	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)
+	msgs, err := ds.GetMessages(mbName)
-	if err != nil {
-		t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-	}
-	msgs, err := mb.GetMessages()
 	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)
+	msgs, err = ds.GetMessages(mbName)
-	if err != nil {
-		t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-	}
-	msgs, err = mb.GetMessages()
 	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)
+	msgs, err = ds.GetMessages(mbName)
-	if err != nil {
-		t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-	}
-	msgs, err = mb.GetMessages()
 	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)
+	msgs, err := ds.GetMessages(mbName)
-	if err != nil {
-		t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-	}
-	msgs, err := mb.GetMessages()
 	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)
+	msgs, err = ds.GetMessages(mbName)
-	if err != nil {
-		t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-	}
-	msgs, err = mb.GetMessages()
 	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)
+		msgs, err := ds.GetMessages(mbName)
-		if err != nil {
-			t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-		}
-		msgs, err := mb.GetMessages()
 		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)
+		msgs, err := ds.GetMessages(mbName)
-		if err != nil {
-			t.Fatalf("Failed to MailboxFor(%q): %v", mbName, err)
-		}
-		msgs, err := mb.GetMessages()
 		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)
+	msg, err := ds.GetMessage(mbName, "latest")
-	assert.Nil(t, err)
-	msg, err := mb.GetMessage("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)
+	msg, err = ds.GetMessage(mbName, "latest")
-	assert.Nil(t, err)
-	msg, err = mb.GetMessage("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)
+	msg, err = ds.GetMessage(mbName, "latest")
-	assert.Nil(t, err)
-	msg, err = mb.GetMessage("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() {

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
 }
 

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()

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
+}

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)
+	messages, err := ctx.DataStore.GetMessages(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()
 	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)
+	msg, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	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)
+	message, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	if err == storage.ErrNotExist {
 		http.NotFound(w, req)
 		return nil
@@ -172,7 +157,6 @@ 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),
 	})
 }
@@ -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)
+	message, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	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)
+	message, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	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)
+	message, err := ctx.DataStore.GetMessage(name, id)
-	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)
 	if err == storage.ErrNotExist {
 		http.NotFound(w, req)
 		return nil