Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#10834 closed (fixed)

BaseHandler does not handle resolver returning None

Reported by: xerolas Owned by: ccahoon
Component: HTTP handling Version: dev
Severity: Keywords:
Cc: chris.cahoon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

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 (2)

ticket10834.diff (1.7 KB ) - added by ccahoon 15 years ago.
regression text and fix, updated to exclude the else statement
10834r11126.diff (588 bytes ) - added by ccahoon 15 years ago.
Updated to pass regression tests.

Download all attachments as: .zip

Change History (21)

comment:1 by Malcolm Tredinnick, 15 years ago

Triage Stage: UnreviewedAccepted

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.

comment:2 by ccahoon, 15 years ago

Owner: changed from nobody to ccahoon

comment:3 by ccahoon, 15 years ago

Has patch: set

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.

by ccahoon, 15 years ago

Attachment: ticket10834.diff added

regression text and fix, updated to exclude the else statement

comment:4 by ccahoon, 15 years ago

Cc: malcolm@… added
milestone: 1.1

comment:5 by ccahoon, 15 years ago

Cc: chris.cahoon@… added

comment:6 by Russell Keith-Magee, 15 years ago

Resolution: fixed
Status: newclosed

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

comment:7 by mwelch, 15 years ago

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.

comment:8 by mwelch, 15 years ago

Resolution: fixed
Status: closedreopened

in reply to:  7 comment:9 by anonymous, 15 years ago

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.

comment:10 by Karen Tracey, 15 years ago

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

comment:11 by ccahoon, 15 years ago

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.

comment:12 by Russell Keith-Magee, 15 years ago

@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.

comment:13 by ccahoon, 15 years ago

@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.

by ccahoon, 15 years ago

Attachment: 10834r11126.diff added

Updated to pass regression tests.

comment:14 by ccahoon, 15 years ago

Cc: malcolm@… removed

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.

comment:15 by ccahoon, 15 years ago

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

comment:16 by Russell Keith-Magee, 15 years ago

@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.

comment:17 by Russell Keith-Magee, 15 years ago

Resolution: fixed
Status: reopenedclosed

(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.

comment:18 by Russell Keith-Magee, 15 years ago

(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.

comment:19 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

Note: See TracTickets for help on using tickets.
Back to Top