adding Enable/DisableMXCheck() option - #188
Conversation
NeoCN
left a comment
There was a problem hiding this comment.
Thanks for picking this up. Defaulting mxCheckEnabled to true keeps existing behaviour
untouched, which makes the change easy to reason about, and it mirrors the existing
EnableSMTPCheck/DisableCatchAllCheck pattern. One request here, one inline.
Please set Reachable on the SMTP error path
Right now DisableMXCheck() + EnableSMTPCheck() reports reachable: "unknown" for a
domain that doesn't exist, where the default path reports "no".
The no such host → Reachable: "no" mapping added in #139 hangs off an error returned by
CheckMX:
Lines 90 to 98 in fee1dc6
With MX checking disabled CheckMX returns a nil error, so that branch never runs.
CheckSMTP then fails on its own MX lookup with the same underlying condition, but the SMTP
error path returns without assigning Reachable, leaving the initial unknown:
Lines 100 to 106 in fee1dc6
TestCheckEmailOK_SMTPHostNotExists won't catch this, since it runs with the MX check at
its default — so a case for DisableMXCheck() + EnableSMTPCheck() against a nonexistent
domain would be worth adding alongside.
smtp, err := v.CheckSMTP(syntax.Domain, syntax.Username)
if err != nil {
+ if le, ok := err.(*LookupError); ok && le != nil && le.Message == ErrNoSuchHost {
+ ret.Reachable = reachableNo
+ }
return &ret, err
}The type assertion rather than insContains(err.Error(), ...) is deliberate:
ParseSMTPError can return a nil *LookupError (locked in by TestParseError_Code400_Nil),
which CheckSMTP then wraps in a non-nil error — calling .Error() on that would panic
inside Verify. The existing insContains at L93 is fine, because CheckMX returns a raw
*net.DNSError.
Worth noting this assignment is useful independently of your change: the same path is
already reachable today with a domain whose MX target doesn't resolve, where CheckMX
succeeds and the failure surfaces during the dial instead. Your flag makes it the common
path rather than a corner case.
| return v | ||
| } | ||
|
|
||
| // DisableMXCheck disables MX record check by dns |
There was a problem hiding this comment.
Worth documenting the scope here: this flag can't skip the MX lookup when the SMTP check is
on. newSMTPClient does its own net.LookupMX and needs the result to know where to
connect, with no A-record fallback:
Lines 116 to 124 in fee1dc6
So DisableMXCheck().EnableSMTPCheck() still pays for the DNS round trip, and reports
has_mx_records: false next to a possibly-true smtp.host_exists. That's inherent rather
than fixable, but saying so here would save the next reader a trip through smtp.go:
| // DisableMXCheck disables MX record check by dns | |
| // DisableMXCheck disables the standalone MX record check performed by Verify. | |
| // Note this does not prevent the MX lookup inside the SMTP check, which needs | |
| // MX records to locate a mail server. |
Adding
EnableMXCheck()andDisableMXCheck()usingmxCheckEnabled.It is enabled by default to keep existing behavior.
Code for Issue #92