Proposal to let the caller analyse the reason for ambiguous verification results #55

Closed
reinhard-mueller wants to merge 0 commits from master into master
reinhard-mueller commented 2021-02-22 15:01:50 +01:00 (Migrated from github.com)

This is my proposal for allowing the caller of validate_email_or_fail to analyze the reasons for ambiguous verification results and, if desired, react appropriately.

It adds two new parameters:

raise_communication_errors can be set to True to raise an SMTPCommunicationError exception in case we don't even get to the RCPT TO step in the SMTP communication. The exception contains all information to analyze the exact error code and message. If the parameter is set to False (the default), such a case is considered an ambiguous verification result, and the check function returns None.

raise_temporary_errors can be set to True to raise an SMTPTemporaryError exception in case the RCPT TO command is answered with a 4xx status code. The exception contains all information to analyze the exact error code and message. If the parameter is set to False (the default), such a case is considered an ambiguous verification result, and the check function returns None.

Comments, bug reports, and suggestions for improvements are welcome!

This is my proposal for allowing the caller of `validate_email_or_fail` to analyze the reasons for ambiguous verification results and, if desired, react appropriately. It adds two new parameters: `raise_communication_errors` can be set to `True` to raise an `SMTPCommunicationError` exception in case we don't even get to the `RCPT TO` step in the SMTP communication. The exception contains all information to analyze the exact error code and message. If the parameter is set to `False` (the default), such a case is considered an ambiguous verification result, and the check function returns `None`. `raise_temporary_errors` can be set to `True` to raise an `SMTPTemporaryError` exception in case the `RCPT TO` command is answered with a 4xx status code. The exception contains all information to analyze the exact error code and message. If the parameter is set to `False` (the default), such a case is considered an ambiguous verification result, and the check function returns `None`. Comments, bug reports, and suggestions for improvements are welcome!
karolyi (Migrated from github.com) requested changes 2021-02-22 18:29:49 +01:00
karolyi (Migrated from github.com) left a comment

I am not sure you need this much changes, especially with the added new parameters. You also unified functions that were intentionally kept separate for clarity.

All in all, I like the new exceptions, but I can't help but feel you overengineered it.

I am not sure you need this much changes, especially with the added new parameters. You also unified functions that were intentionally kept separate for clarity. All in all, I like the new exceptions, but I can't help but feel you overengineered it.
@ -172,4 +58,3 @@
The package contains an auto-updater for downloading and updating the built-in blacklist.txt. It will run on each module load (and installation), but will try to update the content only if the file is older than 5 days, and if the content is not the same that's already downloaded.
The update can be triggered manually::
karolyi (Migrated from github.com) commented 2021-02-22 18:28:04 +01:00

I am not sure you need this. The function returning True/False/None doesn't need this, and in case of the function that raises errors, you should just simply raise the ambigious exceptions. No extra parameters needed.

I am not sure you need this. The function returning `True/False/None` doesn't need this, and in case of the function that raises errors, you should just simply raise the ambigious exceptions. No extra parameters needed.
@ -0,0 +75,4 @@
self, local_hostname: str, timeout: float, debug: bool,
raise_communication_errors: bool,
raise_temporary_errors: bool,
sender: str, recip: str):
karolyi (Migrated from github.com) commented 2021-02-22 18:24:52 +01:00

Do you handle 3xx responses here? Not sure if it doesn't need to be an error, but since they're not handled, I'd keep the original here.

Do you handle 3xx responses here? Not sure if it doesn't need to be an error, but since they're not handled, I'd keep the original here.
@ -0,0 +92,4 @@
# Avoid error on close() after unsuccessful connect
self.sock = None
def putcmd(self, cmd, args=""):
karolyi (Migrated from github.com) commented 2021-02-22 18:25:14 +01:00

See above ...

See above ...
@ -0,0 +272,4 @@
local_hostname=helo_host, timeout=smtp_timeout, debug=debug,
raise_communication_errors=raise_communication_errors,
raise_temporary_errors=raise_temporary_errors,
sender=from_address.ace, recip=email_address.ace)
karolyi (Migrated from github.com) commented 2021-02-22 18:26:11 +01:00

