fix: preserve the underlying cause in LookupError

CHANGELOG.md

@@ -3,6 +3,7 @@
 Unreleased
 ----------
 * Feature: Support a custom DNS resolver for MX and SMTP host lookups via `Resolver()` [#191](https://github.com/AfterShip/email-verifier/pull/191)
+* **Breaking**: `LookupError` wraps the error it was derived from, reachable via `errors.Is`/`errors.As`. `ParseSMTPError` no longer returns a nil `*LookupError` for a non-nil input. Adds an unexported field, so whole-struct comparison against a `LookupError` literal no longer matches [#202](https://github.com/AfterShip/email-verifier/pull/202)
 * **Breaking**: Remove the non-functional Yahoo API verifier; `EnableAPIVerifier(YAHOO)` and the `YAHOO` constant are gone [#198](https://github.com/AfterShip/email-verifier/pull/198)
 * Fix: Yahoo API test panic no longer aborts the test suite [#196](https://github.com/AfterShip/email-verifier/pull/196)
 

error.go

@@ -29,20 +29,60 @@ const (
 type LookupError struct {
 	Message string `json:"message" xml:"message"`
 	Details string `json:"details" xml:"details"`
+
+	// cause is the error this was derived from. It is deliberately unexported
+	// and not serialised; it exists so that callers can use errors.Is and
+	// errors.As to reach the underlying *net.DNSError, *net.OpError or
+	// *textproto.Error instead of having to match on Details.
+	cause error
 }
 
 // newLookupError creates a new LookupError reference and returns it
 func newLookupError(message, details string) *LookupError {
-	return &LookupError{message, details}
+	return &LookupError{Message: message, Details: details}
+}
+
+// withCause records the error this LookupError was derived from, so that
+// errors.Is and errors.As can see through to it.
+func (e *LookupError) withCause(cause error) *LookupError {
+	if e == nil {
+		return nil
+	}
+	e.cause = cause
+	return e
+}
+
+// Unwrap returns the error this LookupError was derived from, if any.
+func (e *LookupError) Unwrap() error {
+	if e == nil {
+		return nil
+	}
+	return e.cause
 }
 
 func (e *LookupError) Error() string {
 	return fmt.Sprintf("%s : %s", e.Message, e.Details)
 }
 
 // ParseSMTPError receives an MX Servers response message
-// and generates the corresponding MX error
+// and generates the corresponding MX error.
+//
+// A non-nil err always yields a non-nil result: an error we cannot classify is
+// reported verbatim rather than discarded. Returning nil here would hand the
+// caller a non-nil error interface wrapping a nil *LookupError, whose Error
+// method then panics.
 func ParseSMTPError(err error) *LookupError {
+	if err == nil {
+		return nil
+	}
+	if le := parseSMTPError(err); le != nil {
+		return le.withCause(err)
+	}
+	errStr := err.Error()
+	return newLookupError(errStr, errStr).withCause(err)
+}
+
+func parseSMTPError(err error) *LookupError {
 	errStr := err.Error()
 
 	// Verify the length of the error before reading nil indexes

error_test.go

@@ -2,9 +2,11 @@ package emailverifier
 
 import (
 	"errors"
+	"net"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestParse550RCPTError(t *testing.T) {
@@ -32,7 +34,8 @@ func TestParseNoMxRecordsFoundError(t *testing.T) {
 	errStr := "No MX records found"
 	err := errors.New(errStr)
 	le := ParseSMTPError(err)
-	assert.Equal(t, &LookupError{Details: errStr, Message: errStr}, le)
+	assert.Equal(t, errStr, le.Message)
+	assert.Equal(t, errStr, le.Details)
 }
 
 func TestParseFullInBoxError(t *testing.T) {
@@ -48,7 +51,8 @@ func TestParseDailSMTPServerError(t *testing.T) {
 	errStr := "Unexpected response dialing SMTP server"
 	err := errors.New(errStr)
 	le := ParseSMTPError(err)
-	assert.Equal(t, &LookupError{Details: errStr, Message: errStr}, le)
+	assert.Equal(t, errStr, le.Message)
+	assert.Equal(t, errStr, le.Details)
 }
 
 func TestParseError_Code550(t *testing.T) {
@@ -60,20 +64,64 @@ func TestParseError_Code550(t *testing.T) {
 	assert.Equal(t, err.Error(), le.Details)
 }
 
-func TestParseError_Code400_Nil(t *testing.T) {
-	errStr := "400"
-	err := errors.New(errStr)
-	le := ParseSMTPError(err)
+// A status line that does not itself indicate a failure is still reported,
+// because ParseSMTPError is only ever reached with a non-nil error. Returning
+// nil used to hand callers a non-nil error interface wrapping a nil
+// *LookupError, and calling Error on that panics.
+func TestParseError_UnclassifiedIsReportedVerbatim(t *testing.T) {
+	// Anything whose status line parses to 400 or below takes this branch. Real
+	// servers do not send a success code as an error, but the guarantee is about
+	// the function's contract: it is only ever called with a non-nil error, so it
+	// must not discard one.
+	for _, reply := range []string{"200 OK", "300 Redirect", "399", "400"} {
+		test := reply
+		t.Run(test, func(tt *testing.T) {
+			cause := errors.New(test)
+
+			le := ParseSMTPError(cause)
+
+			require.NotNil(tt, le)
+			assert.Equal(tt, test, le.Message)
+			assert.Equal(tt, test, le.Details)
+			assert.NotPanics(tt, func() { _ = le.Error() })
+			// the cause has to survive on this branch too, not just the
+			// classified one
+			assert.ErrorIs(tt, error(le), cause)
+		})
+	}
+}
+
+func TestParseSMTPError_NilInput(t *testing.T) {
+	assert.Nil(t, ParseSMTPError(nil))
+}
+
+func TestParseSMTPError_UnwrapsToCause(t *testing.T) {
+	cause := &net.DNSError{Err: "no such host", Name: "example.invalid", IsNotFound: true}
+
+	le := ParseSMTPError(cause)
+	require.NotNil(t, le)
+	assert.Equal(t, ErrNoSuchHost, le.Message)
+
+	// the point of wrapping: callers can inspect the underlying error by type
+	// instead of matching on Details
+	var dnsErr *net.DNSError
+	require.ErrorAs(t, error(le), &dnsErr)
+	assert.True(t, dnsErr.IsNotFound)
+	assert.ErrorIs(t, error(le), cause)
+}
 
-	assert.Equal(t, (*LookupError)(nil), le)
+func TestLookupError_UnwrapNilSafe(t *testing.T) {
+	var le *LookupError
+	assert.NoError(t, le.Unwrap())
 }
 
 func TestParseError_Code401(t *testing.T) {
 	errStr := "401"
 	err := errors.New(errStr)
 	le := ParseSMTPError(err)
 
-	assert.Equal(t, &LookupError{Details: errStr, Message: errStr}, le)
+	assert.Equal(t, errStr, le.Message)
+	assert.Equal(t, errStr, le.Details)
 }
 
 func TestParseError_Code421(t *testing.T) {

verifier.go

@@ -91,7 +91,7 @@ func (v *Verifier) Verify(email string) (*Result, error) {
 		errStr := err.Error()
 		if insContains(errStr, "no such host") {
 			ret.Reachable = reachableNo
-			return &ret, newLookupError(ErrNoSuchHost, errStr)
+			return &ret, newLookupError(ErrNoSuchHost, errStr).withCause(err)
 		}
 		return &ret, err
 	}