#33981: When a DisallowedHost exception is raised, the log message contains an
exception trace
-------------------------------------+-------------------------------------
Reporter: Andrew Selby | Owner: (none)
Type: Uncategorized | Status: closed
Component: Error reporting | Version: 4.1
Severity: Normal | Resolution: wontfix
Keywords: DisallowedHost | Triage Stage:
HTTP_HOST exception trace | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Andrew Selby):
Hello Carlton,
Well...! I'd already spent today looking at the tests and working out how
to tweak things so that the existing test functionality was maintained,
specifically the exception testing you mention as the motivation for PR
13910, while also removing that exception trace dump from the logs. I
think you'll find that my changes protect the intent of the original PR,
so I've taken the liberty of creating a PR for this ticket. Would you
please review my PR & consider reopening the ticket?
Specific commentary on the motivations for my PR follow:
Putting an exception trace into the logs in order to enable a test to
verify the exception type was a problematic case of the tail wagging the
dog - user functionality was being affected in order to enable a test to
be run. That's a mistake.
There is a strange requirement in LoggingAssertionMixin that exc_info must
be non-zero - this has nothing to do with verifying the exception type,
and is likely only there to ensure that exc_info is there so that the
exception type can be extracted from it.
There is also a limitation in AdminEmailHandler in that it **requires**
exc_info in the exception (done by adding an exception trace to the logs)
in order to provide the exception in the notification message. My fix also
gets around this issue, so that now means that if we **don't** want to
provide an exception trace in the notification email, we have the ability
to not do so. It is still possible to include it by adding an exception
trace to the log - but it's no longer necessary, as we now have another
mechanism available to include the exception type in the notification
email.
I've identified a minimal set of changes that protect the logic of
existing tests, eliminates unnecessary existing requirements and
limitations, and removes the unnecessary exception trace from the logs, so
that should keep everybody happy.
Replying to [comment:2 Carlton Gibson]:
> Hi Andrew, thanks. There was a pretty clear consensus on the PR to
accept this change.
>
> > The other logging calls from response_for_exception() include the
exception, it seems to only have been missed here. This allows better
debugging and filtering of errors.
>
> ... as such I'm going to close as `wontfix`.
>
> If the traceback bothers you a custom formatter overriding
[https://docs.python.org/3/library/logging.html#logging.Formatter.formatException
formatException] would likely be the way to go.
--
Ticket URL: <https://code.djangoproject.com/ticket/33981#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/0107018312a01f96-8b0e5a30-99ee-4c37-a5fd-df1f3efb4736-000000%40eu-central-1.amazonses.com.
