diff --git a/CHANGELOG.md b/CHANGELOG.md index faf7fc0a..d4486f2e 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/error.go b/error.go index c1630f6c..ffea1a74 100644 --- a/error.go +++ b/error.go @@ -29,11 +29,35 @@ 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 { @@ -41,8 +65,24 @@ func (e *LookupError) Error() string { } // 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 diff --git a/error_test.go b/error_test.go index 88f80a3d..611f51d8 100644 --- a/error_test.go +++ b/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,12 +64,55 @@ 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) { @@ -73,7 +120,8 @@ func TestParseError_Code401(t *testing.T) { 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) { diff --git a/verifier.go b/verifier.go index a6c0fff6..6d73b1a0 100644 --- a/verifier.go +++ b/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 }