From 86e79c4fb188d10ed4be90ddd936fa7bb9c90bfb Mon Sep 17 00:00:00 2001 From: Qingsheng Ma Date: Tue, 25 Aug 2026 15:41:05 +0800 Subject: [PATCH] fix: preserve the underlying cause in LookupError ParseSMTPError replaced the error it was given with one of a fixed set of canned messages, discarding the original. Callers could only recover the reason by substring-matching Details, and could not reach the underlying *net.DNSError, *net.OpError or *textproto.Error at all. LookupError now records the error it was derived from and exposes it via Unwrap, so errors.Is and errors.As work. The cause is unexported and not serialised, so the JSON and XML shapes are unchanged. Also stops ParseSMTPError returning a nil *LookupError. It did so whenever the status line did not itself indicate a failure, but it is only ever reached with a non-nil error, so that discarded a real failure -- and callers doing return &ret, ParseSMTPError(err) were handed a non-nil error interface wrapping a nil pointer, whose Error method panics on the receiver. cmd/apiserver calls err.Error() on exactly that path. An error we cannot classify is now reported verbatim instead. TestParseError_Code400_Nil asserted the nil return, so it is replaced by TestParseError_Code400_ReportedVerbatim. Three tests that compared a whole LookupError with assert.Equal now assert on Message and Details, matching the rest of the file and staying robust to added fields. Coverage 89.6% -> 89.7%. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + error.go | 44 +++++++++++++++++++++++++++++++++-- error_test.go | 64 ++++++++++++++++++++++++++++++++++++++++++++------- verifier.go | 2 +- 4 files changed, 100 insertions(+), 11 deletions(-) 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 }