From 966615584dea9029f817a52a4da98df57c690062 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 20 Oct 2022 15:52:53 +0200 Subject: [PATCH 1/6] Implement a proper io.Reader interface since the current implementation is flawed --- msg.go | 20 ++++++++++++++++++++ reader.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 reader.go diff --git a/msg.go b/msg.go index 521cd30..dbfac83 100644 --- a/msg.go +++ b/msg.go @@ -807,6 +807,26 @@ func (m *Msg) WriteToSendmailWithContext(ctx context.Context, sp string, a ...st return nil } +// NewReader returns a Msg reader that satisfies the io.Reader interface. +// **Please note:** when creating a new reader, the current state of the Msg is taken, as +// basis for the reader. If you perform changes on Msg after creating the reader, you need +// to perform a call to Msg.UpdateReader first +func (m *Msg) NewReader() io.Reader { + wbuf := bytes.Buffer{} + _, _ = m.Write(&wbuf) + r := &reader{buf: wbuf.Bytes()} + return r +} + +// UpdateReader will update a reader with the content of the current Msg and reset the reader position +// to the start +func (m *Msg) UpdateReader(r *reader) { + wbuf := bytes.Buffer{} + _, _ = m.Write(&wbuf) + r.Reset() + r.buf = wbuf.Bytes() +} + // Read outputs the length of p into p to satisfy the io.Reader interface func (m *Msg) Read(p []byte) (int, error) { wbuf := bytes.Buffer{} diff --git a/reader.go b/reader.go new file mode 100644 index 0000000..819675f --- /dev/null +++ b/reader.go @@ -0,0 +1,33 @@ +package mail + +import "io" + +// reader is a type that implements the io.Reader interface for a Msg +type reader struct { + buf []byte // contents are the bytes buf[off : len(buf)] + off int // read at &buf[off], write at &buf[len(buf)] +} + +// Read reads the length of p of the Msg buffer to satisfy the io.Reader interface +func (r *reader) Read(p []byte) (n int, err error) { + if r.empty() { + r.Reset() + if len(p) == 0 { + return 0, nil + } + return 0, io.EOF + } + n = copy(p, r.buf[r.off:]) + r.off += n + return n, nil +} + +// Reset resets the Reader buffer to be empty, but it retains the underlying storage +// for use by future writes. +func (r *reader) Reset() { + r.buf = r.buf[:0] + r.off = 0 +} + +// empty reports whether the unread portion of the Reader buffer is empty. +func (r *reader) empty() bool { return len(r.buf) <= r.off } From 4a309dc73ae09c2a22422fed83ab090e1e233f45 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 20 Oct 2022 15:53:59 +0200 Subject: [PATCH 2/6] REUSE fix --- reader.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/reader.go b/reader.go index 819675f..124c51d 100644 --- a/reader.go +++ b/reader.go @@ -1,3 +1,7 @@ +// SPDX-FileCopyrightText: 2022 Winni Neessen +// +// SPDX-License-Identifier: MIT + package mail import "io" From 5faa6dfbd630e628b51e59490b4e5d5d4ff7bdf0 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 20 Oct 2022 18:03:57 +0200 Subject: [PATCH 3/6] We now return a Reader instead of the io.Reader interface --- msg.go | 29 +++++++++++-------- msg_test.go | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++ reader.go | 20 +++++++++----- 3 files changed, 111 insertions(+), 18 deletions(-) diff --git a/msg.go b/msg.go index dbfac83..782489f 100644 --- a/msg.go +++ b/msg.go @@ -807,24 +807,31 @@ func (m *Msg) WriteToSendmailWithContext(ctx context.Context, sp string, a ...st return nil } -// NewReader returns a Msg reader that satisfies the io.Reader interface. -// **Please note:** when creating a new reader, the current state of the Msg is taken, as -// basis for the reader. If you perform changes on Msg after creating the reader, you need -// to perform a call to Msg.UpdateReader first -func (m *Msg) NewReader() io.Reader { +// NewReader returns a Reader type that satisfies the io.Reader interface. +// +// IMPORTANT: when creating a new Reader, the current state of the Msg is taken, as +// basis for the Reader. If you perform changes on Msg after creating the Reader, these +// changes will not be reflected in the Reader. You will have to use Msg.UpdateReader +// first to update the Reader's buffer with the current Msg content +func (m *Msg) NewReader() *Reader { + r := &Reader{} wbuf := bytes.Buffer{} - _, _ = m.Write(&wbuf) - r := &reader{buf: wbuf.Bytes()} + _, err := m.Write(&wbuf) + if err != nil { + r.err = fmt.Errorf("failed to write Msg to Reader buffer: %w", err) + } + r.buf = wbuf.Bytes() return r } -// UpdateReader will update a reader with the content of the current Msg and reset the reader position -// to the start -func (m *Msg) UpdateReader(r *reader) { +// UpdateReader will update a Reader with the content of the given Msg and reset the +// Reader position to the start +func (m *Msg) UpdateReader(r *Reader) { wbuf := bytes.Buffer{} - _, _ = m.Write(&wbuf) + _, err := m.Write(&wbuf) r.Reset() r.buf = wbuf.Bytes() + r.err = err } // Read outputs the length of p into p to satisfy the io.Reader interface diff --git a/msg_test.go b/msg_test.go index 5b24251..095c215 100644 --- a/msg_test.go +++ b/msg_test.go @@ -1784,6 +1784,86 @@ func TestMsg_Read_ioCopy(t *testing.T) { } } +// TestMsg_NewReader tests the Msg.NewReader method +func TestMsg_NewReader(t *testing.T) { + m := NewMsg() + m.SetBodyString(TypeTextPlain, "TEST123") + mr := m.NewReader() + if mr == nil { + t.Errorf("NewReader failed: Reader is nil") + } +} + +// TestMsg_NewReader_broken tests the Msg.NewReader method error handling +func TestMsg_NewReader_broken(t *testing.T) { + m := &Msg{} + mr := m.NewReader() + if mr == nil { + t.Errorf("NewReader failed: Reader is nil") + } +} + +// TestMsg_NewReader_ioCopy tests the Msg.NewReader method using io.Copy +func TestMsg_NewReader_ioCopy(t *testing.T) { + wbuf1 := bytes.Buffer{} + wbuf2 := bytes.Buffer{} + m := NewMsg() + m.SetBodyString(TypeTextPlain, "TEST123") + mr := m.NewReader() + if mr == nil { + t.Errorf("NewReader failed: Reader is nil") + } + + // First we use WriteTo to have something to compare to + _, err := m.WriteTo(&wbuf1) + if err != nil { + t.Errorf("failed to write body to buffer: %s", err) + } + + // Then we write to wbuf2 via io.Copy + n, err := io.Copy(&wbuf2, mr) + if err != nil { + t.Errorf("failed to use io.Copy on Reader: %s", err) + } + if n != int64(wbuf1.Len()) { + t.Errorf("message length of WriteTo and io.Copy differ. Expected: %d, got: %d", wbuf1.Len(), n) + } + if wbuf1.String() != wbuf2.String() { + t.Errorf("message content of WriteTo and io.Copy differ") + } +} + +// TestMsg_UpdateReader tests the Msg.UpdateReader method +func TestMsg_UpdateReader(t *testing.T) { + m := NewMsg() + m.Subject("Subject-Run 1") + mr := m.NewReader() + if mr == nil { + t.Errorf("NewReader failed: Reader is nil") + } + wbuf1 := bytes.Buffer{} + _, err := io.Copy(&wbuf1, mr) + if err != nil { + t.Errorf("io.Copy on Reader failed: %s", err) + } + if !strings.Contains(wbuf1.String(), "Subject: Subject-Run 1") { + t.Errorf("io.Copy on Reader failed. Expected to find %q but string in Subject was not found", + "Subject-Run 1") + } + + m.Subject("Subject-Run 2") + m.UpdateReader(mr) + wbuf2 := bytes.Buffer{} + _, err = io.Copy(&wbuf2, mr) + if err != nil { + t.Errorf("2nd io.Copy on Reader failed: %s", err) + } + if !strings.Contains(wbuf2.String(), "Subject: Subject-Run 2") { + t.Errorf("io.Copy on Reader failed. Expected to find %q but string in Subject was not found", + "Subject-Run 2") + } +} + // TestMsg_SetBodyTextTemplate tests the Msg.SetBodyTextTemplate method func TestMsg_SetBodyTextTemplate(t *testing.T) { tests := []struct { diff --git a/reader.go b/reader.go index 124c51d..fbb0612 100644 --- a/reader.go +++ b/reader.go @@ -4,16 +4,22 @@ package mail -import "io" +import ( + "io" +) -// reader is a type that implements the io.Reader interface for a Msg -type reader struct { +// Reader is a type that implements the io.Reader interface for a Msg +type Reader struct { buf []byte // contents are the bytes buf[off : len(buf)] off int // read at &buf[off], write at &buf[len(buf)] + err error // initalization error } // Read reads the length of p of the Msg buffer to satisfy the io.Reader interface -func (r *reader) Read(p []byte) (n int, err error) { +func (r *Reader) Read(p []byte) (n int, err error) { + if r.err != nil { + return 0, r.err + } if r.empty() { r.Reset() if len(p) == 0 { @@ -23,15 +29,15 @@ func (r *reader) Read(p []byte) (n int, err error) { } n = copy(p, r.buf[r.off:]) r.off += n - return n, nil + return n, err } // Reset resets the Reader buffer to be empty, but it retains the underlying storage // for use by future writes. -func (r *reader) Reset() { +func (r *Reader) Reset() { r.buf = r.buf[:0] r.off = 0 } // empty reports whether the unread portion of the Reader buffer is empty. -func (r *reader) empty() bool { return len(r.buf) <= r.off } +func (r *Reader) empty() bool { return len(r.buf) <= r.off } From 183fb347d6fcc7f416918ad5d2645d9e0cad38cf Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 20 Oct 2022 18:06:26 +0200 Subject: [PATCH 4/6] Fix create header maps in case SetHeader or SetAddrHeader is called on an empty Msg type --- msg.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/msg.go b/msg.go index 782489f..89d4ad2 100644 --- a/msg.go +++ b/msg.go @@ -181,6 +181,9 @@ func (m *Msg) Charset() string { // SetHeader sets a generic header field of the Msg func (m *Msg) SetHeader(h Header, v ...string) { + if m.genHeader == nil { + m.genHeader = make(map[Header][]string) + } for i, hv := range v { v[i] = m.encodeString(hv) } @@ -189,6 +192,9 @@ func (m *Msg) SetHeader(h Header, v ...string) { // SetAddrHeader sets an address related header field of the Msg func (m *Msg) SetAddrHeader(h AddrHeader, v ...string) error { + if m.addrHeader == nil { + m.addrHeader = make(map[AddrHeader][]*mail.Address) + } var al []*mail.Address for _, av := range v { a, err := mail.ParseAddress(av) From b6beeb5cae29655f6fc1cd157b342754a2c74c58 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 20 Oct 2022 18:26:51 +0200 Subject: [PATCH 5/6] Finalized the Reader type and removed the broken Read() method --- msg.go | 10 ------- msg_test.go | 76 ++------------------------------------------------ reader.go | 5 ++++ reader_test.go | 68 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 84 deletions(-) create mode 100644 reader_test.go diff --git a/msg.go b/msg.go index 89d4ad2..bfb521d 100644 --- a/msg.go +++ b/msg.go @@ -840,16 +840,6 @@ func (m *Msg) UpdateReader(r *Reader) { r.err = err } -// Read outputs the length of p into p to satisfy the io.Reader interface -func (m *Msg) Read(p []byte) (int, error) { - wbuf := bytes.Buffer{} - _, err := m.WriteTo(&wbuf) - if err != nil { - return 0, fmt.Errorf("failed to write message to internal write buffer: %w", err) - } - return wbuf.Read(p) -} - // encodeString encodes a string based on the configured message encoder and the corresponding // charset for the Msg func (m *Msg) encodeString(s string) string { diff --git a/msg_test.go b/msg_test.go index 095c215..0abb724 100644 --- a/msg_test.go +++ b/msg_test.go @@ -1718,72 +1718,6 @@ func TestMsg_multipleWrites(t *testing.T) { } } -// TestMsg_Read tests the Msg.Read method that implements the io.Reader interface -func TestMsg_Read(t *testing.T) { - tests := []struct { - name string - plen int - }{ - {"P length is bigger than the mail", 32000}, - {"P length is smaller than the mail", 128}, - } - - m := NewMsg() - m.SetBodyString(TypeTextPlain, "TEST123") - wbuf := bytes.Buffer{} - _, err := m.Write(&wbuf) - if err != nil { - t.Errorf("failed to write message into temporary buffer: %s", err) - } - elen := wbuf.Len() - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - p := make([]byte, tt.plen) - n, err := m.Read(p) - if err != nil { - t.Errorf("failed to Read(): %s", err) - } - if n == 0 { - t.Errorf("failed to Read() - received 0 bytes of data") - } - if tt.plen >= elen && n != elen { - t.Errorf("failed to Read() - not all data received. Expected: %d, got: %d", elen, n) - } - if tt.plen < elen && n != tt.plen { - t.Errorf("failed to Read() - full length of p wasn't filled with data. Expected: %d, got: %d", - tt.plen, n) - } - }) - } -} - -// TestMsg_Read_ioCopy tests the Msg.Read method using io.Copy -func TestMsg_Read_ioCopy(t *testing.T) { - wbuf1 := bytes.Buffer{} - wbuf2 := bytes.Buffer{} - m := NewMsg() - m.SetBodyString(TypeTextPlain, "TEST123") - - // First we use WriteTo to have something to compare to - _, err := m.WriteTo(&wbuf1) - if err != nil { - t.Errorf("failed to write body to buffer: %s", err) - } - - // Then we write to wbuf2 via io.Copy - n, err := io.Copy(&wbuf2, m) - if err != nil { - t.Errorf("failed to use io.Copy on Msg: %s", err) - } - if n != int64(wbuf1.Len()) { - t.Errorf("message length of WriteTo and io.Copy differ. Expected: %d, got: %d", wbuf1.Len(), n) - } - if wbuf1.String() != wbuf2.String() { - t.Errorf("message content of WriteTo and io.Copy differ") - } -} - // TestMsg_NewReader tests the Msg.NewReader method func TestMsg_NewReader(t *testing.T) { m := NewMsg() @@ -1792,14 +1726,8 @@ func TestMsg_NewReader(t *testing.T) { if mr == nil { t.Errorf("NewReader failed: Reader is nil") } -} - -// TestMsg_NewReader_broken tests the Msg.NewReader method error handling -func TestMsg_NewReader_broken(t *testing.T) { - m := &Msg{} - mr := m.NewReader() - if mr == nil { - t.Errorf("NewReader failed: Reader is nil") + if mr.Error() != nil { + t.Errorf("NewReader failed: %s", mr.Error()) } } diff --git a/reader.go b/reader.go index fbb0612..dfc5113 100644 --- a/reader.go +++ b/reader.go @@ -15,6 +15,11 @@ type Reader struct { err error // initalization error } +// Error returns an error if the Reader err field is not nil +func (r *Reader) Error() error { + return r.err +} + // Read reads the length of p of the Msg buffer to satisfy the io.Reader interface func (r *Reader) Read(p []byte) (n int, err error) { if r.err != nil { diff --git a/reader_test.go b/reader_test.go new file mode 100644 index 0000000..82055f9 --- /dev/null +++ b/reader_test.go @@ -0,0 +1,68 @@ +package mail + +import ( + "bytes" + "fmt" + "testing" +) + +// TestReader_Read tests the Reader.Read method that implements the io.Reader interface +func TestReader_Read(t *testing.T) { + tests := []struct { + name string + plen int + }{ + {"P length is bigger than the mail", 3200000}, + {"P length is smaller than the mail", 128}, + } + + m := NewMsg() + m.SetBodyString(TypeTextPlain, "TEST123") + wbuf := bytes.Buffer{} + _, err := m.Write(&wbuf) + if err != nil { + t.Errorf("failed to write message into temporary buffer: %s", err) + } + elen := wbuf.Len() + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := make([]byte, tt.plen) + mr := m.NewReader() + n, err := mr.Read(p) + if err != nil { + t.Errorf("failed to Read(): %s", err) + } + if n == 0 { + t.Errorf("failed to Read() - received 0 bytes of data") + } + if tt.plen >= elen && n != elen { + t.Errorf("failed to Read() - not all data received. Expected: %d, got: %d", elen, n) + } + if tt.plen < elen && n != tt.plen { + t.Errorf("failed to Read() - full length of p wasn't filled with data. Expected: %d, got: %d", + tt.plen, n) + } + }) + } +} + +// TestReader_Read_error tests the Reader.Read method with an intentional error +func TestReader_Read_error(t *testing.T) { + r := Reader{err: fmt.Errorf("FAILED")} + var p []byte + _, err := r.Read(p) + if err == nil { + t.Errorf("Reader was supposed to fail, but didn't") + } +} + +// TestReader_Read_empty tests the Reader.Read method with an empty buffer +func TestReader_Read_empty(t *testing.T) { + r := Reader{buf: []byte{}} + var p []byte + _, err := r.Read(p) + if err != nil { + t.Errorf("Reader failed: %s", err) + } +} From 7cbb5a71e3d1f9dd0c3a67d027855c83806aeb09 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 20 Oct 2022 18:33:18 +0200 Subject: [PATCH 6/6] Fix REUSE header --- reader_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/reader_test.go b/reader_test.go index 82055f9..64e2193 100644 --- a/reader_test.go +++ b/reader_test.go @@ -1,3 +1,7 @@ +// SPDX-FileCopyrightText: 2022 Winni Neessen +// +// SPDX-License-Identifier: MIT + package mail import (