From 12ad0cb3f0abec74c191e3ab62293792b1062cae Mon Sep 17 00:00:00 2001 From: James Hillyerd Date: Sun, 11 Mar 2018 11:54:35 -0700 Subject: [PATCH] storage: Eliminate storage.Mailbox interface for #69 storage/file Mailbox has been renamed mbox, and is now just an implementation detail. --- pkg/server/pop3/handler.go | 17 ------ pkg/server/smtp/handler.go | 15 ++---- pkg/server/smtp/handler_test.go | 6 --- pkg/storage/file/fmessage.go | 8 ++- pkg/storage/file/fstore.go | 94 ++++++++++++++------------------- pkg/storage/storage.go | 7 --- pkg/storage/testing.go | 41 -------------- 7 files changed, 45 insertions(+), 143 deletions(-) diff --git a/pkg/server/pop3/handler.go b/pkg/server/pop3/handler.go index 4f6c4b6..5e3c665 100644 --- a/pkg/server/pop3/handler.go +++ b/pkg/server/pop3/handler.go @@ -65,7 +65,6 @@ type Session struct { state State // Current session state reader *bufio.Reader // Buffered reader for our net conn user string // Mailbox name - mailbox storage.Mailbox // Mailbox instance messages []storage.Message // Slice of messages in mailbox retain []bool // Messages to retain upon UPDATE (true=retain) msgCount int // Number of undeleted messages @@ -195,14 +194,6 @@ func (ses *Session) authorizationHandler(cmd string, args []string) { if ses.user == "" { ses.ooSeq(cmd) } else { - var err error - ses.mailbox, err = ses.server.dataStore.MailboxFor(ses.user) - if err != nil { - ses.logError("Failed to open mailbox for %v", ses.user) - ses.send(fmt.Sprintf("-ERR Failed to open mailbox for %v", ses.user)) - ses.enterState(QUIT) - return - } ses.loadMailbox() ses.send(fmt.Sprintf("+OK Found %v messages for %v", ses.msgCount, ses.user)) ses.enterState(TRANSACTION) @@ -214,14 +205,6 @@ func (ses *Session) authorizationHandler(cmd string, args []string) { return } ses.user = args[0] - var err error - ses.mailbox, err = ses.server.dataStore.MailboxFor(ses.user) - if err != nil { - ses.logError("Failed to open mailbox for %v", ses.user) - ses.send(fmt.Sprintf("-ERR Failed to open mailbox for %v", ses.user)) - ses.enterState(QUIT) - return - } ses.loadMailbox() ses.send(fmt.Sprintf("+OK Found %v messages for %v", ses.msgCount, ses.user)) ses.enterState(TRANSACTION) diff --git a/pkg/server/smtp/handler.go b/pkg/server/smtp/handler.go index 228718a..50bf9ef 100644 --- a/pkg/server/smtp/handler.go +++ b/pkg/server/smtp/handler.go @@ -14,7 +14,6 @@ import ( "github.com/jhillyerd/inbucket/pkg/log" "github.com/jhillyerd/inbucket/pkg/msghub" - "github.com/jhillyerd/inbucket/pkg/storage" "github.com/jhillyerd/inbucket/pkg/stringutil" ) @@ -73,7 +72,6 @@ var commands = map[string]bool{ // recipientDetails for message delivery type recipientDetails struct { address, localPart, domainPart string - mailbox storage.Mailbox } // Session holds the state of an SMTP session @@ -365,14 +363,7 @@ func (ss *Session) dataHandler() { } if strings.ToLower(domain) != ss.server.domainNoStore { // Not our "no store" domain, so store the message - mb, err := ss.server.dataStore.MailboxFor(local) - if err != nil { - ss.logError("Failed to open mailbox for %q: %s", local, err) - ss.send(fmt.Sprintf("451 Failed to open mailbox for %v", local)) - ss.reset() - return - } - recipients = append(recipients, recipientDetails{recip, local, domain, mb}) + recipients = append(recipients, recipientDetails{recip, local, domain}) } else { log.Tracef("Not storing message for %q", recip) } @@ -469,13 +460,13 @@ func (ss *Session) deliverMessage(r recipientDetails, msgBuf [][]byte) (ok bool) // Append lines from msgBuf for _, line := range msgBuf { if err := msg.Append(line); err != nil { - ss.logError("Failed to append to mailbox %v: %v", r.mailbox, err) + ss.logError("Failed to append to mailbox %v: %v", r.localPart, err) // Should really cleanup the crap on filesystem return false } } if err := msg.Close(); err != nil { - ss.logError("Error while closing message for %v: %v", r.mailbox, err) + ss.logError("Error while closing message for %v: %v", r.localPart, err) return false } name, err := stringutil.ParseMailboxName(r.localPart) diff --git a/pkg/server/smtp/handler_test.go b/pkg/server/smtp/handler_test.go index b8603a5..29098ac 100644 --- a/pkg/server/smtp/handler_test.go +++ b/pkg/server/smtp/handler_test.go @@ -145,11 +145,8 @@ func TestReadyState(t *testing.T) { func TestMailState(t *testing.T) { // Setup mock objects mds := &storage.MockDataStore{} - mb1 := &storage.MockMailbox{} msg1 := &storage.MockMessage{} - mds.On("MailboxFor", "u1").Return(mb1, nil) mds.On("NewMessage", "u1").Return(msg1, nil) - mb1.On("Name").Return("u1") msg1.On("ID").Return("") msg1.On("From").Return("") msg1.On("To").Return(make([]string, 0)) @@ -260,11 +257,8 @@ func TestMailState(t *testing.T) { func TestDataState(t *testing.T) { // Setup mock objects mds := &storage.MockDataStore{} - mb1 := &storage.MockMailbox{} msg1 := &storage.MockMessage{} - mds.On("MailboxFor", "u1").Return(mb1, nil) mds.On("NewMessage", "u1").Return(msg1, nil) - mb1.On("Name").Return("u1") msg1.On("ID").Return("") msg1.On("From").Return("") msg1.On("To").Return(make([]string, 0)) diff --git a/pkg/storage/file/fmessage.go b/pkg/storage/file/fmessage.go index 4e496f8..b325678 100644 --- a/pkg/storage/file/fmessage.go +++ b/pkg/storage/file/fmessage.go @@ -18,7 +18,7 @@ import ( // Message implements Message and contains a little bit of data about a // particular email message, and methods to retrieve the rest of it from disk. type Message struct { - mailbox *Mailbox + mailbox *mbox // Stored in GOB Fid string Fdate time.Time @@ -32,16 +32,15 @@ type Message struct { writer *bufio.Writer } -// NewMessage creates a new FileMessage object and sets the Date and Id fields. +// newMessage creates a new FileMessage object and sets the Date and ID fields. // It will also delete messages over messageCap if configured. -func (mb *Mailbox) NewMessage() (storage.Message, error) { +func (mb *mbox) newMessage() (storage.Message, error) { // Load index if !mb.indexLoaded { if err := mb.readIndex(); err != nil { return nil, err } } - // Delete old messages over messageCap if mb.store.messageCap > 0 { for len(mb.messages) >= mb.store.messageCap { @@ -51,7 +50,6 @@ func (mb *Mailbox) NewMessage() (storage.Message, error) { } } } - date := time.Now() id := generateID(date) return &Message{mailbox: mb, Fid: id, Fdate: date, writable: true}, nil diff --git a/pkg/storage/file/fstore.go b/pkg/storage/file/fstore.go index b3a2e5e..fb09ad0 100644 --- a/pkg/storage/file/fstore.go +++ b/pkg/storage/file/fstore.go @@ -76,46 +76,29 @@ func New(cfg config.DataStoreConfig) storage.Store { // 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) + mb, err := fs.mbox(mailbox) if err != nil { return nil, err } - return mb.(*Mailbox).GetMessage(id) + return mb.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) + mb, err := fs.mbox(mailbox) if err != nil { return nil, err } - return mb.(*Mailbox).GetMessages() + return mb.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) +func (fs *Store) PurgeMessages(mailbox string) error { + mb, err := fs.mbox(mailbox) 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 (fs *Store) MailboxFor(emailAddress string) (storage.Mailbox, error) { - name, err := stringutil.ParseMailboxName(emailAddress) - if err != nil { - return nil, err - } - dir := stringutil.HashMailboxName(name) - s1 := dir[0:3] - s2 := dir[0:6] - path := filepath.Join(fs.mailPath, s1, s2, dir) - indexPath := filepath.Join(path, indexFileName) - - return &Mailbox{store: fs, name: name, dirName: dir, path: path, - indexPath: indexPath}, nil + return mb.purge() } // VisitMailboxes accepts a function that will be called with the messages in each mailbox while it @@ -147,9 +130,9 @@ func (fs *Store) VisitMailboxes(f func([]storage.Message) (cont bool)) error { mbdir := inf3.Name() mbpath := filepath.Join(fs.mailPath, l1, l2, mbdir) idx := filepath.Join(mbpath, indexFileName) - mb := &Mailbox{store: fs, dirName: mbdir, path: mbpath, + mb := &mbox{store: fs, dirName: mbdir, path: mbpath, indexPath: idx} - msgs, err := mb.GetMessages() + msgs, err := mb.getMessages() if err != nil { return err } @@ -177,16 +160,31 @@ func (fs *Store) LockFor(emailAddress string) (*sync.RWMutex, error) { // NewMessage is temproary until #69 MessageData refactor func (fs *Store) NewMessage(mailbox string) (storage.Message, error) { - mb, err := fs.MailboxFor(mailbox) + mb, err := fs.mbox(mailbox) if err != nil { return nil, err } - return mb.(*Mailbox).NewMessage() + return mb.newMessage() } -// Mailbox implements Mailbox, manages the mail for a specific user and -// correlates to a particular directory on disk. -type Mailbox struct { +// mbox returns the named mailbox. +func (fs *Store) mbox(mailbox string) (*mbox, error) { + name, err := stringutil.ParseMailboxName(mailbox) + if err != nil { + return nil, err + } + dir := stringutil.HashMailboxName(name) + s1 := dir[0:3] + s2 := dir[0:6] + path := filepath.Join(fs.mailPath, s1, s2, dir) + indexPath := filepath.Join(path, indexFileName) + + return &mbox{store: fs, name: name, dirName: dir, path: path, + indexPath: indexPath}, nil +} + +// mbox manages the mail for a specific user and correlates to a particular directory on disk. +type mbox struct { store *Store name string dirName string @@ -196,25 +194,14 @@ type Mailbox struct { messages []*Message } -// Name of the mailbox -func (mb *Mailbox) Name() string { - return mb.name -} - -// String renders the name and directory path of the mailbox -func (mb *Mailbox) String() string { - return mb.name + "[" + mb.dirName + "]" -} - -// GetMessages scans the mailbox directory for .gob files and decodes them into +// getMessages scans the mailbox directory for .gob files and decodes them into // a slice of Message objects. -func (mb *Mailbox) GetMessages() ([]storage.Message, error) { +func (mb *mbox) getMessages() ([]storage.Message, error) { if !mb.indexLoaded { if err := mb.readIndex(); err != nil { return nil, err } } - messages := make([]storage.Message, len(mb.messages)) for i, m := range mb.messages { messages[i] = m @@ -222,35 +209,32 @@ func (mb *Mailbox) GetMessages() ([]storage.Message, error) { return messages, nil } -// GetMessage decodes a single message by Id and returns a Message object -func (mb *Mailbox) GetMessage(id string) (storage.Message, error) { +// getMessage decodes a single message by ID and returns a Message object. +func (mb *mbox) getMessage(id string) (storage.Message, error) { if !mb.indexLoaded { if err := mb.readIndex(); err != nil { return nil, err } } - if id == "latest" && len(mb.messages) != 0 { return mb.messages[len(mb.messages)-1], nil } - for _, m := range mb.messages { if m.Fid == id { return m, nil } } - return nil, storage.ErrNotExist } -// Purge deletes all messages in this mailbox -func (mb *Mailbox) Purge() error { +// purge deletes all messages in this mailbox. +func (mb *mbox) purge() error { mb.messages = mb.messages[:0] return mb.writeIndex() } // readIndex loads the mailbox index data from disk -func (mb *Mailbox) readIndex() error { +func (mb *mbox) readIndex() error { // Clear message slice, open index mb.messages = mb.messages[:0] // Lock for reading @@ -293,7 +277,7 @@ func (mb *Mailbox) readIndex() error { } // writeIndex overwrites the index on disk with the current mailbox data -func (mb *Mailbox) writeIndex() error { +func (mb *mbox) writeIndex() error { // Lock for writing indexMx.Lock() defer indexMx.Unlock() @@ -335,7 +319,7 @@ func (mb *Mailbox) writeIndex() error { } // createDir checks for the presence of the path for this mailbox, creates it if needed -func (mb *Mailbox) createDir() error { +func (mb *mbox) createDir() error { dirMx.Lock() defer dirMx.Unlock() if _, err := os.Stat(mb.path); err != nil { @@ -348,7 +332,7 @@ func (mb *Mailbox) createDir() error { } // removeDir removes the mailbox, plus empty higher level directories -func (mb *Mailbox) removeDir() error { +func (mb *mbox) removeDir() error { dirMx.Lock() defer dirMx.Unlock() // remove mailbox dir, including index file diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 54adb58..83cf635 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -25,19 +25,12 @@ type Store interface { GetMessages(mailbox string) ([]Message, error) PurgeMessages(mailbox string) error VisitMailboxes(f func([]Message) (cont bool)) error - MailboxFor(emailAddress string) (Mailbox, error) // LockFor is a temporary hack to fix #77 until Datastore revamp LockFor(emailAddress string) (*sync.RWMutex, error) // NewMessage is temproary until #69 MessageData refactor NewMessage(mailbox string) (Message, error) } -// Mailbox is an interface to get and manipulate messages in a DataStore -type Mailbox interface { - GetMessages() ([]Message, error) - String() string -} - // Message is an interface for a single message in a Mailbox type Message interface { ID() string diff --git a/pkg/storage/testing.go b/pkg/storage/testing.go index 8757230..6b2604d 100644 --- a/pkg/storage/testing.go +++ b/pkg/storage/testing.go @@ -33,12 +33,6 @@ func (m *MockDataStore) PurgeMessages(name string) error { return args.Error(0) } -// MailboxFor mock function -func (m *MockDataStore) MailboxFor(name string) (Mailbox, error) { - args := m.Called(name) - return args.Get(0).(Mailbox), args.Error(1) -} - // LockFor mock function returns a new RWMutex, never errors. func (m *MockDataStore) LockFor(name string) (*sync.RWMutex, error) { return &sync.RWMutex{}, nil @@ -56,41 +50,6 @@ func (m *MockDataStore) VisitMailboxes(f func([]Message) (cont bool)) error { return nil } -// MockMailbox is a shared mock for unit testing -type MockMailbox struct { - mock.Mock -} - -// GetMessages mock function -func (m *MockMailbox) GetMessages() ([]Message, error) { - args := m.Called() - return args.Get(0).([]Message), args.Error(1) -} - -// GetMessage mock function -func (m *MockMailbox) GetMessage(id string) (Message, error) { - args := m.Called(id) - return args.Get(0).(Message), args.Error(1) -} - -// Purge mock function -func (m *MockMailbox) Purge() error { - args := m.Called() - return args.Error(0) -} - -// NewMessage mock function -func (m *MockMailbox) NewMessage() (Message, error) { - args := m.Called() - return args.Get(0).(Message), args.Error(1) -} - -// String mock function -func (m *MockMailbox) String() string { - args := m.Called() - return args.String(0) -} - // MockMessage is a shared mock for unit testing type MockMessage struct { mock.Mock