Finishing up index.gob work

- Wrote more unit tests to make sure filestore behaves as expected
 - Renamed NewFileDataStore to DefaultFileDataStore, implemented a new
   NewFileDataStore for unit tests.
 - filestore now removes entire mailbox dir when last message is deleted

.goxc.json

@@ -6,7 +6,7 @@
 	"Resources": {
 		"Include": "README*,LICENSE*,bin,etc,themes"
 	},
-	"PackageVersion": "20131010",
+	"PackageVersion": "20131013",
 	"PrereleaseInfo": "snapshot",
 	"FormatVersion": "0.8"
 }

pop3d/listener.go

@@ -23,7 +23,7 @@ type Server struct {
 // Init a new Server object
 func New() *Server {
 	// TODO is two filestores better/worse than sharing w/ smtpd?
-	ds := smtpd.NewFileDataStore()
+	ds := smtpd.DefaultFileDataStore()
 	cfg := config.GetPop3Config()
 	return &Server{domain: cfg.Domain, dataStore: ds, maxIdleSeconds: cfg.MaxIdleSeconds,
 		waitgroup: new(sync.WaitGroup)}

smtpd/filestore.go

@@ -48,9 +48,19 @@ type FileDataStore struct {
 	mailPath string
 }
 
-// NewDataStore creates a new DataStore object.  It uses the inbucket.Config object to
+// NewFileDataStore creates a new DataStore object using the specified path
+func NewFileDataStore(path string) DataStore {
+	mailPath := filepath.Join(path, "mail")
+	if _, err := os.Stat(mailPath); err != nil {
+		// Mail datastore does not yet exist
+		os.MkdirAll(mailPath, 0770)
+	}
+	return &FileDataStore{path: path, mailPath: mailPath}
+}
+
+// DefaultFileDataStore creates a new DataStore object.  It uses the inbucket.Config object to
 // construct it's path.
-func NewFileDataStore() DataStore {
+func DefaultFileDataStore() DataStore {
 	path, err := config.Config.String("datastore", "path")
 	if err != nil {
 		log.LogError("Error getting datastore path: %v", err)
@@ -60,12 +70,7 @@ func NewFileDataStore() DataStore {
 		log.LogError("No value configured for datastore path")
 		return nil
 	}
-	mailPath := filepath.Join(path, "mail")
+	return NewFileDataStore(path)
-	if _, err := os.Stat(mailPath); err != nil {
-		// Mail datastore does not yet exist
-		os.MkdirAll(mailPath, 0770)
-	}
-	return &FileDataStore{path: path, mailPath: mailPath}
 }
 
 // Retrieves the Mailbox object for a specified email address, if the mailbox
@@ -230,27 +235,33 @@ func (mb *FileMailbox) writeIndex() error {
 	// Lock for writing
 	indexLock.Lock()
 	defer indexLock.Unlock()
-	// Ensure mailbox directory exists
+	if len(mb.messages) > 0 {
-	if err := mb.createDir(); err != nil {
+		// Ensure mailbox directory exists
-		return err
+		if err := mb.createDir(); err != nil {
-	}
+			return err
-	// Open index for writing
+		}
-	file, err := os.Create(mb.indexPath)
+		// Open index for writing
-	if err != nil {
+		file, err := os.Create(mb.indexPath)
-		return err
-	}
-	defer file.Close()
-	writer := bufio.NewWriter(file)
-
-	// Write each message and then flush
-	enc := gob.NewEncoder(writer)
-	for _, m := range mb.messages {
-		err = enc.Encode(m)
 		if err != nil {
 			return err
 		}
+		defer file.Close()
+		writer := bufio.NewWriter(file)
+
+		// Write each message and then flush
+		enc := gob.NewEncoder(writer)
+		for _, m := range mb.messages {
+			err = enc.Encode(m)
+			if err != nil {
+				return err
+			}
+		}
+		writer.Flush()
+	} else {
+		// No messages, delete index+maildir
+		log.LogTrace("Removing mailbox %v", mb.path)
+		return os.RemoveAll(mb.path)
 	}
-	writer.Flush()
 
 	return nil
 }
@@ -444,6 +455,13 @@ func (m *FileMessage) Delete() error {
 	}
 	m.mailbox.writeIndex()
 
+	if len(m.mailbox.messages) == 0 {
+		// This was the last message, writeIndex() has removed the entire
+		// directory
+		return nil
+	}
+
+	// There are still messages in the index
 	log.LogTrace("Deleting %v", m.rawPath())
 	return os.Remove(m.rawPath())
 }

smtpd/filestore_test.go

@@ -10,6 +10,84 @@ import (
 	"time"
 )
 
+// Test directory structure created by filestore
+func TestFSDirStructure(t *testing.T) {
+	ds := setupDataStore()
+	defer teardownDataStore(ds)
+	root := ds.path
+
+	// james hashes to 474ba67bdb289c6263b36dfd8a7bed6c85b04943
+	mbName := "james"
+
+	// Check filestore root exists
+	assert.True(t, isDir(root), "Expected %q to be a directory", root)
+
+	// Check mail dir exists
+	expect := filepath.Join(root, "mail")
+	assert.True(t, isDir(expect), "Expected %q to be a directory", expect)
+
+	// Check first hash section does not exist
+	expect = filepath.Join(root, "mail", "474")
+	assert.False(t, isDir(expect), "Expected %q to not exist", expect)
+
+	// Deliver test message
+	id1, _ := deliverMessage(ds, mbName, "test", time.Now())
+
+	// Check path to message exists
+	assert.True(t, isDir(expect), "Expected %q to be a directory", expect)
+	expect = filepath.Join(expect, "474ba6")
+	assert.True(t, isDir(expect), "Expected %q to be a directory", expect)
+	expect = filepath.Join(expect, "474ba67bdb289c6263b36dfd8a7bed6c85b04943")
+	assert.True(t, isDir(expect), "Expected %q to be a directory", expect)
+
+	// Check files
+	mbPath := expect
+	expect = filepath.Join(mbPath, "index.gob")
+	assert.True(t, isFile(expect), "Expected %q to be a file", expect)
+	expect = filepath.Join(mbPath, id1 + ".raw")
+	assert.True(t, isFile(expect), "Expected %q to be a file", expect)
+
+	// Deliver second test message
+	id2, _ := deliverMessage(ds, mbName, "test 2", time.Now())
+
+	// Check files
+	expect = filepath.Join(mbPath, "index.gob")
+	assert.True(t, isFile(expect), "Expected %q to be a file", expect)
+	expect = filepath.Join(mbPath, id2 + ".raw")
+	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)
+	assert.Nil(t, err)
+	err = msg.Delete()
+	assert.Nil(t, err)
+
+	// Message should be removed
+	expect = filepath.Join(mbPath, id1 + ".raw")
+	assert.False(t, isPresent(expect), "Did not expect %q to exist", expect)
+	expect = filepath.Join(mbPath, "index.gob")
+	assert.True(t, isFile(expect), "Expected %q to be a file", expect)
+
+	// Delete message
+	msg, err = mb.GetMessage(id2)
+	assert.Nil(t, err)
+	err = msg.Delete()
+	assert.Nil(t, err)
+
+	// Message should be removed
+	expect = filepath.Join(mbPath, id2 + ".raw")
+	assert.False(t, isPresent(expect), "Did not expect %q to exist", expect)
+
+	// No messages, index & maildir should be removed
+	expect = filepath.Join(mbPath, "index.gob")
+	assert.False(t, isPresent(expect), "Did not expect %q to exist", expect)
+	expect = mbPath
+	assert.False(t, isPresent(expect), "Did not expect %q to exist", expect)
+}
+
+
 // Test FileDataStore.AllMailboxes()
 func TestFSAllMailboxes(t *testing.T) {
 	ds := setupDataStore()
@@ -147,13 +225,12 @@ func setupDataStore() *FileDataStore {
 	if err != nil {
 		panic(err)
 	}
-	mailPath := filepath.Join(path, "mail")
+	return NewFileDataStore(path).(*FileDataStore)
-	return &FileDataStore{path: path, mailPath: mailPath}
 }
 
 // deliverMessage creates and delivers a message to the specific mailbox, returning
 // the size of the generated message.
-func deliverMessage(ds *FileDataStore, mbName string, subject string, date time.Time) int {
+func deliverMessage(ds *FileDataStore, mbName string, subject string, date time.Time) (id string, size int) {
 	// Build fake SMTP message for delivery
 	testMsg := make([]byte, 0, 300)
 	testMsg = append(testMsg, []byte("To: somebody@host\r\n")...)
@@ -166,19 +243,20 @@ func deliverMessage(ds *FileDataStore, mbName string, subject string, date time.
 	if err != nil {
 		panic(err)
 	}
-	// Create day old message
+	// Create message object
+	id = generateId(date)
 	msg := &FileMessage{
 		mailbox:  mb.(*FileMailbox),
 		writable: true,
 		Fdate:    date,
-		Fid:      generateId(date),
+		Fid:      id,
 	}
 	msg.Append(testMsg)
 	if err = msg.Close(); err != nil {
 		panic(err)
 	}
 
-	return len(testMsg)
+	return id, len(testMsg)
 }
 
 func teardownDataStore(ds *FileDataStore) {
@@ -186,3 +264,23 @@ func teardownDataStore(ds *FileDataStore) {
 		panic(err)
 	}
 }
+
+func isPresent(path string) bool {
+	_, err := os.Lstat(path)
+	return err == nil
+}
+
+func isFile(path string) bool {
+	if fi, err := os.Lstat(path); err == nil {
+		return !fi.IsDir()
+	}
+	return false
+}
+
+func isDir(path string) bool {
+	if fi, err := os.Lstat(path); err == nil {
+		return fi.IsDir()
+	}
+	return false
+}
+

smtpd/listener.go

@@ -47,7 +47,7 @@ var expWarnsHist = new(expvar.String)
 
 // Init a new Server object
 func New() *Server {
-	ds := NewFileDataStore()
+	ds := DefaultFileDataStore()
 	cfg := config.GetSmtpConfig()
 	return &Server{dataStore: ds, domain: cfg.Domain, maxRecips: cfg.MaxRecipients,
 		maxIdleSeconds: cfg.MaxIdleSeconds, maxMessageBytes: cfg.MaxMessageBytes,

web/context.go

@@ -39,7 +39,7 @@ func headerMatch(req *http.Request, name string, value string) bool {
 func NewContext(req *http.Request) (*Context, error) {
 	vars := mux.Vars(req)
 	sess, err := sessionStore.Get(req, "inbucket")
-	ds := smtpd.NewFileDataStore()
+	ds := smtpd.DefaultFileDataStore()
 	ctx := &Context{
 		Vars:      vars,
 		Session:   sess,