Repository: aio-libs/aiosmtpd
Pull request #184 (2020-02-12 20:38:56)

Default status code on exceptions to 451
## What do these changes do?

Change the status code for exceptions to 451 instead of 500.

## Are there changes in behavior for the user?

The user will see delivery retries when their code errors out.

## Related issue number

n/a

## Checklist

- [x] I think the code is well written
- [x] Unit tests for the changes exist

I am not an experienced Python developer and tox fails on the current master within my environment. I believe I updated all the tests and hope that Travis catches any mistakes.

- [x] Documentation reflects the changes
- [ ] Add a new news fragment into the `CHANGES` folder
  * name it `<issue_id>.<type>` (e.g. `588.bugfix`)
  * if you don't have an `issue_id` change it to the pr id after creating the PR
  * ensure type is one of the following:
    * `.feature`: Signifying a new feature.
    * `.bugfix`: Signifying a bug fix.
    * `.doc`: Signifying a documentation improvement.
    * `.removal`: Signifying a deprecation or removal of public API.
    * `.misc`: A ticket has been closed, but it is not of interest to users.
  * Make sure to use full sentences with correct case and punctuation, for example: `Fix issue with non-ascii contents in doctest text files.`

I was unable to find a CHANGES folder.

codecov[bot]: # [Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/184?src=pr&el=h1) Report
> Merging [#184](https://codecov.io/gh/aio-libs/aiosmtpd/pull/184?src=pr&el=desc) into [master](https://codecov.io/gh/aio-libs/aiosmtpd/commit/f414dcdc0312c4cf3f3d39deb3ea7d15e89a5334&el=desc) will **not change** coverage by `%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/aio-libs/aiosmtpd/pull/184/graphs/tree.svg?width=650&height=150&src=pr&token=k9dc7NVG69)](https://codecov.io/gh/aio-libs/aiosmtpd/pull/184?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##            master      #184   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines          796       794    -2     
  Branches       133       132    -1     
=========================================
- Hits           796       794    -2     
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/184?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/aio-libs/aiosmtpd/pull/184?src=pr&el=footer). Last update [97730f3...f5f0cf6](https://codecov.io/gh/aio-libs/aiosmtpd/pull/184?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

TimWolla: Python 3.7 failures appear to be unrelated to my changes.
waynew: You may not have intended to make the change, but if you check the failing line:

```
ERROR: InvocationError for command /home/travis/build/aio-libs/aiosmtpd/.tox/py37-cov/bin/python -m coverage report -m --rcfile=/home/travis/build/aio-libs/aiosmtpd/.coverage.ini --fail-under=100 (exited with code 2)
```
it's https://github.com/aio-libs/aiosmtpd/pull/184/files#diff-f10fb736fb88e7f745b00fc1180ea6f2R187 - somehow you're no longer triggering the condition to get there. Probably because you're changing the return, which another test is looking for. See if you can find that test!
TimWolla: @waynew Ah. At least I understand the failure reason now, the code coverage dropped. I thought it was related to this warning:

> Coverage.py warning: Couldn't use data file '/home/travis/build/aio-libs/aiosmtpd/.coverage.ini': file is encrypted or is not a database

By enabling Travis on my repository I can also confirm that `master` passes the coverage and my branch fails the coverage there. However have no idea where to start looking. I've searched the code for `500` to make sure I did not miss any obvious location and also all the tests pass like they did before. Based on the code block not covered I would assume that some test in `test_starttls.py` is related. It ends there, though.

I also can't get the tests to pass on my local machine within my Docker environment. I believe it's related to IPv6 not being enabled within Docker. I'll see if I can figure that out.

In any case I'd appreciate a helping hand. I'm not an experienced Python developer, while I can find my way around (I was able to build a simple SMTP server fitting my needs using this library :smile:) debugging this is definitely way outside my comfort zone. I'm eager to learn, though!
TimWolla: @waynew I did nothing but rebase my commit onto the current `master` and the change appears to be green now. I assume that #195 fixed it and it was actually a false positive.

Can you PTAL? :smiley: 
pepoluan: > @waynew I did nothing but rebase my commit onto the current master and the change appears to be green now. I assume that #195 fixed it and it was actually a false positive.

That's why I decided to cherry-pick #195 from the hyuuuuge 192, because I know the problems with tox/cov will cause blocking to many 😉

> Can you PTAL? 😃

I will take a look later tonight.

pepoluan: Hi @TimWolla , now that the huge #192 has been merged, can you do another merge/rebase?
TimWolla: @pepoluan Sure, done. I did a "dumb" rebase + fix of the conflict. I did not check whether I need to adjust any new locations, but I suspect I don't.
pepoluan: Looks good to me! 👍 

Let me test this out a bit later tonight, and then we can get this merged.
pepoluan: Ugh, I totally forgot about this... a bit too consumed by the pytest-conversion effort ... sorry about that.

A bit too late in the night for me to test tonight (22:17 UTC+7), I'll try to take a gander tomorrow.
CLAassistant: [![CLA assistant check](https://cla-assistant.io/pull/badge/not_signed)](https://cla-assistant.io/aio-libs/aiosmtpd?pullRequest=184) <br/>Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/aio-libs/aiosmtpd?pullRequest=184) before we can accept your contribution.<br/><sub>You have signed the CLA already but the status is still pending? Let us [recheck](https://cla-assistant.io/check/aio-libs/aiosmtpd?pullRequest=184) it.</sub>
TimWolla: > Like many open source projects, we ask that you sign our [Contributor License Agreement](https://cla-assistant.io/aio-libs/aiosmtpd?pullRequest=184) before we can accept your contribution.

Yeah, no. Please either take this effectively trivial patch as is or … don't.
TimWolla: I just went ahead and rebased this PR to fix the merge conflict and to get rid of that failing CLA status check. PTAL.
pepoluan: > I just went ahead and rebased this PR to fix the merge conflict and to get rid of that failing CLA status check. PTAL.

Green across the board, beautiful! I'll try to find some bandwidth this weekend to take a look.
pepoluan: I'll give 4x24h for others to comment, and if after that there's still no objection, we'll squash'n'merge this :-)
waynew: @pepoluan @TimWolla looks like some merge conflicts but otherwise I'm on board with this
TimWolla: I was tempted to just close this PR, but I did a final rebase, fixing the merge conflicts.
pepoluan: Cool! Let's merge this :-)

Patch:
diff --git a/aiosmtpd/docs/handlers.rst b/aiosmtpd/docs/handlers.rst
index 4a99255..b9776aa 100644
--- a/aiosmtpd/docs/handlers.rst
+++ b/aiosmtpd/docs/handlers.rst
@@ -120,7 +120,7 @@ synchronously (i.e. they are **not** coroutines).
     exception).  The exception object is passed in.  This method *must* return
     a status string, such as ``'542 Internal server error'``.  If the method
     returns ``None`` or raises an exception, an exception will be logged, and a
-    ``500`` code will be returned to the client.
+    ``451`` code will be returned to the client.
     **Note:** If client connection losted function will not be called.
 
 
diff --git a/aiosmtpd/smtp.py b/aiosmtpd/smtp.py
index 54c8606..e2a3b4f 100644
--- a/aiosmtpd/smtp.py
+++ b/aiosmtpd/smtp.py
@@ -315,7 +315,7 @@ class SMTP(asyncio.StreamReaderProtocol):
             return status
         else:
             log.exception('SMTP session exception')
-            status = '500 Error: ({}) {}'.format(
+            status = '451 Error: ({}) {}'.format(
                 error.__class__.__name__, str(error))
             return status
 
@@ -425,10 +425,10 @@ class SMTP(asyncio.StreamReaderProtocol):
                 except Exception as error:
                     try:
                         log.exception('Exception in handle_exception()')
-                        status = '500 Error: ({}) {}'.format(
+                        status = '451 Error: ({}) {}'.format(
                             error.__class__.__name__, str(error))
                     except Exception:
-                        status = '500 Error: Cannot describe error'
+                        status = '451 Error: Cannot describe error'
                     await self.push(status)
 
     async def check_helo_needed(self, helo: str = "HELO") -> bool:


Test patch:
diff --git a/aiosmtpd/tests/test_smtp.py b/aiosmtpd/tests/test_smtp.py
index e29da7c..6577772 100644
--- a/aiosmtpd/tests/test_smtp.py
+++ b/aiosmtpd/tests/test_smtp.py
@@ -167,7 +167,7 @@ class ErroringHandlerCustomResponse:
 
     async def handle_exception(self, error):
         self.error = error
-        return '451 Temporary error: ({}) {}'.format(
+        return '554 Persistent error: ({}) {}'.format(
             error.__class__.__name__, str(error))
 
 
@@ -1516,7 +1516,7 @@ Testing
         self.addCleanup(controller.stop)
         with SMTP(controller.hostname, controller.port) as client:
             code, mesg = client.helo('example.com')
-        self.assertEqual(code, 500)
+        self.assertEqual(code, 451)
         self.assertEqual(mesg, b'Error: (ValueError) test')
         # handler.error did not change because the handler does not have a
         # handle_exception() method.
@@ -1532,8 +1532,8 @@ Testing
         self.addCleanup(controller.stop)
         with SMTP(controller.hostname, controller.port) as client:
             code, mesg = client.helo('example.com')
-        self.assertEqual(code, 451)
-        self.assertEqual(mesg, b'Temporary error: (ValueError) test')
+        self.assertEqual(code, 554)
+        self.assertEqual(mesg, b'Persistent error: (ValueError) test')
         self.assertIsInstance(handler.error, ValueError)
 
     # Suppress logging to the console during the tests.  Depending on
@@ -1546,7 +1546,7 @@ Testing
         self.addCleanup(controller.stop)
         with SMTP(controller.hostname, controller.port) as client:
             code, mesg = client.helo('example.com')
-        self.assertEqual(code, 500)
+        self.assertEqual(code, 451)
         self.assertEqual(mesg, b'Error: (ValueError) ErroringErrorHandler test')
         self.assertIsInstance(handler.error, ValueError)
 
@@ -1587,7 +1587,7 @@ Testing
         self.addCleanup(controller.stop)
         with SMTP(controller.hostname, controller.port) as client:
             code, mesg = client.helo('example.com')
-        self.assertEqual(code, 500)
+        self.assertEqual(code, 451)
         self.assertEqual(mesg, b'Error: Cannot describe error')
         self.assertIsInstance(handler.error, ValueError)