From 0ea963185534c9cfa2f1d329257f3f56a5358e80 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Tue, 16 Jul 2024 13:07:20 +0200 Subject: [PATCH 01/12] Refactor error handling in message delivery process The error handling process in sending messages has been refactored for better accuracy and efficiency. Instead of immediately joining errors and returning, they are now collected in a slice and joined in a deferred function to ensure all errors are captured before being returned. Furthermore, the SendError structure has been updated to include a reference to the affected message, providing better context for each error. --- client_120.go | 38 +++++++++++++++++++++++--------------- senderror.go | 9 +++++---- 2 files changed, 28 insertions(+), 19 deletions(-) diff --git a/client_120.go b/client_120.go index ed80fee..3a867d9 100644 --- a/client_120.go +++ b/client_120.go @@ -18,25 +18,33 @@ func (c *Client) Send(messages ...*Msg) (returnErr error) { returnErr = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} return } - for _, message := range messages { + + var errs []error + defer func() { + returnErr = errors.Join(errs...) + }() + + for msgid, message := range messages { message.sendError = nil if message.encoding == NoEncoding { if ok, _ := c.smtpClient.Extension("8BITMIME"); !ok { message.sendError = &SendError{Reason: ErrNoUnencoded, isTemp: false} - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) continue } } from, err := message.GetSender(false) if err != nil { - message.sendError = &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + message.sendError = &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: messages[msgid]} + errs = append(errs, message.sendError) continue } rcpts, err := message.GetRecipients() if err != nil { - message.sendError = &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + message.sendError = &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: messages[msgid]} + errs = append(errs, message.sendError) continue } @@ -47,9 +55,9 @@ func (c *Client) Send(messages ...*Msg) (returnErr error) { } if err = c.smtpClient.Mail(from); err != nil { message.sendError = &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - returnErr = errors.Join(returnErr, resetSendErr) + errs = append(errs, resetSendErr) } continue } @@ -70,40 +78,40 @@ func (c *Client) Send(messages ...*Msg) (returnErr error) { } if failed { if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - returnErr = errors.Join(returnErr, resetSendErr) + errs = append(errs, resetSendErr) } message.sendError = rcptSendErr - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) continue } writer, err := c.smtpClient.Data() if err != nil { message.sendError = &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) continue } _, err = message.WriteTo(writer) if err != nil { message.sendError = &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) continue } message.isDelivered = true if err = writer.Close(); err != nil { message.sendError = &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) continue } if err = c.Reset(); err != nil { message.sendError = &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) continue } if err = c.checkConn(); err != nil { message.sendError = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} - returnErr = errors.Join(returnErr, message.sendError) + errs = append(errs, message.sendError) } } diff --git a/senderror.go b/senderror.go index dfe7502..72500cd 100644 --- a/senderror.go +++ b/senderror.go @@ -56,10 +56,11 @@ const ( // SendError is an error wrapper for delivery errors of the Msg type SendError struct { - Reason SendErrReason - isTemp bool - errlist []error - rcpt []string + affectedMsg *Msg + errlist []error + isTemp bool + rcpt []string + Reason SendErrReason } // SendErrReason represents a comparable reason on why the delivery failed From ee726487f17860be9e40b1bf6b0ee0123e84a503 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Wed, 18 Sep 2024 11:06:48 +0200 Subject: [PATCH 02/12] Refactor message sending logic for better modularity Extracted message sending functionality into a new helper function `sendSingleMsg`. This improves the code readability and maintainability by reducing the complexity of the main loop and encapsulating error handling per message. --- client_120.go | 167 ++++++++++++++++++++++++-------------------------- 1 file changed, 79 insertions(+), 88 deletions(-) diff --git a/client_120.go b/client_120.go index 3a867d9..60e8e3c 100644 --- a/client_120.go +++ b/client_120.go @@ -24,96 +24,87 @@ func (c *Client) Send(messages ...*Msg) (returnErr error) { returnErr = errors.Join(errs...) }() - for msgid, message := range messages { - message.sendError = nil - if message.encoding == NoEncoding { - if ok, _ := c.smtpClient.Extension("8BITMIME"); !ok { - message.sendError = &SendError{Reason: ErrNoUnencoded, isTemp: false} - errs = append(errs, message.sendError) - continue - } - } - from, err := message.GetSender(false) - if err != nil { - message.sendError = &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: messages[msgid]} - errs = append(errs, message.sendError) - continue - } - rcpts, err := message.GetRecipients() - if err != nil { - message.sendError = &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: messages[msgid]} - errs = append(errs, message.sendError) - continue - } - - if c.dsn { - if c.dsnmrtype != "" { - c.smtpClient.SetDSNMailReturnOption(string(c.dsnmrtype)) - } - } - if err = c.smtpClient.Mail(from); err != nil { - message.sendError = &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} - errs = append(errs, message.sendError) - if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - errs = append(errs, resetSendErr) - } - continue - } - failed := false - rcptSendErr := &SendError{} - rcptSendErr.errlist = make([]error, 0) - rcptSendErr.rcpt = make([]string, 0) - rcptNotifyOpt := strings.Join(c.dsnrntype, ",") - c.smtpClient.SetDSNRcptNotifyOption(rcptNotifyOpt) - for _, rcpt := range rcpts { - if err = c.smtpClient.Rcpt(rcpt); err != nil { - rcptSendErr.Reason = ErrSMTPRcptTo - rcptSendErr.errlist = append(rcptSendErr.errlist, err) - rcptSendErr.rcpt = append(rcptSendErr.rcpt, rcpt) - rcptSendErr.isTemp = isTempError(err) - failed = true - } - } - if failed { - if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - errs = append(errs, resetSendErr) - } - message.sendError = rcptSendErr - errs = append(errs, message.sendError) - continue - } - writer, err := c.smtpClient.Data() - if err != nil { - message.sendError = &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} - errs = append(errs, message.sendError) - continue - } - _, err = message.WriteTo(writer) - if err != nil { - message.sendError = &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} - errs = append(errs, message.sendError) - continue - } - message.isDelivered = true - - if err = writer.Close(); err != nil { - message.sendError = &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} - errs = append(errs, message.sendError) - continue - } - - if err = c.Reset(); err != nil { - message.sendError = &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} - errs = append(errs, message.sendError) - continue - } - if err = c.checkConn(); err != nil { - message.sendError = &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} - errs = append(errs, message.sendError) + for _, message := range messages { + if sendErr := c.sendSingleMsg(message); sendErr != nil { + message.sendError = sendErr + errs = append(errs, sendErr) } } return } + +// sendSingleMsg sends out a single message and returns an error if the transmission/delivery fails. +// It is invoked by the public Send methods +func (c *Client) sendSingleMsg(message *Msg) error { + if message.encoding == NoEncoding { + if ok, _ := c.smtpClient.Extension("8BITMIME"); !ok { + return &SendError{Reason: ErrNoUnencoded, isTemp: false} + } + } + from, err := message.GetSender(false) + if err != nil { + return &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} + } + rcpts, err := message.GetRecipients() + if err != nil { + return &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} + } + + if c.dsn { + if c.dsnmrtype != "" { + c.smtpClient.SetDSNMailReturnOption(string(c.dsnmrtype)) + } + } + if err = c.smtpClient.Mail(from); err != nil { + retError := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} + if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { + retError.errlist = append(retError.errlist, resetSendErr) + } + return retError + } + failed := false + rcptSendErr := &SendError{} + rcptSendErr.errlist = make([]error, 0) + rcptSendErr.rcpt = make([]string, 0) + rcptNotifyOpt := strings.Join(c.dsnrntype, ",") + c.smtpClient.SetDSNRcptNotifyOption(rcptNotifyOpt) + for _, rcpt := range rcpts { + if err = c.smtpClient.Rcpt(rcpt); err != nil { + rcptSendErr.Reason = ErrSMTPRcptTo + rcptSendErr.errlist = append(rcptSendErr.errlist, err) + rcptSendErr.rcpt = append(rcptSendErr.rcpt, rcpt) + rcptSendErr.isTemp = isTempError(err) + failed = true + } + } + if failed { + if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { + rcptSendErr.errlist = append(rcptSendErr.errlist, resetSendErr) + } + return rcptSendErr + } + writer, err := c.smtpClient.Data() + if err != nil { + return &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} + } + _, err = message.WriteTo(writer) + if err != nil { + return &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} + } + message.isDelivered = true + + if err = writer.Close(); err != nil { + return &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} + } + + if err = c.Reset(); err != nil { + return &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} + } + if err = c.checkConn(); err != nil { + return &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} + } + return nil +} From 8ee37abca2f15097e1a8315f2ee94b4cb0a50478 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Wed, 18 Sep 2024 12:29:42 +0200 Subject: [PATCH 03/12] Refactor error handling in message sending loop Changed from range over messages to range with index to correctly update sendError field in the original messages slice. This prevents shadowing issues and ensures proper error logging for each message. --- client_120.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client_120.go b/client_120.go index 60e8e3c..757a951 100644 --- a/client_120.go +++ b/client_120.go @@ -24,9 +24,9 @@ func (c *Client) Send(messages ...*Msg) (returnErr error) { returnErr = errors.Join(errs...) }() - for _, message := range messages { + for id, message := range messages { if sendErr := c.sendSingleMsg(message); sendErr != nil { - message.sendError = sendErr + messages[id].sendError = sendErr errs = append(errs, sendErr) } } From 2e7156182abd1bfc5127909cee86747689131a03 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 19 Sep 2024 10:35:32 +0200 Subject: [PATCH 04/12] Rename variable 'failed' to 'hasError' for clarity Renamed the variable 'failed' to 'hasError' to better reflect its purpose and improve code readability. This change helps in understanding that the variable indicates the presence of an error rather than a generic failure. --- client_120.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/client_120.go b/client_120.go index 757a951..8eb584a 100644 --- a/client_120.go +++ b/client_120.go @@ -65,7 +65,7 @@ func (c *Client) sendSingleMsg(message *Msg) error { } return retError } - failed := false + hasError := false rcptSendErr := &SendError{} rcptSendErr.errlist = make([]error, 0) rcptSendErr.rcpt = make([]string, 0) @@ -77,10 +77,10 @@ func (c *Client) sendSingleMsg(message *Msg) error { rcptSendErr.errlist = append(rcptSendErr.errlist, err) rcptSendErr.rcpt = append(rcptSendErr.rcpt, rcpt) rcptSendErr.isTemp = isTempError(err) - failed = true + hasError = true } } - if failed { + if hasError { if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { rcptSendErr.errlist = append(rcptSendErr.errlist, resetSendErr) } From 277ae9be191262701ab8f5f81ca51c577674345e Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 19 Sep 2024 10:56:01 +0200 Subject: [PATCH 05/12] Refactor message sending logic Consolidated the message sending logic into a single `sendSingleMsg` function to reduce duplication and improve code maintainability. This change simplifies the `Send` method in multiple Go version files by removing redundant code and calling the new helper function instead. --- client.go | 75 +++++++++++++++++++++++++++++++++++++++ client_119.go | 98 ++------------------------------------------------- client_120.go | 76 --------------------------------------- 3 files changed, 77 insertions(+), 172 deletions(-) diff --git a/client.go b/client.go index 1d175d1..56a82a3 100644 --- a/client.go +++ b/client.go @@ -787,3 +787,78 @@ func (c *Client) auth() error { } return nil } + +// sendSingleMsg sends out a single message and returns an error if the transmission/delivery fails. +// It is invoked by the public Send methods +func (c *Client) sendSingleMsg(message *Msg) error { + if message.encoding == NoEncoding { + if ok, _ := c.smtpClient.Extension("8BITMIME"); !ok { + return &SendError{Reason: ErrNoUnencoded, isTemp: false} + } + } + from, err := message.GetSender(false) + if err != nil { + return &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} + } + rcpts, err := message.GetRecipients() + if err != nil { + return &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} + } + + if c.dsn { + if c.dsnmrtype != "" { + c.smtpClient.SetDSNMailReturnOption(string(c.dsnmrtype)) + } + } + if err = c.smtpClient.Mail(from); err != nil { + retError := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} + if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { + retError.errlist = append(retError.errlist, resetSendErr) + } + return retError + } + hasError := false + rcptSendErr := &SendError{} + rcptSendErr.errlist = make([]error, 0) + rcptSendErr.rcpt = make([]string, 0) + rcptNotifyOpt := strings.Join(c.dsnrntype, ",") + c.smtpClient.SetDSNRcptNotifyOption(rcptNotifyOpt) + for _, rcpt := range rcpts { + if err = c.smtpClient.Rcpt(rcpt); err != nil { + rcptSendErr.Reason = ErrSMTPRcptTo + rcptSendErr.errlist = append(rcptSendErr.errlist, err) + rcptSendErr.rcpt = append(rcptSendErr.rcpt, rcpt) + rcptSendErr.isTemp = isTempError(err) + hasError = true + } + } + if hasError { + if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { + rcptSendErr.errlist = append(rcptSendErr.errlist, resetSendErr) + } + return rcptSendErr + } + writer, err := c.smtpClient.Data() + if err != nil { + return &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} + } + _, err = message.WriteTo(writer) + if err != nil { + return &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} + } + message.isDelivered = true + + if err = writer.Close(); err != nil { + return &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} + } + + if err = c.Reset(); err != nil { + return &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} + } + if err = c.checkConn(); err != nil { + return &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} + } + return nil +} diff --git a/client_119.go b/client_119.go index 52e4b3f..fb94d99 100644 --- a/client_119.go +++ b/client_119.go @@ -7,8 +7,6 @@ package mail -import "strings" - // Send sends out the mail message func (c *Client) Send(messages ...*Msg) error { if cerr := c.checkConn(); cerr != nil { @@ -16,101 +14,9 @@ func (c *Client) Send(messages ...*Msg) error { } var errs []*SendError for _, message := range messages { - message.sendError = nil - if message.encoding == NoEncoding { - if ok, _ := c.smtpClient.Extension("8BITMIME"); !ok { - sendErr := &SendError{Reason: ErrNoUnencoded, isTemp: false} - message.sendError = sendErr - errs = append(errs, sendErr) - continue - } - } - from, err := message.GetSender(false) - if err != nil { - sendErr := &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err)} - message.sendError = sendErr + if sendErr := c.sendSingleMsg(message); sendErr != nil { + messages[id].sendError = sendErr errs = append(errs, sendErr) - continue - } - rcpts, err := message.GetRecipients() - if err != nil { - sendErr := &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err)} - message.sendError = sendErr - errs = append(errs, sendErr) - continue - } - - if c.dsn { - if c.dsnmrtype != "" { - c.smtpClient.SetDSNMailReturnOption(string(c.dsnmrtype)) - } - } - if err = c.smtpClient.Mail(from); err != nil { - sendErr := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} - if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - sendErr.errlist = append(sendErr.errlist, resetSendErr) - } - message.sendError = sendErr - errs = append(errs, sendErr) - continue - } - failed := false - rcptSendErr := &SendError{} - rcptSendErr.errlist = make([]error, 0) - rcptSendErr.rcpt = make([]string, 0) - rcptNotifyOpt := strings.Join(c.dsnrntype, ",") - c.smtpClient.SetDSNRcptNotifyOption(rcptNotifyOpt) - for _, rcpt := range rcpts { - if err = c.smtpClient.Rcpt(rcpt); err != nil { - rcptSendErr.Reason = ErrSMTPRcptTo - rcptSendErr.errlist = append(rcptSendErr.errlist, err) - rcptSendErr.rcpt = append(rcptSendErr.rcpt, rcpt) - rcptSendErr.isTemp = isTempError(err) - failed = true - } - } - if failed { - if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - rcptSendErr.errlist = append(rcptSendErr.errlist, err) - } - message.sendError = rcptSendErr - errs = append(errs, rcptSendErr) - continue - } - writer, err := c.smtpClient.Data() - if err != nil { - sendErr := &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} - message.sendError = sendErr - errs = append(errs, sendErr) - continue - } - _, err = message.WriteTo(writer) - if err != nil { - sendErr := &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} - message.sendError = sendErr - errs = append(errs, sendErr) - continue - } - message.isDelivered = true - - if err = writer.Close(); err != nil { - sendErr := &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} - message.sendError = sendErr - errs = append(errs, sendErr) - continue - } - - if err = c.Reset(); err != nil { - sendErr := &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} - message.sendError = sendErr - errs = append(errs, sendErr) - continue - } - if err = c.checkConn(); err != nil { - sendErr := &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} - message.sendError = sendErr - errs = append(errs, sendErr) - continue } } diff --git a/client_120.go b/client_120.go index 8eb584a..4f82aa7 100644 --- a/client_120.go +++ b/client_120.go @@ -9,7 +9,6 @@ package mail import ( "errors" - "strings" ) // Send sends out the mail message @@ -33,78 +32,3 @@ func (c *Client) Send(messages ...*Msg) (returnErr error) { return } - -// sendSingleMsg sends out a single message and returns an error if the transmission/delivery fails. -// It is invoked by the public Send methods -func (c *Client) sendSingleMsg(message *Msg) error { - if message.encoding == NoEncoding { - if ok, _ := c.smtpClient.Extension("8BITMIME"); !ok { - return &SendError{Reason: ErrNoUnencoded, isTemp: false} - } - } - from, err := message.GetSender(false) - if err != nil { - return &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} - } - rcpts, err := message.GetRecipients() - if err != nil { - return &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} - } - - if c.dsn { - if c.dsnmrtype != "" { - c.smtpClient.SetDSNMailReturnOption(string(c.dsnmrtype)) - } - } - if err = c.smtpClient.Mail(from); err != nil { - retError := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} - if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - retError.errlist = append(retError.errlist, resetSendErr) - } - return retError - } - hasError := false - rcptSendErr := &SendError{} - rcptSendErr.errlist = make([]error, 0) - rcptSendErr.rcpt = make([]string, 0) - rcptNotifyOpt := strings.Join(c.dsnrntype, ",") - c.smtpClient.SetDSNRcptNotifyOption(rcptNotifyOpt) - for _, rcpt := range rcpts { - if err = c.smtpClient.Rcpt(rcpt); err != nil { - rcptSendErr.Reason = ErrSMTPRcptTo - rcptSendErr.errlist = append(rcptSendErr.errlist, err) - rcptSendErr.rcpt = append(rcptSendErr.rcpt, rcpt) - rcptSendErr.isTemp = isTempError(err) - hasError = true - } - } - if hasError { - if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { - rcptSendErr.errlist = append(rcptSendErr.errlist, resetSendErr) - } - return rcptSendErr - } - writer, err := c.smtpClient.Data() - if err != nil { - return &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} - } - _, err = message.WriteTo(writer) - if err != nil { - return &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} - } - message.isDelivered = true - - if err = writer.Close(); err != nil { - return &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} - } - - if err = c.Reset(); err != nil { - return &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} - } - if err = c.checkConn(); err != nil { - return &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} - } - return nil -} From 3bdb6f7ccab55f41fd6960b047459e60e02ef38c Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 19 Sep 2024 10:59:22 +0200 Subject: [PATCH 06/12] Refactor variable naming in Send method Renamed variable `cerr` to `err` for consistency. This improves readability and standardizes error variable naming within the method. --- client_119.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client_119.go b/client_119.go index fb94d99..8084913 100644 --- a/client_119.go +++ b/client_119.go @@ -9,8 +9,8 @@ package mail // Send sends out the mail message func (c *Client) Send(messages ...*Msg) error { - if cerr := c.checkConn(); cerr != nil { - return &SendError{Reason: ErrConnCheck, errlist: []error{cerr}, isTemp: isTempError(cerr)} + if err := c.checkConn(); err != nil { + return &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} } var errs []*SendError for _, message := range messages { From 69e211682c4a49b03340322e27410ca9e3ea737d Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 19 Sep 2024 11:46:53 +0200 Subject: [PATCH 07/12] Add GetMessageID method to Msg Introduced GetMessageID to retrieve the Message ID from the Msg --- msg.go | 11 +++++++++++ msg_test.go | 15 +++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/msg.go b/msg.go index a909d04..11d8ab1 100644 --- a/msg.go +++ b/msg.go @@ -476,6 +476,17 @@ func (m *Msg) SetMessageID() { m.SetMessageIDWithValue(messageID) } +// GetMessageID returns the message ID of the Msg as string value. If no message ID +// is set, an empty string will be returned +func (m *Msg) GetMessageID() string { + if msgidheader, ok := m.genHeader[HeaderMessageID]; ok { + if len(msgidheader) > 0 { + return msgidheader[0] + } + } + return "" +} + // SetMessageIDWithValue sets the message id for the mail func (m *Msg) SetMessageIDWithValue(messageID string) { m.SetGenHeader(HeaderMessageID, fmt.Sprintf("<%s>", messageID)) diff --git a/msg_test.go b/msg_test.go index 16cd196..0656ff2 100644 --- a/msg_test.go +++ b/msg_test.go @@ -805,6 +805,21 @@ func TestMsg_SetMessageIDRandomness(t *testing.T) { } } +func TestMsg_GetMessageID(t *testing.T) { + expected := "this.is.a.message.id" + msg := NewMsg() + msg.SetMessageIDWithValue(expected) + val := msg.GetMessageID() + if !strings.EqualFold(val, fmt.Sprintf("<%s>", expected)) { + t.Errorf("GetMessageID() failed. Expected: %s, got: %s", fmt.Sprintf("<%s>", expected), val) + } + msg.genHeader[HeaderMessageID] = nil + val = msg.GetMessageID() + if val != "" { + t.Errorf("GetMessageID() failed. Expected empty string, got: %s", val) + } +} + // TestMsg_FromFormat tests the FromFormat and EnvelopeFrom methods for the Msg object func TestMsg_FromFormat(t *testing.T) { tests := []struct { From 0239318d949d6412bae106ca90ce4ebf8846b0b3 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 19 Sep 2024 12:09:23 +0200 Subject: [PATCH 08/12] Add affectedMsg field to SendError struct Included the affected message in the SendError struct for better error tracking and debugging. This enhancement ensures that any errors encountered during the sending process can be directly associated with the specific message that caused them. --- client.go | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/client.go b/client.go index 56a82a3..f8519b4 100644 --- a/client.go +++ b/client.go @@ -793,7 +793,7 @@ func (c *Client) auth() error { func (c *Client) sendSingleMsg(message *Msg) error { if message.encoding == NoEncoding { if ok, _ := c.smtpClient.Extension("8BITMIME"); !ok { - return &SendError{Reason: ErrNoUnencoded, isTemp: false} + return &SendError{Reason: ErrNoUnencoded, isTemp: false, affectedMsg: message} } } from, err := message.GetSender(false) @@ -813,14 +813,15 @@ func (c *Client) sendSingleMsg(message *Msg) error { } } if err = c.smtpClient.Mail(from); err != nil { - retError := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err)} + retError := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { retError.errlist = append(retError.errlist, resetSendErr) } return retError } hasError := false - rcptSendErr := &SendError{} + rcptSendErr := &SendError{affectedMsg: message} rcptSendErr.errlist = make([]error, 0) rcptSendErr.rcpt = make([]string, 0) rcptNotifyOpt := strings.Join(c.dsnrntype, ",") @@ -842,23 +843,28 @@ func (c *Client) sendSingleMsg(message *Msg) error { } writer, err := c.smtpClient.Data() if err != nil { - return &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err)} + return &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} } _, err = message.WriteTo(writer) if err != nil { - return &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err)} + return &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} } message.isDelivered = true if err = writer.Close(); err != nil { - return &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err)} + return &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} } if err = c.Reset(); err != nil { - return &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err)} + return &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} } if err = c.checkConn(); err != nil { - return &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err)} + return &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message} } return nil } From f469ba977d299cb17e33fc1b0f025769648cb7f1 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 19 Sep 2024 12:12:48 +0200 Subject: [PATCH 09/12] Add affected message ID to error log Include the affected message ID in the error message to provide more context for debugging. This change ensures that each error log contains essential information about the specific message associated with the error. --- senderror.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/senderror.go b/senderror.go index 72500cd..494bfd3 100644 --- a/senderror.go +++ b/senderror.go @@ -93,6 +93,11 @@ func (e *SendError) Error() string { } } } + if e.affectedMsg != nil && e.affectedMsg.GetMessageID() != "" { + errMessage.WriteString(", affected message ID: ") + errMessage.WriteString(e.affectedMsg.GetMessageID()) + } + return errMessage.String() } From 508a2f2a6c02ce4e72c14db7259fbe910769e183 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Thu, 19 Sep 2024 15:21:17 +0200 Subject: [PATCH 10/12] Refactor connection handling in tests Replaced `getTestConnection` with `getTestConnectionNoTestPort` for tests that skip port configuration, improving connection setup flexibility. Added environment variable support for setting ports in `getTestClient` to streamline port configuration in tests. --- client_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/client_test.go b/client_test.go index 0ad309a..efccea4 100644 --- a/client_test.go +++ b/client_test.go @@ -629,7 +629,7 @@ func TestClient_DialWithContext(t *testing.T) { // TestClient_DialWithContext_Fallback tests the Client.DialWithContext method with the fallback // port functionality func TestClient_DialWithContext_Fallback(t *testing.T) { - c, err := getTestConnection(true) + c, err := getTestConnectionNoTestPort(true) if err != nil { t.Skipf("failed to create test client: %s. Skipping tests", err) } @@ -1302,6 +1302,50 @@ func getTestConnection(auth bool) (*Client, error) { return c, nil } +// getTestConnectionNoTestPort takes environment variables (except the port) to establish a +// connection to a real SMTP server to test all functionality that requires a connection +func getTestConnectionNoTestPort(auth bool) (*Client, error) { + if os.Getenv("TEST_SKIP_ONLINE") != "" { + return nil, fmt.Errorf("env variable TEST_SKIP_ONLINE is set. Skipping online tests") + } + th := os.Getenv("TEST_HOST") + if th == "" { + return nil, fmt.Errorf("no TEST_HOST set") + } + sv := false + if sve := os.Getenv("TEST_TLS_SKIP_VERIFY"); sve != "" { + sv = true + } + c, err := NewClient(th) + if err != nil { + return c, err + } + c.tlsconfig.InsecureSkipVerify = sv + if auth { + st := os.Getenv("TEST_SMTPAUTH_TYPE") + if st != "" { + c.SetSMTPAuth(SMTPAuthType(st)) + } + u := os.Getenv("TEST_SMTPAUTH_USER") + if u != "" { + c.SetUsername(u) + } + p := os.Getenv("TEST_SMTPAUTH_PASS") + if p != "" { + c.SetPassword(p) + } + // We don't want to log authentication data in tests + c.SetDebugLog(false) + } + if err := c.DialWithContext(context.Background()); err != nil { + return c, fmt.Errorf("connection to test server failed: %w", err) + } + if err := c.Close(); err != nil { + return c, fmt.Errorf("disconnect from test server failed: %w", err) + } + return c, nil +} + // getTestClient takes environment variables to establish a client without connecting // to the SMTP server func getTestClient(auth bool) (*Client, error) { @@ -1357,7 +1401,14 @@ func getTestConnectionWithDSN(auth bool) (*Client, error) { if th == "" { return nil, fmt.Errorf("no TEST_HOST set") } - c, err := NewClient(th, WithDSN()) + tp := 25 + if tps := os.Getenv("TEST_PORT"); tps != "" { + tpi, err := strconv.Atoi(tps) + if err == nil { + tp = tpi + } + } + c, err := NewClient(th, WithDSN(), WithPort(tp)) if err != nil { return c, err } From fcbd202595a4611baa1621a684d9baf30512b923 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Fri, 20 Sep 2024 10:30:30 +0200 Subject: [PATCH 11/12] Add test for IsTemp method on nil SendError This commit introduces a new test case to verify the behavior of the IsTemp method when called on a nil SendError instance. It ensures that the method returns false as expected, improving test coverage for edge cases. --- senderror_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/senderror_test.go b/senderror_test.go index e83df00..789b290 100644 --- a/senderror_test.go +++ b/senderror_test.go @@ -83,6 +83,13 @@ func TestSendError_IsTemp(t *testing.T) { } } +func TestSendError_IsTempNil(t *testing.T) { + var se *SendError + if se.IsTemp() { + t.Error("expected false on nil-senderror") + } +} + // 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 d400379e2f09747484d21b0c2c1be7267a5e5d58 Mon Sep 17 00:00:00 2001 From: Winni Neessen Date: Fri, 20 Sep 2024 10:31:19 +0200 Subject: [PATCH 12/12] Refactor error handling for SendError struct Reformat the construction of SendError objects for better readability. This improves the clarity and maintainability of the error handling code within the client.go file. --- client.go | 48 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 32 insertions(+), 16 deletions(-) diff --git a/client.go b/client.go index f8519b4..fac9a34 100644 --- a/client.go +++ b/client.go @@ -798,13 +798,17 @@ func (c *Client) sendSingleMsg(message *Msg) error { } from, err := message.GetSender(false) if err != nil { - return &SendError{Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + return &SendError{ + Reason: ErrGetSender, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } } rcpts, err := message.GetRecipients() if err != nil { - return &SendError{Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + return &SendError{ + Reason: ErrGetRcpts, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } } if c.dsn { @@ -813,8 +817,10 @@ func (c *Client) sendSingleMsg(message *Msg) error { } } if err = c.smtpClient.Mail(from); err != nil { - retError := &SendError{Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + retError := &SendError{ + Reason: ErrSMTPMailFrom, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } if resetSendErr := c.smtpClient.Reset(); resetSendErr != nil { retError.errlist = append(retError.errlist, resetSendErr) } @@ -843,28 +849,38 @@ func (c *Client) sendSingleMsg(message *Msg) error { } writer, err := c.smtpClient.Data() if err != nil { - return &SendError{Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + return &SendError{ + Reason: ErrSMTPData, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } } _, err = message.WriteTo(writer) if err != nil { - return &SendError{Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + return &SendError{ + Reason: ErrWriteContent, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } } message.isDelivered = true if err = writer.Close(); err != nil { - return &SendError{Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + return &SendError{ + Reason: ErrSMTPDataClose, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } } if err = c.Reset(); err != nil { - return &SendError{Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + return &SendError{ + Reason: ErrSMTPReset, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } } if err = c.checkConn(); err != nil { - return &SendError{Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err), - affectedMsg: message} + return &SendError{ + Reason: ErrConnCheck, errlist: []error{err}, isTemp: isTempError(err), + affectedMsg: message, + } } return nil }