From 47bff15de9cddcd20345377132b9cc173b33503f Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sat, 31 Dec 2022 12:40:42 +0100 Subject: [PATCH 01/16] Introduction of a Msg error type as proposal for #90 This PR introduces the `SendError` type which implements the error interface. A new `senderror` field has been added to the `Msg` as well, so introduce this type to it. I've also added different error variables that indicate the different things that can go wrong during mail delivery. These variables can be checked for, for each `Msg` using the `errors.As` method The `Error()` method of `SendError` will return a detailed error string on why the `Msg` could not be delivered. Additionally, `HasSendError()` and `SendError()` methods have been added to `Msg`. While `HasSendError()` simply returns a bool in case a `Msg` failed during delivery, the `SendError()` will return the full `SendError` error interface. --- client_119.go | 16 ++++++++++++ client_120.go | 16 ++++++++++++ client_test.go | 48 +++++++++++++++++++++++++++++++++++ msg.go | 14 +++++++++++ senderror.go | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 162 insertions(+) create mode 100644 senderror.go diff --git a/client_119.go b/client_119.go index f5b942c..ed27216 100644 --- a/client_119.go +++ b/client_119.go @@ -21,31 +21,41 @@ func (c *Client) Send(ml ...*Msg) error { if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { errs = append(errs, ErrServerNoUnencoded) + m.sendError = SendError{Err: ErrServerNoUnencoded} continue } } f, err := m.GetSender(false) if err != nil { errs = append(errs, err) + m.sendError = SendError{Err: ErrGetSender, details: []error{err}} continue } rl, err := m.GetRecipients() if err != nil { + m.sendError = SendError{Err: ErrGetRcpts, details: []error{err}} errs = append(errs, err) continue } if err := c.mail(f); err != nil { errs = append(errs, fmt.Errorf("sending MAIL FROM command failed: %w", err)) + m.sendError = SendError{Err: ErrSMTPMailFrom, details: []error{err}} if reserr := c.sc.Reset(); reserr != nil { errs = append(errs, reserr) } continue } failed := false + rse := SendError{} + rse.details = make([]error, 0) + rse.rcpt = make([]string, 0) for _, r := range rl { if err := c.rcpt(r); err != nil { errs = append(errs, fmt.Errorf("sending RCPT TO command failed: %w", err)) + rse.Err = ErrSMTPRcptTo + rse.details = append(rse.details, err) + rse.rcpt = append(rse.rcpt, r) failed = true } } @@ -53,30 +63,36 @@ func (c *Client) Send(ml ...*Msg) error { if reserr := c.sc.Reset(); reserr != nil { errs = append(errs, reserr) } + m.sendError = rse continue } w, err := c.sc.Data() if err != nil { errs = append(errs, fmt.Errorf("sending DATA command failed: %w", err)) + m.sendError = SendError{Err: ErrSMTPData, details: []error{err}} continue } _, err = m.WriteTo(w) if err != nil { errs = append(errs, fmt.Errorf("sending mail content failed: %w", err)) + m.sendError = SendError{Err: ErrWriteContent, details: []error{err}} continue } if err := w.Close(); err != nil { errs = append(errs, fmt.Errorf("failed to close DATA writer: %w", err)) + m.sendError = SendError{Err: ErrSMTPDataClose, details: []error{err}} continue } if err := c.Reset(); err != nil { errs = append(errs, fmt.Errorf("sending RSET command failed: %w", err)) + m.sendError = SendError{Err: ErrSMTPReset, details: []error{err}} continue } if err := c.checkConn(); err != nil { errs = append(errs, fmt.Errorf("failed to check server connection: %w", err)) + m.sendError = SendError{Err: ErrConnCheck, details: []error{err}} continue } } diff --git a/client_120.go b/client_120.go index 8990737..568c21d 100644 --- a/client_120.go +++ b/client_120.go @@ -22,31 +22,41 @@ func (c *Client) Send(ml ...*Msg) (rerr error) { if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { rerr = errors.Join(rerr, ErrServerNoUnencoded) + m.sendError = SendError{Err: ErrServerNoUnencoded} continue } } f, err := m.GetSender(false) if err != nil { rerr = errors.Join(rerr, err) + m.sendError = SendError{Err: ErrGetSender, details: []error{err}} continue } rl, err := m.GetRecipients() if err != nil { rerr = errors.Join(rerr, err) + m.sendError = SendError{Err: ErrGetRcpts, details: []error{err}} continue } if err := c.mail(f); err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending MAIL FROM command failed: %w", err)) + m.sendError = SendError{Err: ErrSMTPMailFrom, details: []error{err}} if reserr := c.sc.Reset(); reserr != nil { rerr = errors.Join(rerr, reserr) } continue } failed := false + rse := SendError{} + rse.details = make([]error, 0) + rse.rcpt = make([]string, 0) for _, r := range rl { if err := c.rcpt(r); err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending RCPT TO command failed: %w", err)) + rse.Err = ErrSMTPRcptTo + rse.details = append(rse.details, err) + rse.rcpt = append(rse.rcpt, r) failed = true } } @@ -54,30 +64,36 @@ func (c *Client) Send(ml ...*Msg) (rerr error) { if reserr := c.sc.Reset(); reserr != nil { rerr = errors.Join(rerr, reserr) } + m.sendError = rse continue } w, err := c.sc.Data() if err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending DATA command failed: %w", err)) + m.sendError = SendError{Err: ErrSMTPData, details: []error{err}} continue } _, err = m.WriteTo(w) if err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending mail content failed: %w", err)) + m.sendError = SendError{Err: ErrWriteContent, details: []error{err}} continue } if err := w.Close(); err != nil { rerr = errors.Join(rerr, fmt.Errorf("failed to close DATA writer: %w", err)) + m.sendError = SendError{Err: ErrSMTPDataClose, details: []error{err}} continue } if err := c.Reset(); err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending RSET command failed: %w", err)) + m.sendError = SendError{Err: ErrSMTPReset, details: []error{err}} continue } if err := c.checkConn(); err != nil { rerr = errors.Join(rerr, fmt.Errorf("failed to check server connection: %w", err)) + m.sendError = SendError{Err: ErrConnCheck, details: []error{err}} } } diff --git a/client_test.go b/client_test.go index f674d4f..edf850b 100644 --- a/client_test.go +++ b/client_test.go @@ -7,6 +7,7 @@ package mail import ( "context" "crypto/tls" + "errors" "fmt" "net/smtp" "os" @@ -989,6 +990,53 @@ func TestValidateLine(t *testing.T) { } } +// TestClient_Send_MsgSendError tests the Send() method of Client with a broken and verifies +// that the SendError type works properly +func TestClient_Send_MsgSendError(t *testing.T) { + if os.Getenv("TEST_ALLOW_SEND") == "" { + t.Skipf("TEST_ALLOW_SEND is not set. Skipping mail sending test") + } + var msgs []*Msg + rcpts := []string{"invalid@domain.tld", "invalid@address.invalid"} + for _, rcpt := range rcpts { + m := NewMsg() + _ = m.FromFormat("go-mail Test Mailer", os.Getenv("TEST_FROM")) + _ = m.To(rcpt) + m.Subject(fmt.Sprintf("This is a test mail from go-mail/v%s", VERSION)) + m.SetBulk() + m.SetDate() + m.SetMessageID() + m.SetBodyString(TypeTextPlain, "This is a test mail from the go-mail library") + msgs = append(msgs, m) + } + + c, err := getTestConnection(true) + if err != nil { + t.Skipf("failed to create test client: %s. Skipping tests", err) + } + + ctx, cfn := context.WithTimeout(context.Background(), DefaultTimeout) + defer cfn() + if err := c.DialWithContext(ctx); err != nil { + t.Errorf("failed to dial to sending server: %s", err) + } + if err := c.Send(msgs...); err == nil { + t.Errorf("sending messages with broken recipients was supposed to fail but didn't") + } + if err := c.Close(); err != nil { + t.Errorf("failed to close client connection: %s", err) + } + for _, m := range msgs { + if !m.HasSendError() { + t.Errorf("message was expected to have a send error, but didn't") + } + se := SendError{Err: ErrSMTPRcptTo} + if !errors.As(m.SendError(), &se) { + t.Errorf("message with broken recipient was expected to return a ErrSMTPRcptTo error, but didn't") + } + } +} + // getTestConnection takes environment variables to establish a connection to a real // SMTP server to test all functionality that requires a connection func getTestConnection(auth bool) (*Client, error) { diff --git a/msg.go b/msg.go index 608e080..82e4739 100644 --- a/msg.go +++ b/msg.go @@ -90,6 +90,9 @@ type Msg struct { // middlewares is the list of middlewares to apply to the Msg before sending in FIFO order middlewares []Middleware + + // sendError holds the SendError in case a Msg could not be delivered during the Client.Send operation + sendError error } // SendmailPath is the default system path to the sendmail binary @@ -957,6 +960,17 @@ func (m *Msg) UpdateReader(r *Reader) { r.err = err } +// HasSendError returns true if the Msg experienced an error during the message delivery and the +// senderror field of the Msg is not nil +func (m *Msg) HasSendError() bool { + return m.sendError != nil +} + +// SendError returns the senderror field of the Msg +func (m *Msg) SendError() error { + return m.sendError +} + // 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/senderror.go b/senderror.go new file mode 100644 index 0000000..4ed78bb --- /dev/null +++ b/senderror.go @@ -0,0 +1,68 @@ +package mail + +import ( + "errors" + "fmt" + "strings" +) + +// List of SendError errors +var ( + // ErrGetSender is returned if the Msg.GetSender method fails during a Client.Send + ErrGetSender = errors.New("getting sender address") + + // ErrGetRcpts is returned if the Msg.GetRecipients method fails during a Client.Send + ErrGetRcpts = errors.New("getting recipient addresses") + + // ErrSMTPMailFrom is returned if the Msg delivery failed when sending the MAIL FROM command + // to the sending SMTP server + ErrSMTPMailFrom = errors.New("sending SMTP MAIL FROM command") + + // ErrSMTPRcptTo is returned if the Msg delivery failed when sending the RCPT TO command + // to the sending SMTP server + ErrSMTPRcptTo = errors.New("sending SMTP RCPT TO command") + + // ErrSMTPData is returned if the Msg delivery failed when sending the DATA command + // to the sending SMTP server + ErrSMTPData = errors.New("sending SMTP DATA command") + + // ErrSMTPDataClose is returned if the Msg delivery failed when trying to close the + // Client data writer + ErrSMTPDataClose = errors.New("closing SMTP DATA writer") + + // ErrSMTPReset is returned if the Msg delivery failed when sending the RSET command + // to the sending SMTP server + ErrSMTPReset = errors.New("sending SMTP RESET command") + + // ErrWriteContent is returned if the Msg delivery failed when sending Msg content + // to the Client writer + ErrWriteContent = errors.New("sending message content") + + // ErrConnCheck is returned if the Msg delivery failed when checking if the SMTP + // server connection is still working + ErrConnCheck = errors.New("checking SMTP connection") +) + +// SendError is an error wrapper for delivery errors of the Msg +type SendError struct { + Err error + details []error + rcpt []string +} + +// Error implements the error interface for the SendError type +func (e SendError) Error() string { + var em strings.Builder + _, _ = fmt.Fprintf(&em, "client_send: %s", e.Err) + if len(e.details) > 0 { + for i := range e.details { + em.WriteString(fmt.Sprintf(", error_details: %s", e.details[i])) + } + } + if len(e.rcpt) > 0 { + for i := range e.rcpt { + em.WriteString(fmt.Sprintf(", rcpt: %s", e.rcpt[i])) + } + } + return em.String() +} From 88eb4c56635cf56f83e43d0a13fd5cda86f1c58d Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sat, 31 Dec 2022 12:46:11 +0100 Subject: [PATCH 02/16] I think it's fair that we reset the `Msg.senderror` to `nil` before we `Client.Send` it. Since the send error indicate an error during the mail delivery, in my opinion it should be reset when a re-send is initiated, so that the senderror field always represents the latest delivery error. The send error should be checked after mail delivery and before a retry is started. --- client_119.go | 1 + client_120.go | 1 + 2 files changed, 2 insertions(+) diff --git a/client_119.go b/client_119.go index ed27216..bcb8ec4 100644 --- a/client_119.go +++ b/client_119.go @@ -18,6 +18,7 @@ func (c *Client) Send(ml ...*Msg) error { return fmt.Errorf("failed to send mail: %w", err) } for _, m := range ml { + m.sendError = nil if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { errs = append(errs, ErrServerNoUnencoded) diff --git a/client_120.go b/client_120.go index 568c21d..42b744d 100644 --- a/client_120.go +++ b/client_120.go @@ -19,6 +19,7 @@ func (c *Client) Send(ml ...*Msg) (rerr error) { return } for _, m := range ml { + m.sendError = nil if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { rerr = errors.Join(rerr, ErrServerNoUnencoded) From 42a1fde51f85e9b2d2468d611d51d4ec472ec910 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sat, 31 Dec 2022 12:47:20 +0100 Subject: [PATCH 03/16] Fix missing REUSE license header in senderror.go --- senderror.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/senderror.go b/senderror.go index 4ed78bb..f093852 100644 --- a/senderror.go +++ b/senderror.go @@ -1,3 +1,7 @@ +// SPDX-FileCopyrightText: 2022 Winni Neessen +// +// SPDX-License-Identifier: MIT + package mail import ( From 78df99139927f3748934932edd16839ab80b4c43 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 1 Jan 2023 14:20:13 +0100 Subject: [PATCH 04/16] Proposal change for #90 Did a complete overhaul of the senderror.go. - the list of `errors.New()` has been replaced with constant itoa error reasons as `SendErrReason`. Instead, the `Error()` method now reports the corresponding error message based on the reason. - The `SendError` received a `Is()` method so that we can use `errors.Is()` for very specific error checking. I.e. we can check for `&SendErrors{Reason: ErrSMTPMailFrom, isTemp: true}`. This provides much more flexibility in the error checking capabilities - A `isTemp` field has been added to the `SendError` type, indicating whether the received error is temporary and can be retried or not. Accordingly, the `*Msg` now has a `SendErrorIsTemp()` method indicating the same. The decision is based on the first 3 characters returned from the SMTP server. If the error code is within the 4xx range, the error is seen as temporary - A test for the SendError type has been added --- client_119.go | 27 ++++++------ client_120.go | 27 ++++++------ client_test.go | 9 ++-- msg.go | 10 +++++ senderror.go | 110 +++++++++++++++++++++++++++++++++++++--------- senderror_test.go | 55 +++++++++++++++++++++++ 6 files changed, 189 insertions(+), 49 deletions(-) create mode 100644 senderror_test.go diff --git a/client_119.go b/client_119.go index bcb8ec4..0f06c6c 100644 --- a/client_119.go +++ b/client_119.go @@ -22,41 +22,42 @@ func (c *Client) Send(ml ...*Msg) error { if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { errs = append(errs, ErrServerNoUnencoded) - m.sendError = SendError{Err: ErrServerNoUnencoded} + m.sendError = &SendError{Reason: ErrNoUnencoded, isTemp: false} continue } } f, err := m.GetSender(false) if err != nil { errs = append(errs, err) - m.sendError = SendError{Err: ErrGetSender, details: []error{err}} + m.sendError = &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err)} continue } rl, err := m.GetRecipients() if err != nil { - m.sendError = SendError{Err: ErrGetRcpts, details: []error{err}} + m.sendError = &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err)} errs = append(errs, err) continue } if err := c.mail(f); err != nil { errs = append(errs, fmt.Errorf("sending MAIL FROM command failed: %w", err)) - m.sendError = SendError{Err: ErrSMTPMailFrom, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} if reserr := c.sc.Reset(); reserr != nil { errs = append(errs, reserr) } continue } failed := false - rse := SendError{} - rse.details = make([]error, 0) + rse := &SendError{} + rse.errlist = make([]error, 0) rse.rcpt = make([]string, 0) for _, r := range rl { if err := c.rcpt(r); err != nil { errs = append(errs, fmt.Errorf("sending RCPT TO command failed: %w", err)) - rse.Err = ErrSMTPRcptTo - rse.details = append(rse.details, err) + rse.Reason = ErrSMTPRcptTo + rse.errlist = append(rse.errlist, err) rse.rcpt = append(rse.rcpt, r) + rse.isTemp = isTempError(err) failed = true } } @@ -70,30 +71,30 @@ func (c *Client) Send(ml ...*Msg) error { w, err := c.sc.Data() if err != nil { errs = append(errs, fmt.Errorf("sending DATA command failed: %w", err)) - m.sendError = SendError{Err: ErrSMTPData, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} continue } _, err = m.WriteTo(w) if err != nil { errs = append(errs, fmt.Errorf("sending mail content failed: %w", err)) - m.sendError = SendError{Err: ErrWriteContent, details: []error{err}} + m.sendError = &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} continue } if err := w.Close(); err != nil { errs = append(errs, fmt.Errorf("failed to close DATA writer: %w", err)) - m.sendError = SendError{Err: ErrSMTPDataClose, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} continue } if err := c.Reset(); err != nil { errs = append(errs, fmt.Errorf("sending RSET command failed: %w", err)) - m.sendError = SendError{Err: ErrSMTPReset, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} continue } if err := c.checkConn(); err != nil { errs = append(errs, fmt.Errorf("failed to check server connection: %w", err)) - m.sendError = SendError{Err: ErrConnCheck, details: []error{err}} + m.sendError = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} continue } } diff --git a/client_120.go b/client_120.go index 42b744d..d2efc72 100644 --- a/client_120.go +++ b/client_120.go @@ -23,41 +23,42 @@ func (c *Client) Send(ml ...*Msg) (rerr error) { if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { rerr = errors.Join(rerr, ErrServerNoUnencoded) - m.sendError = SendError{Err: ErrServerNoUnencoded} + m.sendError = &SendError{Reason: ErrNoUnencoded, isTemp: false} continue } } f, err := m.GetSender(false) if err != nil { rerr = errors.Join(rerr, err) - m.sendError = SendError{Err: ErrGetSender, details: []error{err}} + m.sendError = &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err)} continue } rl, err := m.GetRecipients() if err != nil { rerr = errors.Join(rerr, err) - m.sendError = SendError{Err: ErrGetRcpts, details: []error{err}} + m.sendError = &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err)} continue } if err := c.mail(f); err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending MAIL FROM command failed: %w", err)) - m.sendError = SendError{Err: ErrSMTPMailFrom, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} if reserr := c.sc.Reset(); reserr != nil { rerr = errors.Join(rerr, reserr) } continue } failed := false - rse := SendError{} - rse.details = make([]error, 0) + rse := &SendError{} + rse.errlist = make([]error, 0) rse.rcpt = make([]string, 0) for _, r := range rl { if err := c.rcpt(r); err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending RCPT TO command failed: %w", err)) - rse.Err = ErrSMTPRcptTo - rse.details = append(rse.details, err) + rse.Reason = ErrSMTPRcptTo + rse.errlist = append(rse.errlist, err) rse.rcpt = append(rse.rcpt, r) + rse.isTemp = isTempError(err) failed = true } } @@ -71,30 +72,30 @@ func (c *Client) Send(ml ...*Msg) (rerr error) { w, err := c.sc.Data() if err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending DATA command failed: %w", err)) - m.sendError = SendError{Err: ErrSMTPData, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} continue } _, err = m.WriteTo(w) if err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending mail content failed: %w", err)) - m.sendError = SendError{Err: ErrWriteContent, details: []error{err}} + m.sendError = &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} continue } if err := w.Close(); err != nil { rerr = errors.Join(rerr, fmt.Errorf("failed to close DATA writer: %w", err)) - m.sendError = SendError{Err: ErrSMTPDataClose, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} continue } if err := c.Reset(); err != nil { rerr = errors.Join(rerr, fmt.Errorf("sending RSET command failed: %w", err)) - m.sendError = SendError{Err: ErrSMTPReset, details: []error{err}} + m.sendError = &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} continue } if err := c.checkConn(); err != nil { rerr = errors.Join(rerr, fmt.Errorf("failed to check server connection: %w", err)) - m.sendError = SendError{Err: ErrConnCheck, details: []error{err}} + m.sendError = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} } } diff --git a/client_test.go b/client_test.go index edf850b..ae3bcce 100644 --- a/client_test.go +++ b/client_test.go @@ -1030,9 +1030,12 @@ func TestClient_Send_MsgSendError(t *testing.T) { if !m.HasSendError() { t.Errorf("message was expected to have a send error, but didn't") } - se := SendError{Err: ErrSMTPRcptTo} - if !errors.As(m.SendError(), &se) { - t.Errorf("message with broken recipient was expected to return a ErrSMTPRcptTo error, but didn't") + se := &SendError{Reason: ErrSMTPRcptTo} + if !errors.Is(m.SendError(), se) { + t.Errorf("error mismatch, expected: %s, got: %s", se, m.SendError()) + } + if m.SendErrorIsTemp() { + t.Errorf("message was not expected to be a temporary error, but reported as such") } } } diff --git a/msg.go b/msg.go index 82e4739..16b9c9b 100644 --- a/msg.go +++ b/msg.go @@ -966,6 +966,16 @@ func (m *Msg) HasSendError() bool { return m.sendError != nil } +// SendErrorIsTemp returns true if the Msg experienced an error during the message delivery and the +// corresponding error was of temporary nature and should be retried later +func (m *Msg) SendErrorIsTemp() bool { + e, ok := m.sendError.(*SendError) + if !ok { + return false + } + return e.isTemp +} + // SendError returns the senderror field of the Msg func (m *Msg) SendError() error { return m.sendError diff --git a/senderror.go b/senderror.go index f093852..b4fb404 100644 --- a/senderror.go +++ b/senderror.go @@ -5,68 +5,138 @@ package mail import ( - "errors" "fmt" + "strconv" "strings" ) -// List of SendError errors -var ( +// List of SendError reasons +const ( // ErrGetSender is returned if the Msg.GetSender method fails during a Client.Send - ErrGetSender = errors.New("getting sender address") + ErrGetSender SendErrReason = iota // ErrGetRcpts is returned if the Msg.GetRecipients method fails during a Client.Send - ErrGetRcpts = errors.New("getting recipient addresses") + ErrGetRcpts // ErrSMTPMailFrom is returned if the Msg delivery failed when sending the MAIL FROM command // to the sending SMTP server - ErrSMTPMailFrom = errors.New("sending SMTP MAIL FROM command") + ErrSMTPMailFrom // ErrSMTPRcptTo is returned if the Msg delivery failed when sending the RCPT TO command // to the sending SMTP server - ErrSMTPRcptTo = errors.New("sending SMTP RCPT TO command") + ErrSMTPRcptTo // ErrSMTPData is returned if the Msg delivery failed when sending the DATA command // to the sending SMTP server - ErrSMTPData = errors.New("sending SMTP DATA command") + ErrSMTPData // ErrSMTPDataClose is returned if the Msg delivery failed when trying to close the // Client data writer - ErrSMTPDataClose = errors.New("closing SMTP DATA writer") + ErrSMTPDataClose // ErrSMTPReset is returned if the Msg delivery failed when sending the RSET command // to the sending SMTP server - ErrSMTPReset = errors.New("sending SMTP RESET command") + ErrSMTPReset // ErrWriteContent is returned if the Msg delivery failed when sending Msg content // to the Client writer - ErrWriteContent = errors.New("sending message content") + ErrWriteContent // ErrConnCheck is returned if the Msg delivery failed when checking if the SMTP // server connection is still working - ErrConnCheck = errors.New("checking SMTP connection") + ErrConnCheck + + // ErrNoUnencoded is returned if the Msg delivery failed when the Msg is configured for + // unencoded delivery but the server does not support this + ErrNoUnencoded ) // SendError is an error wrapper for delivery errors of the Msg type SendError struct { - Err error - details []error + Reason SendErrReason + isTemp bool + errlist []error rcpt []string } +// SendErrReason represents a comparable reason on why the delivery failed +type SendErrReason int + // Error implements the error interface for the SendError type -func (e SendError) Error() string { +func (e *SendError) Error() string { + if e.Reason > 9 { + return "client_send: unknown error" + } + var em strings.Builder - _, _ = fmt.Fprintf(&em, "client_send: %s", e.Err) - if len(e.details) > 0 { - for i := range e.details { - em.WriteString(fmt.Sprintf(", error_details: %s", e.details[i])) + _, _ = fmt.Fprintf(&em, "client_send: %s", e.Reason) + if len(e.errlist) > 0 { + em.WriteRune(':') + for i := range e.errlist { + em.WriteRune(' ') + em.WriteString(e.errlist[i].Error()) + if i != len(e.errlist)-1 { + em.WriteString(", ") + } } } if len(e.rcpt) > 0 { + em.WriteString(", affected recipient(s): ") for i := range e.rcpt { - em.WriteString(fmt.Sprintf(", rcpt: %s", e.rcpt[i])) + em.WriteString(e.rcpt[i]) + if i != len(e.rcpt)-1 { + em.WriteString(", ") + } } } return em.String() } + +// Is implements the errors.Is functionality and compares the SendErrReason +func (e *SendError) Is(et error) bool { + t, ok := et.(*SendError) + if !ok { + return false + } + return e.Reason == t.Reason && e.isTemp == t.isTemp +} + +// String implements the Stringer interface for the SendErrReason +func (r SendErrReason) String() string { + switch r { + case ErrGetSender: + return "getting sender address" + case ErrGetRcpts: + return "getting recipient addresses" + case ErrSMTPMailFrom: + return "sending SMTP MAIL FROM command" + case ErrSMTPRcptTo: + return "sending SMTP RCPT TO command" + case ErrSMTPData: + return "sending SMTP DATA command" + case ErrSMTPDataClose: + return "closing SMTP DATA writer" + case ErrSMTPReset: + return "sending SMTP RESET command" + case ErrWriteContent: + return "sending message content" + case ErrConnCheck: + return "checking SMTP connection" + case ErrNoUnencoded: + return ErrServerNoUnencoded.Error() + } + return "unknown reason" +} + +// isTempError checks the given SMTP error and returns true if the given error is of temporary nature +// and should be retried +func isTempError(e error) bool { + ec, err := strconv.Atoi(e.Error()[:3]) + if err != nil { + return false + } + if ec >= 400 && ec <= 500 { + return true + } + return false +} diff --git a/senderror_test.go b/senderror_test.go new file mode 100644 index 0000000..0a63f07 --- /dev/null +++ b/senderror_test.go @@ -0,0 +1,55 @@ +package mail + +import ( + "errors" + "testing" +) + +// TestSendError_Error tests the SendError and SendErrReason error handling methods +func TestSendError_Error(t *testing.T) { + tl := []struct { + n string + r SendErrReason + te bool + }{ + {"ErrGetSender/temp", ErrGetSender, true}, + {"ErrGetSender/perm", ErrGetSender, false}, + {"ErrGetRcpts/temp", ErrGetRcpts, true}, + {"ErrGetRcpts/perm", ErrGetRcpts, false}, + {"ErrSMTPMailFrom/temp", ErrSMTPMailFrom, true}, + {"ErrSMTPMailFrom/perm", ErrSMTPMailFrom, false}, + {"ErrSMTPRcptTo/temp", ErrSMTPRcptTo, true}, + {"ErrSMTPRcptTo/perm", ErrSMTPRcptTo, false}, + {"ErrSMTPData/temp", ErrSMTPData, true}, + {"ErrSMTPData/perm", ErrSMTPData, false}, + {"ErrSMTPDataClose/temp", ErrSMTPDataClose, true}, + {"ErrSMTPDataClose/perm", ErrSMTPDataClose, false}, + {"ErrSMTPReset/temp", ErrSMTPReset, true}, + {"ErrSMTPReset/perm", ErrSMTPReset, false}, + {"ErrWriteContent/temp", ErrWriteContent, true}, + {"ErrWriteContent/perm", ErrWriteContent, false}, + {"ErrConnCheck/temp", ErrConnCheck, true}, + {"ErrConnCheck/perm", ErrConnCheck, false}, + {"ErrNoUnencoded/temp", ErrNoUnencoded, true}, + {"ErrNoUnencoded/perm", ErrNoUnencoded, false}, + {"Unknown/temp", 9999, true}, + {"Unknown/perm", 9999, false}, + } + + for _, tt := range tl { + t.Run(tt.n, func(t *testing.T) { + if err := returnSendError(tt.r, tt.te); err != nil { + exp := &SendError{Reason: tt.r, isTemp: tt.te} + if !errors.Is(err, exp) { + t.Errorf("error mismatch, expected: %s (temp: %t), got: %s (temp: %t)", tt.r, tt.te, + exp.Error(), exp.isTemp) + } + } + }) + } +} + +// returnSendError is a helper method to retunr a SendError with a specific reason +func returnSendError(r SendErrReason, t bool) error { + return &SendError{Reason: r, isTemp: t} +} From f6709e90cd45e20ecd61a3ee2c9445ef6f51d6ea Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 1 Jan 2023 14:25:00 +0100 Subject: [PATCH 05/16] Make golangci-lint happy by using `errors.As` instead of using type assertion on error (which can fail on wrapped errors) --- msg.go | 8 ++++---- senderror.go | 9 +++++---- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/msg.go b/msg.go index 16b9c9b..320b427 100644 --- a/msg.go +++ b/msg.go @@ -969,11 +969,11 @@ func (m *Msg) HasSendError() bool { // SendErrorIsTemp returns true if the Msg experienced an error during the message delivery and the // corresponding error was of temporary nature and should be retried later func (m *Msg) SendErrorIsTemp() bool { - e, ok := m.sendError.(*SendError) - if !ok { - return false + var e *SendError + if errors.As(m.sendError, &e) { + return e.isTemp } - return e.isTemp + return false } // SendError returns the senderror field of the Msg diff --git a/senderror.go b/senderror.go index b4fb404..264a318 100644 --- a/senderror.go +++ b/senderror.go @@ -5,6 +5,7 @@ package mail import ( + "errors" "fmt" "strconv" "strings" @@ -94,11 +95,11 @@ func (e *SendError) Error() string { // Is implements the errors.Is functionality and compares the SendErrReason func (e *SendError) Is(et error) bool { - t, ok := et.(*SendError) - if !ok { - return false + var t *SendError + if errors.As(et, &t) { + return e.Reason == t.Reason && e.isTemp == t.isTemp } - return e.Reason == t.Reason && e.isTemp == t.isTemp + return false } // String implements the Stringer interface for the SendErrReason From 43efd6b3a8a7c9340f17096ba306523ef00aa6c4 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 1 Jan 2023 14:25:44 +0100 Subject: [PATCH 06/16] Add missing REUSE header in senderror_test.go --- senderror_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/senderror_test.go b/senderror_test.go index 0a63f07..031f118 100644 --- a/senderror_test.go +++ b/senderror_test.go @@ -1,3 +1,7 @@ +// SPDX-FileCopyrightText: 2022 Winni Neessen +// +// SPDX-License-Identifier: MIT + package mail import ( From 15b3a0028a849cb9e927a049c8d04639d0861fb6 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Sun, 1 Jan 2023 20:35:21 +0100 Subject: [PATCH 07/16] Fix/optimize isTempError method Not that this particular part of the code is performance critical, but I figured that the `strconv.Atoi()` is actually useless in here. Since all we want to know is if the error code from the SMTP server is a 4xx error, we can just check the first rune of the returned error. The `Atoi` provides us with no advantage over the simple rune compare (except of taking about 3ns longer to execute) --- senderror.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/senderror.go b/senderror.go index 264a318..ea6e633 100644 --- a/senderror.go +++ b/senderror.go @@ -7,7 +7,6 @@ package mail import ( "errors" "fmt" - "strconv" "strings" ) @@ -132,12 +131,5 @@ func (r SendErrReason) String() string { // isTempError checks the given SMTP error and returns true if the given error is of temporary nature // and should be retried func isTempError(e error) bool { - ec, err := strconv.Atoi(e.Error()[:3]) - if err != nil { - return false - } - if ec >= 400 && ec <= 500 { - return true - } - return false + return e.Error()[0] == '4' } From 2c7ea3e532583f93e72f4a8a1bb1d88974c2987e Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 12:14:14 +0100 Subject: [PATCH 08/16] More changes in regard to #90 As proposed by @iwittkau the `SendError` type now has a `IsTemp()` method as well indicating to the user if the delivery error is retryable or not. Since we want to use it in the error response from the Client functions like `Send` or `DialAndSend` we need to return the SendError type not only as part of the `*Msg` but also as return value for these methods. Hence, the changes made for #85 been overhauled to return the new error type instead. In the pre Go1.20 version of the `Send()` method we need to return an accumulated version of the SendError type, since we don't have `errors.Join()` and therefore, if more than one error occurred during the delivery we return an ambiguous error reason since we can't tell which of the captured errors is main error. For more details the user can always check the `*Msg.SendError` --- client_119.go | 83 +++++++++++++++++++++++++++++--------------------- client_120.go | 23 +++++++------- client_test.go | 37 ++++++++++++++++++++++ senderror.go | 16 ++++++++-- 4 files changed, 110 insertions(+), 49 deletions(-) diff --git a/client_119.go b/client_119.go index 0f06c6c..465b142 100644 --- a/client_119.go +++ b/client_119.go @@ -7,44 +7,44 @@ package mail -import ( - "fmt" -) - // Send sends out the mail message func (c *Client) Send(ml ...*Msg) error { - var errs []error - if err := c.checkConn(); err != nil { - return fmt.Errorf("failed to send mail: %w", err) + if cerr := c.checkConn(); cerr != nil { + return &SendError{Reason: ErrConnCheck, errlist: []error{cerr}, isTemp: isTempError(cerr)} } + var errs []*SendError for _, m := range ml { m.sendError = nil if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { - errs = append(errs, ErrServerNoUnencoded) - m.sendError = &SendError{Reason: ErrNoUnencoded, isTemp: false} + se := &SendError{Reason: ErrNoUnencoded, isTemp: false} + m.sendError = se + errs = append(errs, se) continue } } f, err := m.GetSender(false) if err != nil { - errs = append(errs, err) - m.sendError = &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err)} + se := &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err)} + m.sendError = se + errs = append(errs, se) continue } rl, err := m.GetRecipients() if err != nil { - m.sendError = &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err)} - errs = append(errs, err) + se := &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err)} + m.sendError = se + errs = append(errs, se) continue } if err := c.mail(f); err != nil { - errs = append(errs, fmt.Errorf("sending MAIL FROM command failed: %w", err)) - m.sendError = &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} + se := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} if reserr := c.sc.Reset(); reserr != nil { - errs = append(errs, reserr) + se.errlist = append(se.errlist, reserr) } + m.sendError = se + errs = append(errs, se) continue } failed := false @@ -53,7 +53,6 @@ func (c *Client) Send(ml ...*Msg) error { rse.rcpt = make([]string, 0) for _, r := range rl { if err := c.rcpt(r); err != nil { - errs = append(errs, fmt.Errorf("sending RCPT TO command failed: %w", err)) rse.Reason = ErrSMTPRcptTo rse.errlist = append(rse.errlist, err) rse.rcpt = append(rse.rcpt, r) @@ -63,51 +62,67 @@ func (c *Client) Send(ml ...*Msg) error { } if failed { if reserr := c.sc.Reset(); reserr != nil { - errs = append(errs, reserr) + rse.errlist = append(rse.errlist, err) } m.sendError = rse + errs = append(errs, rse) continue } w, err := c.sc.Data() if err != nil { - errs = append(errs, fmt.Errorf("sending DATA command failed: %w", err)) - m.sendError = &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} + se := &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} + m.sendError = se + errs = append(errs, se) continue } _, err = m.WriteTo(w) if err != nil { - errs = append(errs, fmt.Errorf("sending mail content failed: %w", err)) - m.sendError = &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} + se := &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} + m.sendError = se + errs = append(errs, se) continue } if err := w.Close(); err != nil { - errs = append(errs, fmt.Errorf("failed to close DATA writer: %w", err)) - m.sendError = &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} + se := &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} + m.sendError = se + errs = append(errs, se) continue } if err := c.Reset(); err != nil { - errs = append(errs, fmt.Errorf("sending RSET command failed: %w", err)) - m.sendError = &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} + se := &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} + m.sendError = se + errs = append(errs, se) continue } if err := c.checkConn(); err != nil { - errs = append(errs, fmt.Errorf("failed to check server connection: %w", err)) - m.sendError = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} + se := &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} + m.sendError = se + errs = append(errs, se) continue } } if len(errs) > 0 { - errtxt := "" - for i := range errs { - errtxt += fmt.Sprintf("%s", errs[i]) - if i < len(errs) { - errtxt += "\n" + if len(errs) > 1 { + re := &SendError{Reason: ErrAmbiguous} + for i := range errs { + for _, e := range errs[i].errlist { + re.errlist = append(re.errlist, e) + } + for _, r := range errs[i].rcpt { + re.rcpt = append(re.rcpt, r) + } } + + // We assume that the isTemp flage from the last error we received should be the + // indicator for the returned isTemp flag as well + re.isTemp = errs[len(errs)-1].isTemp + + return re } - return fmt.Errorf("%s", errtxt) + return errs[0] } return nil } diff --git a/client_120.go b/client_120.go index d2efc72..33be7e3 100644 --- a/client_120.go +++ b/client_120.go @@ -9,40 +9,39 @@ package mail import ( "errors" - "fmt" ) // Send sends out the mail message func (c *Client) Send(ml ...*Msg) (rerr error) { if err := c.checkConn(); err != nil { - rerr = fmt.Errorf("failed to send mail: %w", err) + rerr = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} return } for _, m := range ml { m.sendError = nil if m.encoding == NoEncoding { if ok, _ := c.sc.Extension("8BITMIME"); !ok { - rerr = errors.Join(rerr, ErrServerNoUnencoded) m.sendError = &SendError{Reason: ErrNoUnencoded, isTemp: false} + rerr = errors.Join(rerr, m.sendError) continue } } f, err := m.GetSender(false) if err != nil { - rerr = errors.Join(rerr, err) m.sendError = &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) continue } rl, err := m.GetRecipients() if err != nil { - rerr = errors.Join(rerr, err) m.sendError = &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) continue } if err := c.mail(f); err != nil { - rerr = errors.Join(rerr, fmt.Errorf("sending MAIL FROM command failed: %w", err)) m.sendError = &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) if reserr := c.sc.Reset(); reserr != nil { rerr = errors.Join(rerr, reserr) } @@ -54,7 +53,6 @@ func (c *Client) Send(ml ...*Msg) (rerr error) { rse.rcpt = make([]string, 0) for _, r := range rl { if err := c.rcpt(r); err != nil { - rerr = errors.Join(rerr, fmt.Errorf("sending RCPT TO command failed: %w", err)) rse.Reason = ErrSMTPRcptTo rse.errlist = append(rse.errlist, err) rse.rcpt = append(rse.rcpt, r) @@ -67,35 +65,36 @@ func (c *Client) Send(ml ...*Msg) (rerr error) { rerr = errors.Join(rerr, reserr) } m.sendError = rse + rerr = errors.Join(rerr, m.sendError) continue } w, err := c.sc.Data() if err != nil { - rerr = errors.Join(rerr, fmt.Errorf("sending DATA command failed: %w", err)) m.sendError = &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) continue } _, err = m.WriteTo(w) if err != nil { - rerr = errors.Join(rerr, fmt.Errorf("sending mail content failed: %w", err)) m.sendError = &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) continue } if err := w.Close(); err != nil { - rerr = errors.Join(rerr, fmt.Errorf("failed to close DATA writer: %w", err)) m.sendError = &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) continue } if err := c.Reset(); err != nil { - rerr = errors.Join(rerr, fmt.Errorf("sending RSET command failed: %w", err)) m.sendError = &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) continue } if err := c.checkConn(); err != nil { - rerr = errors.Join(rerr, fmt.Errorf("failed to check server connection: %w", err)) m.sendError = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} + rerr = errors.Join(rerr, m.sendError) } } diff --git a/client_test.go b/client_test.go index ae3bcce..96fd0c2 100644 --- a/client_test.go +++ b/client_test.go @@ -1040,6 +1040,43 @@ func TestClient_Send_MsgSendError(t *testing.T) { } } +// TestClient_DialAndSendWithContext_withSendError tests the Client.DialAndSendWithContext method +// with a broken recipient to make sure that the returned error satisfies the Msg.SendError type +func TestClient_DialAndSendWithContext_withSendError(t *testing.T) { + if os.Getenv("TEST_ALLOW_SEND") == "" { + t.Skipf("TEST_ALLOW_SEND is not set. Skipping mail sending test") + } + m := NewMsg() + _ = m.FromFormat("go-mail Test Mailer", os.Getenv("TEST_FROM")) + _ = m.To("invalid@domain.tld") + m.Subject(fmt.Sprintf("This is a test mail from go-mail/v%s", VERSION)) + m.SetBulk() + m.SetDate() + m.SetMessageID() + m.SetBodyString(TypeTextPlain, "This is a test mail from the go-mail library") + + c, err := getTestConnection(true) + if err != nil { + t.Skipf("failed to create test client: %s. Skipping tests", err) + } + ctx, cfn := context.WithTimeout(context.Background(), DefaultTimeout) + defer cfn() + err = c.DialAndSendWithContext(ctx, m) + if err == nil { + t.Errorf("expected DialAndSendWithContext with broken mail recipient to fail, but didn't") + return + } + var se *SendError + if !errors.As(err, &se) { + t.Errorf("expected *SendError type as returned error, but didn't") + return + } + if se.IsTemp() { + t.Errorf("expected permanent error but IsTemp() returned true") + return + } +} + // getTestConnection takes environment variables to establish a connection to a real // SMTP server to test all functionality that requires a connection func getTestConnection(auth bool) (*Client, error) { diff --git a/senderror.go b/senderror.go index 264a318..79fdac1 100644 --- a/senderror.go +++ b/senderror.go @@ -6,7 +6,6 @@ package mail import ( "errors" - "fmt" "strconv" "strings" ) @@ -50,6 +49,10 @@ const ( // ErrNoUnencoded is returned if the Msg delivery failed when the Msg is configured for // unencoded delivery but the server does not support this ErrNoUnencoded + + // ErrAmbiguous is a generalized delivery error for the SendError type that is + // returned if the exact reason for the delivery failure is ambiguous + ErrAmbiguous ) // SendError is an error wrapper for delivery errors of the Msg @@ -65,12 +68,12 @@ type SendErrReason int // Error implements the error interface for the SendError type func (e *SendError) Error() string { - if e.Reason > 9 { + if e.Reason > 10 { return "client_send: unknown error" } var em strings.Builder - _, _ = fmt.Fprintf(&em, "client_send: %s", e.Reason) + em.WriteString(e.Reason.String()) if len(e.errlist) > 0 { em.WriteRune(':') for i := range e.errlist { @@ -102,6 +105,11 @@ func (e *SendError) Is(et error) bool { return false } +// IsTemp returns true if the delivery error is of temporary nature and can be retried +func (e *SendError) IsTemp() bool { + return e.isTemp +} + // String implements the Stringer interface for the SendErrReason func (r SendErrReason) String() string { switch r { @@ -125,6 +133,8 @@ func (r SendErrReason) String() string { return "checking SMTP connection" case ErrNoUnencoded: return ErrServerNoUnencoded.Error() + case ErrAmbiguous: + return "ambiguous reason, check Msg.SendError for message specific reasons" } return "unknown reason" } From 8463f8524aa8c0e33f9d4bc657eda036d98ef600 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 12:16:47 +0100 Subject: [PATCH 09/16] Fix merge conflict --- senderror.go | 1 - 1 file changed, 1 deletion(-) diff --git a/senderror.go b/senderror.go index 660c189..8f2b9c7 100644 --- a/senderror.go +++ b/senderror.go @@ -6,7 +6,6 @@ package mail import ( "errors" - "fmt" "strings" ) From b8ee25f014117f5d6915668dcf684844b71281ad Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 20:49:35 +0100 Subject: [PATCH 10/16] Simplify error return to make golangci-lint happy --- client_119.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/client_119.go b/client_119.go index 465b142..8347963 100644 --- a/client_119.go +++ b/client_119.go @@ -108,12 +108,8 @@ func (c *Client) Send(ml ...*Msg) error { if len(errs) > 1 { re := &SendError{Reason: ErrAmbiguous} for i := range errs { - for _, e := range errs[i].errlist { - re.errlist = append(re.errlist, e) - } - for _, r := range errs[i].rcpt { - re.rcpt = append(re.rcpt, r) - } + re.errlist = append(re.errlist, errs[i].errlist...) + re.rcpt = append(re.rcpt, errs[i].rcpt...) } // We assume that the isTemp flage from the last error we received should be the From 8d86c3d8b2f3a9b7bf6b90b8dfcffac66b8511db Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 21:55:37 +0100 Subject: [PATCH 11/16] Better test coverage for senderror.go --- senderror.go | 2 +- senderror_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/senderror.go b/senderror.go index 8f2b9c7..da9402a 100644 --- a/senderror.go +++ b/senderror.go @@ -68,7 +68,7 @@ type SendErrReason int // Error implements the error interface for the SendError type func (e *SendError) Error() string { if e.Reason > 10 { - return "client_send: unknown error" + return "unknown reason" } var em strings.Builder diff --git a/senderror_test.go b/senderror_test.go index 031f118..b72a14e 100644 --- a/senderror_test.go +++ b/senderror_test.go @@ -6,6 +6,8 @@ package mail import ( "errors" + "fmt" + "strings" "testing" ) @@ -36,6 +38,8 @@ func TestSendError_Error(t *testing.T) { {"ErrConnCheck/perm", ErrConnCheck, false}, {"ErrNoUnencoded/temp", ErrNoUnencoded, true}, {"ErrNoUnencoded/perm", ErrNoUnencoded, false}, + {"ErrAmbiguous/temp", ErrAmbiguous, true}, + {"ErrAmbiguous/perm", ErrAmbiguous, false}, {"Unknown/temp", 9999, true}, {"Unknown/perm", 9999, false}, } @@ -48,11 +52,37 @@ func TestSendError_Error(t *testing.T) { t.Errorf("error mismatch, expected: %s (temp: %t), got: %s (temp: %t)", tt.r, tt.te, exp.Error(), exp.isTemp) } + if !strings.Contains(fmt.Sprintf("%s", err), tt.r.String()) { + t.Errorf("error string mismatch, expected: %s, got: %s", + tt.r.String(), fmt.Sprintf("%s", err)) + } } }) } } +func TestSendError_IsTemp(t *testing.T) { + var se *SendError + err1 := returnSendError(ErrAmbiguous, true) + if !errors.As(err1, &se) { + t.Errorf("error mismatch, expected error to be of type *SendError") + return + } + if errors.As(err1, &se) && !se.IsTemp() { + t.Errorf("error mismatch, expected temporary error") + return + } + err2 := returnSendError(ErrAmbiguous, false) + if !errors.As(err2, &se) { + t.Errorf("error mismatch, expected error to be of type *SendError") + return + } + if errors.As(err2, &se) && se.IsTemp() { + t.Errorf("error mismatch, expected non-temporary error") + return + } +} + // returnSendError is a helper method to retunr a SendError with a specific reason func returnSendError(r SendErrReason, t bool) error { return &SendError{Reason: r, isTemp: t} From e6b73c23977630a47639973200267eed2f6231d4 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 22:21:23 +0100 Subject: [PATCH 12/16] Update client_119.go Co-authored-by: iwittkau --- client_119.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client_119.go b/client_119.go index 8347963..7fb25df 100644 --- a/client_119.go +++ b/client_119.go @@ -112,7 +112,7 @@ func (c *Client) Send(ml ...*Msg) error { re.rcpt = append(re.rcpt, errs[i].rcpt...) } - // We assume that the isTemp flage from the last error we received should be the + // We assume that the isTemp flag from the last error we received should be the // indicator for the returned isTemp flag as well re.isTemp = errs[len(errs)-1].isTemp From b12b0bb363d0e888b2e93014dc3e149b71f277b2 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 22:22:31 +0100 Subject: [PATCH 13/16] Update client_test.go Co-authored-by: iwittkau --- client_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client_test.go b/client_test.go index 96fd0c2..d7ef11c 100644 --- a/client_test.go +++ b/client_test.go @@ -990,7 +990,7 @@ func TestValidateLine(t *testing.T) { } } -// TestClient_Send_MsgSendError tests the Send() method of Client with a broken and verifies +// TestClient_Send_MsgSendError tests the Client.Send method with a broken recipient and verifies // that the SendError type works properly func TestClient_Send_MsgSendError(t *testing.T) { if os.Getenv("TEST_ALLOW_SEND") == "" { From dbb596893dceabbf5e623bdb4dff7a54f59119e7 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 22:28:39 +0100 Subject: [PATCH 14/16] Update msg.go Co-authored-by: iwittkau --- msg.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msg.go b/msg.go index 320b427..10c0476 100644 --- a/msg.go +++ b/msg.go @@ -961,7 +961,7 @@ func (m *Msg) UpdateReader(r *Reader) { } // HasSendError returns true if the Msg experienced an error during the message delivery and the -// senderror field of the Msg is not nil +// sendError field of the Msg is not nil func (m *Msg) HasSendError() bool { return m.sendError != nil } From 5897bddd11faba58cb8847c1acac579dde978760 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Mon, 2 Jan 2023 22:29:10 +0100 Subject: [PATCH 15/16] Update msg.go Co-authored-by: iwittkau --- msg.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/msg.go b/msg.go index 10c0476..6fe7f5a 100644 --- a/msg.go +++ b/msg.go @@ -976,7 +976,7 @@ func (m *Msg) SendErrorIsTemp() bool { return false } -// SendError returns the senderror field of the Msg +// SendError returns the sendError field of the Msg func (m *Msg) SendError() error { return m.sendError } From 3d0939b597ba346df8fb1e73327e806e3a96bb0c Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 3 Jan 2023 11:19:07 +0100 Subject: [PATCH 16/16] Bump version to 0.3.7 for new release --- doc.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc.go b/doc.go index a9b96de..f2bef6c 100644 --- a/doc.go +++ b/doc.go @@ -6,4 +6,4 @@ package mail // VERSION is used in the default user agent string -const VERSION = "0.3.6" +const VERSION = "0.3.7"