From e66ad2be1a47ea5f6cce8fcb8db5b054812bbdf3 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 31 Jan 2023 17:38:31 +0100 Subject: [PATCH 1/3] Fix Attach/EmbedReader and implement Attach/EmbedReadSeeker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR addresses #110. `Msg.AttachReader()` would not output the attached file after consecutive writes (e.g. a write to a file and then send via Client). This PR fixes this behaviour by first reading the io.Reader into memory and then creating a new `bytes.Reader`, which does support seeking. In the writeFunc we then seek to position 0 after a successful `io.Copy`. This is probably not the most memory efficient way of handling this, but otherwise we'll have to break the `io.Reader` interface. Additionally, a new way of attaching/embedding files has been added: `Msg.AttachReadSeeker()` and `Msg.EmbedReadSeeker()` which take a ´io.ReadSeeker` as argument instead. These two methods will skip the reading into memory and make use of the `Seek` method of the corresponding interface instead. --- msg.go | 42 ++++++++++++++++++++++++++++++++++++++---- 1 file changed, 38 insertions(+), 4 deletions(-) diff --git a/msg.go b/msg.go index 28ed932f..8d692457 100644 --- a/msg.go +++ b/msg.go @@ -719,6 +719,12 @@ func (m *Msg) AttachReader(n string, r io.Reader, o ...FileOption) { m.attachments = m.appendFile(m.attachments, f, o...) } +// AttachReadSeeker adds an attachment File via io.ReadSeeker to the Msg +func (m *Msg) AttachReadSeeker(n string, r io.ReadSeeker, o ...FileOption) { + f := fileFromReadSeeker(n, r) + m.attachments = m.appendFile(m.attachments, f, o...) +} + // AttachHTMLTemplate adds the output of a html/template.Template pointer as File attachment to the Msg func (m *Msg) AttachHTMLTemplate(n string, t *ht.Template, d interface{}, o ...FileOption) error { f, err := fileFromHTMLTemplate(n, t, d) @@ -767,6 +773,12 @@ func (m *Msg) EmbedReader(n string, r io.Reader, o ...FileOption) { m.embeds = m.appendFile(m.embeds, f, o...) } +// EmbedReadSeeker adds an embedded File from an io.ReadSeeker to the Msg +func (m *Msg) EmbedReadSeeker(n string, r io.ReadSeeker, o ...FileOption) { + f := fileFromReadSeeker(n, r) + m.embeds = m.appendFile(m.embeds, f, o...) +} + // EmbedHTMLTemplate adds the output of a html/template.Template pointer as embedded File to the Msg func (m *Msg) EmbedHTMLTemplate(n string, t *ht.Template, d interface{}, o ...FileOption) error { f, err := fileFromHTMLTemplate(n, t, d) @@ -1114,15 +1126,37 @@ func fileFromFS(n string) *File { // fileFromReader returns a File pointer from a given io.Reader func fileFromReader(n string, r io.Reader) *File { + d, err := io.ReadAll(r) + if err != nil { + return &File{} + } + br := bytes.NewReader(d) return &File{ - Name: filepath.Base(n), + Name: n, + Header: make(map[string][]string), + Writer: func(w io.Writer) (int64, error) { + rb, cerr := io.Copy(w, br) + if cerr != nil { + return rb, cerr + } + _, cerr = br.Seek(0, io.SeekStart) + return rb, cerr + }, + } +} + +// fileFromReadSeeker returns a File pointer from a given io.ReadSeeker +func fileFromReadSeeker(n string, r io.ReadSeeker) *File { + return &File{ + Name: n, Header: make(map[string][]string), Writer: func(w io.Writer) (int64, error) { - nb, err := io.Copy(w, r) + rb, err := io.Copy(w, r) if err != nil { - return nb, err + return rb, err } - return nb, nil + _, err = r.Seek(0, io.SeekStart) + return rb, err }, } } From c9794069cd5c3d75a172c33ea295aa0be157f9e9 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 31 Jan 2023 17:48:19 +0100 Subject: [PATCH 2/3] Add Caveat note about the `io.Reader` methods being memory inefficient --- msg.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/msg.go b/msg.go index 8d692457..474a2164 100644 --- a/msg.go +++ b/msg.go @@ -714,6 +714,11 @@ func (m *Msg) AttachFile(n string, o ...FileOption) { } // AttachReader adds an attachment File via io.Reader to the Msg +// +// CAVEAT: For AttachReader to work it has to read all data of the io.Reader +// into memory first, so it can seek through it. Using larger amounts of +// data on the io.Reader should be avoided. For such, it is recommeded to +// either use AttachFile or AttachReadSeeker instead func (m *Msg) AttachReader(n string, r io.Reader, o ...FileOption) { f := fileFromReader(n, r) m.attachments = m.appendFile(m.attachments, f, o...) @@ -768,6 +773,11 @@ func (m *Msg) EmbedFile(n string, o ...FileOption) { } // EmbedReader adds an embedded File from an io.Reader to the Msg +// +// CAVEAT: For AttachReader to work it has to read all data of the io.Reader +// into memory first, so it can seek through it. Using larger amounts of +// data on the io.Reader should be avoided. For such, it is recommeded to +// either use AttachFile or AttachReadSeeker instead func (m *Msg) EmbedReader(n string, r io.Reader, o ...FileOption) { f := fileFromReader(n, r) m.embeds = m.appendFile(m.embeds, f, o...) From 516b1f8ee2b0ad93f8b6a03838446e1fa62a597d Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 31 Jan 2023 20:38:48 +0100 Subject: [PATCH 3/3] Added test coverage for the specific bug in #110 and the newly added methods --- msg_test.go | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/msg_test.go b/msg_test.go index 1ac0ac1b..a2c4a8ef 100644 --- a/msg_test.go +++ b/msg_test.go @@ -2667,3 +2667,127 @@ func TestMsg_GetBccString(t *testing.T) { `"Toni Cc" "`, fh[0]) } } + +// TestMsg_AttachEmbedReader_consecutive tests the Msg.AttachReader and Msg.EmbedReader +// methods with consecutive calls to Msg.WriteTo to make sure the attachments are not +// lost (see Github issue #110) +func TestMsg_AttachEmbedReader_consecutive(t *testing.T) { + ts1 := "This is a test string" + ts2 := "Another test string" + m := NewMsg() + m.AttachReader("attachment.txt", bytes.NewBufferString(ts1)) + m.EmbedReader("embeded.txt", bytes.NewBufferString(ts2)) + obuf1 := &bytes.Buffer{} + obuf2 := &bytes.Buffer{} + _, err := m.WriteTo(obuf1) + if err != nil { + t.Errorf("WriteTo to first output buffer failed: %s", err) + } + _, err = m.WriteTo(obuf2) + if err != nil { + t.Errorf("WriteTo to second output buffer failed: %s", err) + } + if !strings.Contains(obuf1.String(), "VGhpcyBpcyBhIHRlc3Qgc3RyaW5n") { + t.Errorf("Expected file attachment string not found in first output buffer") + } + if !strings.Contains(obuf2.String(), "VGhpcyBpcyBhIHRlc3Qgc3RyaW5n") { + t.Errorf("Expected file attachment string not found in second output buffer") + } + if !strings.Contains(obuf1.String(), "QW5vdGhlciB0ZXN0IHN0cmluZw==") { + t.Errorf("Expected embeded file string not found in first output buffer") + } + if !strings.Contains(obuf2.String(), "QW5vdGhlciB0ZXN0IHN0cmluZw==") { + t.Errorf("Expected embded file string not found in second output buffer") + } +} + +// TestMsg_AttachEmbedReadSeeker_consecutive tests the Msg.AttachReadSeeker and +// Msg.EmbedReadSeeker methods with consecutive calls to Msg.WriteTo to make +// sure the attachments are not lost (see Github issue #110) +func TestMsg_AttachEmbedReadSeeker_consecutive(t *testing.T) { + ts1 := []byte("This is a test string") + ts2 := []byte("Another test string") + m := NewMsg() + m.AttachReadSeeker("attachment.txt", bytes.NewReader(ts1)) + m.EmbedReadSeeker("embeded.txt", bytes.NewReader(ts2)) + obuf1 := &bytes.Buffer{} + obuf2 := &bytes.Buffer{} + _, err := m.WriteTo(obuf1) + if err != nil { + t.Errorf("WriteTo to first output buffer failed: %s", err) + } + _, err = m.WriteTo(obuf2) + if err != nil { + t.Errorf("WriteTo to second output buffer failed: %s", err) + } + if !strings.Contains(obuf1.String(), "VGhpcyBpcyBhIHRlc3Qgc3RyaW5n") { + t.Errorf("Expected file attachment string not found in first output buffer") + } + if !strings.Contains(obuf2.String(), "VGhpcyBpcyBhIHRlc3Qgc3RyaW5n") { + t.Errorf("Expected file attachment string not found in second output buffer") + } + if !strings.Contains(obuf1.String(), "QW5vdGhlciB0ZXN0IHN0cmluZw==") { + t.Errorf("Expected embeded file string not found in first output buffer") + } + if !strings.Contains(obuf2.String(), "QW5vdGhlciB0ZXN0IHN0cmluZw==") { + t.Errorf("Expected embded file string not found in second output buffer") + } +} + +// TestMsg_AttachReadSeeker tests the Msg.AttachReadSeeker method +func TestMsg_AttachReadSeeker(t *testing.T) { + m := NewMsg() + ts := []byte("This is a test string") + r := bytes.NewReader(ts) + m.AttachReadSeeker("testfile.txt", r) + if len(m.attachments) != 1 { + t.Errorf("AttachReadSeeker() failed. Number of attachments expected: %d, got: %d", 1, + len(m.attachments)) + return + } + file := m.attachments[0] + if file == nil { + t.Errorf("AttachReadSeeker() failed. Attachment file pointer is nil") + return + } + if file.Name != "testfile.txt" { + t.Errorf("AttachReadSeeker() failed. Expected file name: %s, got: %s", "testfile.txt", + file.Name) + } + wbuf := bytes.Buffer{} + if _, err := file.Writer(&wbuf); err != nil { + t.Errorf("execute WriterFunc failed: %s", err) + } + if wbuf.String() != string(ts) { + t.Errorf("AttachReadSeeker() failed. Expected string: %q, got: %q", ts, wbuf.String()) + } +} + +// TestMsg_EmbedReadSeeker tests the Msg.EmbedReadSeeker method +func TestMsg_EmbedReadSeeker(t *testing.T) { + m := NewMsg() + ts := []byte("This is a test string") + r := bytes.NewReader(ts) + m.EmbedReadSeeker("testfile.txt", r) + if len(m.embeds) != 1 { + t.Errorf("EmbedReadSeeker() failed. Number of attachments expected: %d, got: %d", 1, + len(m.embeds)) + return + } + file := m.embeds[0] + if file == nil { + t.Errorf("EmbedReadSeeker() failed. Embedded file pointer is nil") + return + } + if file.Name != "testfile.txt" { + t.Errorf("EmbedReadSeeker() failed. Expected file name: %s, got: %s", "testfile.txt", + file.Name) + } + wbuf := bytes.Buffer{} + if _, err := file.Writer(&wbuf); err != nil { + t.Errorf("execute WriterFunc failed: %s", err) + } + if wbuf.String() != string(ts) { + t.Errorf("EmbedReadSeeker() failed. Expected string: %q, got: %q", ts, wbuf.String()) + } +}