From 5e13e5076301a9b2eaaaa95a8e4b35df93d39b0f Mon Sep 17 00:00:00 2001 From: James Hillyerd Date: Wed, 14 Mar 2018 22:51:40 -0700 Subject: [PATCH] test: Start work on test suite for #82 - smtp: Tidy up []byte/buffer/string use in delivery #69 --- pkg/server/smtp/handler.go | 60 ++++++++-------------- pkg/storage/file/fstore.go | 1 + pkg/storage/file/fstore_test.go | 14 ++++++ pkg/storage/storage.go | 2 - pkg/test/storage_suite.go | 89 +++++++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 42 deletions(-) create mode 100644 pkg/test/storage_suite.go diff --git a/pkg/server/smtp/handler.go b/pkg/server/smtp/handler.go index 96fa6db..d9dad39 100644 --- a/pkg/server/smtp/handler.go +++ b/pkg/server/smtp/handler.go @@ -352,7 +352,6 @@ func (ss *Session) mailHandler(cmd string, arg string) { func (ss *Session) dataHandler() { recipients := make([]recipientDetails, 0, ss.recipients.Len()) // Get a Mailbox and a new Message for each recipient - msgSize := 0 if ss.server.storeMessages { for e := ss.recipients.Front(); e != nil; e = e.Next() { recip := e.Value.(string) @@ -373,11 +372,9 @@ func (ss *Session) dataHandler() { } ss.send("354 Start mail input; end with .") - var lineBuf bytes.Buffer - msgBuf := make([][]byte, 0, 1024) + msgBuf := &bytes.Buffer{} for { - lineBuf.Reset() - err := ss.readByteLine(&lineBuf) + lineBuf, err := ss.readByteLine() if err != nil { if netErr, ok := err.(net.Error); ok { if netErr.Timeout() { @@ -388,9 +385,7 @@ func (ss *Session) dataHandler() { ss.enterState(QUIT) return } - line := lineBuf.Bytes() - // ss.logTrace("DATA: %q", line) - if string(line) == ".\r\n" || string(line) == ".\n" { + if bytes.Equal(lineBuf, []byte(".\r\n")) || bytes.Equal(lineBuf, []byte(".\n")) { // Mail data complete if ss.server.storeMessages { // Create a message for each valid recipient @@ -405,7 +400,7 @@ func (ss *Session) dataHandler() { return } mu.Lock() - ok := ss.deliverMessage(r, msgBuf) + ok := ss.deliverMessage(r, msgBuf.Bytes()) mu.Unlock() if ok { expReceivedTotal.Add(1) @@ -420,47 +415,34 @@ func (ss *Session) dataHandler() { expReceivedTotal.Add(1) } ss.send("250 Mail accepted for delivery") - ss.logInfo("Message size %v bytes", msgSize) + ss.logInfo("Message size %v bytes", msgBuf.Len()) ss.reset() return } // SMTP RFC says remove leading periods from input - if len(line) > 0 && line[0] == '.' { - line = line[1:] + if len(lineBuf) > 0 && lineBuf[0] == '.' { + lineBuf = lineBuf[1:] } - // Second append copies line/lineBuf so we can reuse it - msgBuf = append(msgBuf, append([]byte{}, line...)) - msgSize += len(line) - if msgSize > ss.server.maxMessageBytes { + msgBuf.Write(lineBuf) + if msgBuf.Len() > ss.server.maxMessageBytes { // Max message size exceeded ss.send("552 Maximum message size exceeded") ss.logWarn("Max message size exceeded while in DATA") ss.reset() - // Should really cleanup the crap on filesystem (after issue #23) return } } // end for } // deliverMessage creates and populates a new Message for the specified recipient -func (ss *Session) deliverMessage(r recipientDetails, msgBuf [][]byte) (ok bool) { +func (ss *Session) deliverMessage(r recipientDetails, content []byte) (ok bool) { name, err := stringutil.ParseMailboxName(r.localPart) if err != nil { // This parse already succeeded when MailboxFor was called, shouldn't fail here. return false } - buf := bytes.Buffer{} - // Generate Received header - stamp := time.Now().Format(timeStampFormat) - recd := fmt.Sprintf("Received: from %s ([%s]) by %s\r\n for <%s>; %s\r\n", - ss.remoteDomain, ss.remoteHost, ss.server.domain, r.address, stamp) - buf.WriteString(recd) - // Append lines from msgBuf - for _, line := range msgBuf { - buf.Write(line) - } // TODO replace with something that only reads header? - env, err := enmime.ReadEnvelope(bytes.NewReader(buf.Bytes())) + env, err := enmime.ReadEnvelope(bytes.NewReader(content)) if err != nil { ss.logError("Failed to parse message for %q: %v", r.localPart, err) return false @@ -475,6 +457,10 @@ func (ss *Session) deliverMessage(r recipientDetails, msgBuf [][]byte) (ok bool) ss.logError("Failed to get To addresses: %v", err) return false } + // Generate Received header. + stamp := time.Now().Format(timeStampFormat) + recd := strings.NewReader(fmt.Sprintf("Received: from %s ([%s]) by %s\r\n for <%s>; %s\r\n", + ss.remoteDomain, ss.remoteHost, ss.server.domain, r.address, stamp)) delivery := &message.Delivery{ Meta: message.Metadata{ Mailbox: name, @@ -483,14 +469,14 @@ func (ss *Session) deliverMessage(r recipientDetails, msgBuf [][]byte) (ok bool) Date: time.Now(), Subject: env.GetHeader("Subject"), }, - Reader: bytes.NewReader(buf.Bytes()), + Reader: io.MultiReader(recd, bytes.NewReader(content)), } id, err := ss.server.dataStore.AddMessage(delivery) if err != nil { ss.logError("Failed to store message for %q: %s", r.localPart, err) return false } - // Broadcast message information + // Broadcast message information. broadcast := msghub.Message{ Mailbox: name, ID: id, @@ -501,7 +487,6 @@ func (ss *Session) deliverMessage(r recipientDetails, msgBuf [][]byte) (ok bool) Size: delivery.Size(), } ss.server.msgHub.Dispatch(broadcast) - return true } @@ -535,16 +520,11 @@ func (ss *Session) send(msg string) { // readByteLine reads a line of input into the provided buffer. Does // not reset the Buffer - please do so prior to calling. -func (ss *Session) readByteLine(buf io.Writer) error { +func (ss *Session) readByteLine() ([]byte, error) { if err := ss.conn.SetReadDeadline(ss.nextDeadline()); err != nil { - return err + return nil, err } - line, err := ss.reader.ReadBytes('\n') - if err != nil { - return err - } - _, err = buf.Write(line) - return err + return ss.reader.ReadBytes('\n') } // Reads a line of input diff --git a/pkg/storage/file/fstore.go b/pkg/storage/file/fstore.go index c9ecfee..b41b5d3 100644 --- a/pkg/storage/file/fstore.go +++ b/pkg/storage/file/fstore.go @@ -121,6 +121,7 @@ func (fs *Store) AddMessage(m storage.StoreMessage) (id string, err error) { // Update the index. fm.Fdate = m.Date() fm.Ffrom = m.From() + fm.Fto = m.To() fm.Fsize = size fm.Fsubject = m.Subject() mb.messages = append(mb.messages, fm) diff --git a/pkg/storage/file/fstore_test.go b/pkg/storage/file/fstore_test.go index 4b2f287..94380d9 100644 --- a/pkg/storage/file/fstore_test.go +++ b/pkg/storage/file/fstore_test.go @@ -16,9 +16,23 @@ import ( "github.com/jhillyerd/inbucket/pkg/config" "github.com/jhillyerd/inbucket/pkg/message" "github.com/jhillyerd/inbucket/pkg/storage" + "github.com/jhillyerd/inbucket/pkg/test" "github.com/stretchr/testify/assert" ) +// TestSuite runs storage package test suite on file store. +func TestSuite(t *testing.T) { + ds, logbuf := setupDataStore(config.DataStoreConfig{}) + defer teardownDataStore(ds) + test.StoreSuite(t, ds) + 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 directory structure created by filestore func TestFSDirStructure(t *testing.T) { ds, logbuf := setupDataStore(config.DataStoreConfig{}) diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 8f6783a..62d4972 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -28,8 +28,6 @@ type Store interface { VisitMailboxes(f func([]StoreMessage) (cont bool)) 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) (StoreMessage, error) } // StoreMessage represents a message to be stored, or returned from a storage implementation. diff --git a/pkg/test/storage_suite.go b/pkg/test/storage_suite.go new file mode 100644 index 0000000..fd71774 --- /dev/null +++ b/pkg/test/storage_suite.go @@ -0,0 +1,89 @@ +package test + +import ( + "net/mail" + "strings" + "testing" + "time" + + "github.com/jhillyerd/inbucket/pkg/message" + "github.com/jhillyerd/inbucket/pkg/storage" +) + +// StoreSuite runs a set of general tests on the provided Store +func StoreSuite(t *testing.T, store storage.Store) { + testCases := []struct { + name string + test func(*testing.T, storage.Store) + }{ + {"metadata", testMetadata}, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.test(t, store) + }) + } +} + +func testMetadata(t *testing.T, ds storage.Store) { + // Store a message + mailbox := "testmailbox" + from := &mail.Address{Name: "From Person", Address: "from@person.com"} + to := []*mail.Address{ + {Name: "One Person", Address: "one@a.person.com"}, + {Name: "Two Person", Address: "two@b.person.com"}, + } + date := time.Now() + subject := "fantastic test subject line" + content := "doesn't matter" + delivery := &message.Delivery{ + Meta: message.Metadata{ + // ID and Size will be determined by the Store + Mailbox: mailbox, + From: from, + To: to, + Date: date, + Subject: subject, + }, + Reader: strings.NewReader(content), + } + id, err := ds.AddMessage(delivery) + if err != nil { + t.Fatal(err) + } + if id == "" { + t.Fatal("Expected AddMessage() to return non-empty ID string") + } + // Retrieve and validate the message + sm, err := ds.GetMessage(mailbox, id) + if err != nil { + t.Fatal(err) + } + if sm.Mailbox() != mailbox { + t.Errorf("got mailbox %q, want: %q", sm.Mailbox(), mailbox) + } + if sm.ID() != id { + t.Errorf("got id %q, want: %q", sm.ID(), id) + } + if *sm.From() != *from { + t.Errorf("got from %v, want: %v", sm.From(), from) + } + if len(sm.To()) != len(to) { + t.Errorf("got len(to) = %v, want: %v", len(sm.To()), len(to)) + } else { + for i, got := range sm.To() { + if *to[i] != *got { + t.Errorf("got to[%v] %v, want: %v", i, got, to[i]) + } + } + } + if !sm.Date().Equal(date) { + t.Errorf("got date %v, want: %v", sm.Date(), date) + } + if sm.Subject() != subject { + t.Errorf("got subject %q, want: %q", sm.Subject(), subject) + } + if sm.Size() != int64(len(content)) { + t.Errorf("got size %v, want: %v", sm.Size(), len(content)) + } +}