Django

Code

Ticket #10834 (closed: fixed)

Opened 1 year ago

Last modified 9 months ago

BaseHandler does not handle resolver returning None

Reported by: xerolas Assigned to: ccahoon
Milestone: 1.1 Component: HTTP handling
Version: SVN Keywords:
Cc: chris.cahoon@gmail.com Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

Description

django.core.handler.base.BaseHandler has the following call to the resolver:

            callback, callback_args, callback_kwargs = resolver.resolve(
	                    request.path_info)

However resolve() can return None (this happens to me when I get bogus HTTP requests from bots) and TypeError is not explicitly caught (for trying to unpack None).

The result is that 500 is returned instead of 404 and an email to the admins is sent (by default).

Attachments

ticket10834.diff (1.7 kB) - added by ccahoon on 05/21/09 10:28:58.
regression text and fix, updated to exclude the else statement
10834r11126.diff (0.6 kB) - added by ccahoon on 06/30/09 10:15:07.
Updated to pass regression tests.

Change History

04/26/09 13:21:44 changed by mtredinnick

  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests changed.
  • needs_docs changed.

This is probably addressing the wrong end of the issue. As things stand, there is a problem. But it isn't that we aren't handling None. It's that we are seeing None in the first place.

We should be raising an exception (either Http404 or Resolver404 might be appropriate). It's an exceptional condition and should enter the normal code paths for handling non-existent URLs, not require special handling in the non-exceptional path.

05/01/09 11:51:33 changed by ccahoon

  • owner changed from nobody to ccahoon.

05/19/09 19:18:07 changed by ccahoon

  • has_patch set to 1.

I raise a Resolver404 if self.regex.search(path) returns None. This happens if path is not a regular expression. The unit test gives some examples of test cases that caused resolver to return None before the patch.

05/21/09 10:28:58 changed by ccahoon

  • attachment ticket10834.diff added.

regression text and fix, updated to exclude the else statement

06/26/09 09:29:56 changed by ccahoon

  • cc set to malcolm@pointy-stick.com.
  • milestone set to 1.1.

06/26/09 09:30:48 changed by ccahoon

  • cc changed from malcolm@pointy-stick.com to malcolm@pointy-stick.com, chris.cahoon@gmail.com.

06/29/09 09:02:17 changed by russellm

  • status changed from new to closed.
  • resolution set to fixed.

(In [11120]) Fixed #10834 -- Added bucket condition to ensure that URL resolvers won't ever return None. Thanks to Chris Cahoon for the patch.

(follow-up: ↓ 9 ) 06/30/09 01:01:33 changed by mwelch

This fixed breaks the debug screen. When setting up a new app and setting entries in the urls.py file, you will still get the default blue 'Welcome to django' screen instead of the 404 debug screen.

06/30/09 01:03:58 changed by mwelch

  • status changed from closed to reopened.
  • resolution deleted.

(in reply to: ↑ 7 ) 06/30/09 07:01:33 changed by anonymous

Replying to mwelch:

This fixed breaks the debug screen. When setting up a new app and setting entries in the urls.py file, you will still get the default blue 'Welcome to django' screen instead of the 404 debug screen.

Please provide more information. Using r11126 I cannot recreate any problem getting the 404 debug screen to show up when I have DEBUG set to True and attempt to access a URL that has no mapping in my urlpatterns.

06/30/09 07:17:00 changed by kmtracey

Sorry, anonymous at 7:01:33 was me. (Normally the spam blocker lets me know when my login has expired but not this time.)

06/30/09 08:42:47 changed by ccahoon

That's very interesting. I was able to recreate showing the Welcome to Django screen if the following urlpattern is not covered:

r'^$'

However, the other urls still functioned properly. I looked around and it looks like it is because of this code in django/views/debug.py (this is on trunk r11126):

244 def technical_404_response(request, exception):
245     "Create a technical 404 error response. The exception should be the     
    Http404."
246     try:
247         tried = exception.args[0]['tried']
248     except (IndexError, TypeError):
249         tried = []
250     else:
251         if not tried:
252             # tried exists but is an empty list. The URLconf must've been   
    empty.
253             return empty_urlconf(request)

The content of the changeset (which was different from my patch) creates an empty "tried" list in the dictionary returned in the Resolver404 exception. Thus, debug thinks there is nothing in the urlconf. Attaching a patch.

I am pretty sure I ran into this issue when creating the fix -- only by not even having "tried" in the dictionary does the 404 behave properly. Now I am not sure if this is correct overall behavior, but for a patch this small, it seemed appropriate to just leave it out.

06/30/09 09:01:51 changed by russellm

@ccahoon: There must be something else going on here, because removing tried from the dictionary causes mass test failures in the test_client and test_client_regress unit tests. Your analysis regarding why tried should be omitted looks sound, though - we'll have to dig deeper to find the cause of the failures in the test_client tests.

06/30/09 09:17:35 changed by ccahoon

@russellm: I've had very recent experience with test_client_regress expecting behavior that is consistent with Django but not necessarily correct. I will take a look and see what is going on later tonight.

06/30/09 10:15:07 changed by ccahoon

  • attachment 10834r11126.diff added.

Updated to pass regression tests.

06/30/09 10:18:38 changed by ccahoon

  • cc changed from malcolm@pointy-stick.com, chris.cahoon@gmail.com to chris.cahoon@gmail.com.

That patch passes the test_client_regress tests and seems to behave properly in real use. The machine I made the change on can't handle running the full test suite at the moment, so I do not know about other regression tests.

06/30/09 20:02:49 changed by ccahoon

The patch above passes the regression test suite. @russellm, what do you think of the solution?

07/02/09 02:39:57 changed by russellm

@ccahoon - Not thrilled. The format of the error message with the provided patch is completely misleading. I suspect the patch will need to be more than a single line change - looking at the code, there may be a need to tweak the 404 template to differentiate between the error case described here, and the "new project, no URL's" message.

07/02/09 09:44:28 changed by russellm

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [11156]) [1.0.X] Fixed #10834 -- Corrected [11120] to ensure that there is a difference between catching a bad URL pattern and an new (no URLs) project. Thanks to Matt Welch for the report.

Merge of r11155 from trunk.

07/02/09 10:01:56 changed by russellm

(In [11155]) Fixed #10834 -- Corrected [11120] to ensure that there is a difference between catching a bad URL pattern and an new (no URLs) project. Thanks to Matt Welch for the report.


Add/Change #10834 (BaseHandler does not handle resolver returning None)




Change Properties
Action