search

AfterShip / email-verifier

:white_check_mark: A Go library for email verification without sending any emails.
MIT License
1.18k stars 149 forks source link

return HasMXRecord as true when at least one valid mx records exist #94

Closed avivshafir closed 1 year ago

avivshafir commented 1 year ago

Some domains may have several MX records and some of them may be invalid for example nslookup for the domain test-unicode.definbox.com returns "some" valid MX records

I think we should return True in cases when at least one MX record exist and is valid.

Currently the library will return has_mx_records = False in this case although I think this is incorrect behavior Inside the Go library, they made changes to support this case https://github.com/golang/go/blob/dd96ded6c86b8a38f49fa087b758455243a0f08c/src/net/lookup.go#L515

some additional references: https://go-review.googlesource.com/c/go/+/333331 https://github.com/golang/go/issues/46979

I wanted to add a relevant test but if the tests are actually performing the MX check, if the DNS record is fixed in those domains the test will fail, making it unstable. Any thoughts?

lryong commented 1 year ago

Hi @avivshafir , I'm glad to see your PR, I did go and see these references you have mentioned about Golang's own discussion of LookupMX. But for this PR, I don't really recommend this change because I haven't seen enough test cases to illustrate the data for this case. I think if there is a problem with LookupMX, the better way is to modify it in Golang's source code, or in the future it will be modified to the way you mentioned. If your project relies on the MX checking capability of email-verifier, and if you encounter an error while checking, then determine if the error is errMalformedDNSRecordsDetail, or see this issue, where we provideenable_mx capability so that users can extend MX checking on their own. 😃

avivshafir commented 1 year ago

@lryong Thanks for the feedback. The response you are returning right now isn't correct, as a domain may have a valid MX record (HasMXRecord) even though it has invalid others. Just checking the errMalformedDNSRecordsDetail won't be enough for most cases as you don't return the MX records and is also a weird way to validate if MX records exist.

git-hulk commented 1 year ago

@lryong I agree with what @avivshafir mentioned. It should return the correct Mx record even though we expected users to be aware of this error.

lryong commented 1 year ago

Okay, I can understand this point