The reason why I had this function is to keep the functions smaller than ~24 lines per function. If you end up with a huge function, it is much harder to understand how it works. Can you revert this with your added changes?

The reason why I had this function is to keep the functions smaller than ~24 lines per function. If you end up with a huge function, it is much harder to understand how it works. Can you revert this with your added changes?
reinhard-mueller (Migrated from github.com) reviewed 2021-02-22 18:33:17 +01:00
@ -0,0 +75,4 @@
self, local_hostname: str, timeout: float, debug: bool,
raise_communication_errors: bool,
raise_temporary_errors: bool,
sender: str, recip: str):
reinhard-mueller (Migrated from github.com) commented 2021-02-22 18:33:17 +01:00

Responses 2xx and 3xx generally indicate success, only 4xx and 5xx indicate failure. Actually at this point in the communication, I don't think if any 3xx response is even possible, but if there is one, it would not indicate an error.

Responses 2xx and 3xx generally indicate success, only 4xx and 5xx indicate failure. Actually at this point in the communication, I don't think if any 3xx response is even possible, but if there is one, it would not indicate an error.
karolyi (Migrated from github.com) reviewed 2021-02-22 18:36:47 +01:00
@ -0,0 +75,4 @@
self, local_hostname: str, timeout: float, debug: bool,
raise_communication_errors: bool,
raise_temporary_errors: bool,
sender: str, recip: str):
karolyi (Migrated from github.com) commented 2021-02-22 18:36:47 +01:00

3yz (Positive Intermediate Reply): The command has been accepted, but the requested action is being held in abeyance, pending receipt of further information.

Alright, I'll resolve this part then.

`3yz (Positive Intermediate Reply): The command has been accepted, but the requested action is being held in abeyance, pending receipt of further information.` Alright, I'll resolve this part then.
reinhard-mueller (Migrated from github.com) reviewed 2021-02-22 18:38:24 +01:00
@ -0,0 +272,4 @@
local_hostname=helo_host, timeout=smtp_timeout, debug=debug,
raise_communication_errors=raise_communication_errors,
raise_temporary_errors=raise_temporary_errors,
sender=from_address.ace, recip=email_address.ace)
reinhard-mueller (Migrated from github.com) commented 2021-02-22 18:38:23 +01:00

Splitting this into two functions would mean that I have to pass a lot of things back and forth, for example I'd have to pass both error collection variables into _check_one_mx so they can be filled there. I first tried it that way, but I had the impression that the code is actually harder to understand with split functions. However, if you think it's better, I can of course change it.

Splitting this into two functions would mean that I have to pass a lot of things back and forth, for example I'd have to pass both error collection variables into _check_one_mx so they can be filled there. I first tried it that way, but I had the impression that the code is actually harder to understand with split functions. However, if you think it's better, I can of course change it.
reinhard-mueller (Migrated from github.com) reviewed 2021-02-22 18:39:42 +01:00
@ -172,4 +58,3 @@
The package contains an auto-updater for downloading and updating the built-in blacklist.txt. It will run on each module load (and installation), but will try to update the content only if the file is older than 5 days, and if the content is not the same that's already downloaded.
The update can be triggered manually::
reinhard-mueller (Migrated from github.com) commented 2021-02-22 18:39:42 +01:00

The idea behind this is to keep the default behavior unchanged for verify_email_or_fail for all the users that already use that function and rely on the fact that for ambiguous cases it doesn't raise an exception.

The idea behind this is to keep the default behavior unchanged for verify_email_or_fail for all the users that already use that function and rely on the fact that for ambiguous cases it doesn't raise an exception.
karolyi (Migrated from github.com) reviewed 2021-02-22 18:40:39 +01:00
@ -0,0 +272,4 @@
local_hostname=helo_host, timeout=smtp_timeout, debug=debug,
raise_communication_errors=raise_communication_errors,
raise_temporary_errors=raise_temporary_errors,
sender=from_address.ace, recip=email_address.ace)
karolyi (Migrated from github.com) commented 2021-02-22 18:40:39 +01:00

Sounds like a candidate for an entire class with methods for me then ... If it gets this complicated, classes provide a better way for communication between the smaller parts.

