Allow filtering 404 reports by user agent

[Ram Rachum]

I get a lot of 404 reports to my inbox that are caused by various spiders. I'd like to block these. The same way we have a mechanism for blocking 404 reports for certain URLs, we should be able to block 404 reports with user agents that are easily discernible as spiders.

[Claude Paroz]

Globally, I'm not thrilled by the proposal. Firstly, redirecting 404 to inboxes is not a best practice, as generally it is better to redirect to a service like Sentry. Secondly, this filtering is depending on projects, there might be people wanting to make stats about 404 generated by spiders. And thirdly, the spider's UA are changing and hard-coding a list in Django would be subject to frequent updates.

However, we should ease middleware customization so as filtering is easier. I would like to counsel people to subclass BrokenLinkEmailsMiddleware and write their own is_ignorable_404 method. Unfortunately, the user agent is not accessible for that method. So I'm accepting this ticket with two goals:

• add the user agent as a parameter to is_ignorable_404
• documenting is_ignorable_404 method of ​https://docs.djangoproject.com/en/dev/ref/middleware/#django.middleware.common.BrokenLinkEmailsMiddleware

[Ram Rachum]

I agree regarding email, it's better to redirect to services like Sentry.

Regarding the first item, do we need anything beyond just adding , user_agent=None to the definition of is_ignorable_404 and sending in the user agent in process_response?

[Claude Paroz]

Replying to coolRR:

Regarding the first item, do we need anything beyond just adding , user_agent=None to the definition of is_ignorable_404 and sending in the user agent in process_response?

Yes, something like that.

[Ram Rachum]

Patch

[Ram Rachum]

Patch attached. Let me know whether it's good. (I don't work often with patches, this one was generated by Git, I don't know whether you can use it in hg.)

[Claude Paroz]

Django is in Git (Python is in hg), so yes, the format is right.
Now we still miss tests and docs.

[Ram Rachum]

Tests

[Ram Rachum]

Here's a test. It's pretty lame; I would have preferred a test that creates a subclass but I'm very clueless about Django's test architecture, so I don't know how to do it in a way that won't bother other Django developers.

[Claude Paroz]

is_ignorable_404 test

[Claude Paroz]

Here's how I would have tested that functionality. Of course, testing is often a matter of personal taste (unit-oriented or functionality-oriented). But it's a little more in line with other tests in the same class.

[Aymeric Augustin]

At this point, it would make more sense to pass the whole request to is_ignorable_404.

Please provide a single patch including both code, tests and docs changes.

[Ram Rachum]

aaugustin: I considered giving the request as an argument so the user could do anything they want, but then I thought that if the user needs so much freedom, maybe they're better off replacing BrokenLinkEmailsMiddleware completely.

[Aymeric Augustin]

I'd just like to avoid further tickets asking to add more keyword arguments.

[Ram Rachum]

aaugustin: I've submitted a patch which changes the method to take the request. It also moves the existing request-checking logic into that method. I didn't do docs yet, I want to first know whether this approach is acceptable.

[Aymeric Augustin]

This is entering bikeshedding territory, the only thing I don't like in this patch is the name is_request_we_should_notify_for; is_ignorable_request would be much more straightforward.

[Ram Rachum]

I'm not attached to the name (I felt it was too long but couldn't come up with a shorter one.)

Note though that is_ignorable_request is a negative name while the name I introduced is positive. (I flipped the logic of the method, I think that all those double negatives are really annoying.)

If you can come up with a short and positive name, I'll be happy to hear it.

[Aymeric Augustin]

I thought about the double negative, but I felt it was still worth emphasizing the relation with IGNORABLE_404_URLS.

[Ram Rachum]

I'm not sure what you're suggesting. If you're suggesting to keep the old name and negativity so it'll be easier for people to understand that this method is related to IGNORABLE_404_URLS, I disagree.

[Aymeric Augustin]

#10214 was closed as a duplicate. The patch for this ticket should add a note in the docs, explaining that subclassing the middleware is the recommended way to alter its behavior.

[Ram Rachum]

New patch with docs addition attached.

[Ram Rachum]

Can anyone review this or merge it in?

[Ram Rachum]

Tumbleweed

[Ram Rachum]

Added pull request, if that helps:

​https://github.com/django/django/pull/1014

If you want, I can rename is_request_we_should_notify_for to is_relevant_request.

[Claude Paroz]

Revisiting this issue after some time, here's an alternative approach: ​https://github.com/django/django/pull/1211

We need a referee now :-)

[Simon Charette]

Left some minor comment on @claudep's PR. Apart from those the patch looks RFC.

[Claude Paroz <claude@…>]

In f940e564e4623d531eb97a2cf1b116851003f9fd:

Fixed #20099 -- Eased subclassing of BrokenLinkEmailsMiddleware

Thanks Ram Rachum for the report and the initial patch, and Simon
Charette for the review.