Sounds like a candidate for an entire class with methods for me then ... If it gets this complicated, classes provide a better way for communication between the smaller parts.
karolyi (Migrated from github.com) reviewed 2021-02-22 18:43:07 +01:00
@ -172,4 +58,3 @@
The package contains an auto-updater for downloading and updating the built-in blacklist.txt. It will run on each module load (and installation), but will try to update the content only if the file is older than 5 days, and if the content is not the same that's already downloaded.
The update can be triggered manually::
karolyi (Migrated from github.com) commented 2021-02-22 18:43:06 +01:00

Ah yes, that problem. I think since we're already heading towards a major release with breaking changes, might as well restructure the code to fit the new purpose and then release it as such.

Ah yes, that problem. I think since we're already heading towards a major release with breaking changes, might as well restructure the code to fit the new purpose and then release it as such.
reinhard-mueller (Migrated from github.com) reviewed 2021-02-22 18:45:48 +01:00
@ -0,0 +272,4 @@
local_hostname=helo_host, timeout=smtp_timeout, debug=debug,
raise_communication_errors=raise_communication_errors,
raise_temporary_errors=raise_temporary_errors,
sender=from_address.ace, recip=email_address.ace)
reinhard-mueller (Migrated from github.com) commented 2021-02-22 18:45:48 +01:00

Interesting idea, I think I'll give it a try. (Not today, though)

Interesting idea, I think I'll give it a try. (Not today, though)
karolyi (Migrated from github.com) reviewed 2021-02-22 18:51:01 +01:00
@ -0,0 +272,4 @@
local_hostname=helo_host, timeout=smtp_timeout, debug=debug,
raise_communication_errors=raise_communication_errors,
raise_temporary_errors=raise_temporary_errors,
sender=from_address.ace, recip=email_address.ace)
karolyi (Migrated from github.com) commented 2021-02-22 18:51:01 +01:00

Take your time :) I'm not Mr. Know It All, these are just my suggestions according to my experiences :) I always try to keep it clean and simple, so that anybody who comes into the project can easily contribute.

Take your time :) I'm not Mr. Know It All, these are just my suggestions according to my experiences :) I always try to keep it clean and simple, so that anybody who comes into the project can easily contribute.
reinhard-mueller commented 2021-02-23 21:37:48 +01:00 (Migrated from github.com)

Please have a look at the updated PR. Following your suggestion to run the SMTP test in a class with methods, I realized that it can nicely be implemented as a subclass of the standard smtplib.SMTP class. I hope I managed to make the code readable and understandable.

There is a unittest that now fails, I would need some help how to update it to the new implementation.

For now I kept the parameters to switch on and off the extra exceptions on communication issues or ambiguous responses. When thinking it over, I felt that it should be the default that in these cases there are no exceptions, but users should be add them if they want to investigate deeper. Also, this way, the current PR should not break existing code.

Please have a look at the updated PR. Following your suggestion to run the SMTP test in a class with methods, I realized that it can nicely be implemented as a subclass of the standard `smtplib.SMTP` class. I hope I managed to make the code readable and understandable. There is a unittest that now fails, I would need some help how to update it to the new implementation. For now I kept the parameters to switch on and off the extra exceptions on communication issues or ambiguous responses. When thinking it over, I felt that it should be the default that in these cases there are no exceptions, but users should be add them if they want to investigate deeper. Also, this way, the current PR should not break existing code.
karolyi commented 2021-02-28 15:07:47 +01:00 (Migrated from github.com)

I've cleaned the code up quite a bit starting from your changes and had to create a separate branch for it :https://github.com/karolyi/py3-validate-email/tree/pull-55

Closing this here, the changes have to be made there. The code seems to be working for me now, but I have to add more detailed tests so that the functionality is tested thoroughly.

I've cleaned the code up quite a bit starting from your changes and had to create a separate branch for it :https://github.com/karolyi/py3-validate-email/tree/pull-55 Closing this here, the changes have to be made there. The code seems to be working for me now, but I have to add more detailed tests so that the functionality is tested thoroughly.

Pull request closed

Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: karolyi/py3-validate-email#55
No description provided.