Opened 10 years ago

Closed 9 years ago

Last modified 9 years ago

#23813 closed New feature (fixed)

Add checks for common URLpattern errors

Reported by: Julian Wachholz Owned by: Julian Wachholz
Component: Core (System checks) Version: dev
Severity: Normal Keywords:
Cc: alasdair@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The system checks framework can be expanded to help less experienced users help with common mistakes. One source of those is the urlpatterns we have to configure. It would be nice to have checks in place that verify the validity and sanity of urls.

One particular example that comes to mind (from my own experience):

urlpatterns = [
    url(r'^my_app/$', include('my_app')),
]

This would trigger a warning that the trailing '$' in this regular expression is most likely not intended. :-)


Please do suggest any additional checks that could be added as well.

Change History (12)

comment:1 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Tim Graham, 10 years ago

An idea raised in pull request #3808 is to disallow pattern names that contain a colon to avoid ambiguous namespace lookups.

comment:3 by Alasdair Nicol, 9 years ago

Cc: alasdair@… added

In the last few weeks, I've helped a couple of beginners caught out by the trailing $ when using include. I've had a go at this ticket. I don't think it's ready to check in yet, so I haven't opened a pull request. I'd appreciate a code review and any feedback.

https://github.com/alasdairnicol/django/tree/urlchecks

comment:4 by Josh Smeaton, 9 years ago

After a quick look your changes look good to me. A PR is much easier to review though, even if you feel it's not quite ready to be merged. We can leave feedback on specific lines and also trigger the CI to run its tests. So please, open up a PR and we'll see if we can get this in on time for 1.9.

in reply to:  4 comment:5 by Alasdair Nicol, 9 years ago

Replying to jarshwah:

After a quick look your changes look good to me. A PR is much easier to review though, even if you feel it's not quite ready to be merged. We can leave feedback on specific lines and also trigger the CI to run its tests. So please, open up a PR and we'll see if we can get this in on time for 1.9.

Thanks, I've opened a pull request https://github.com/django/django/pull/5301. In particular, I thought maybe the warning messages and documentation might need improving.

comment:6 by Josh Smeaton, 9 years ago

Has patch: set
Patch needs improvement: set

There are a couple of failing tests. See here: http://djangoci.com/job/pull-requests-trusty/3493/

You'll also need to add some release notes.

I've added a couple of quick comments on the PR.

in reply to:  6 comment:7 by Alasdair Nicol, 9 years ago

Replying to jarshwah:

There are a couple of failing tests. See here: http://djangoci.com/job/pull-requests-trusty/3493/

You'll also need to add some release notes.

I've added a couple of quick comments on the PR.

I've updated the docs as suggested, and added a sentence to the 1.9 release notes.

I tried to look at the failing tests, but I was a bit confused, maybe because I'm unfamiliar with Jenkins. The failing tests seemed to be testing the cache. I wasn't expecting these changes to break those tests.

comment:8 by Josh Smeaton, 9 years ago

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham, 9 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Sorry about those old test failures, that was a transient issue while I was making some updates to Jenkins.

As noted on the pull request, the current warnings don't show which url triggered the warning. I don't think it's acceptable to make the user hunt their entire project to track down the cause of the warning. :-)

in reply to:  9 comment:10 by Alasdair Nicol, 9 years ago

Replying to timgraham:

As noted on the pull request, the current warnings don't show which url triggered the warning. I don't think it's acceptable to make the user hunt their entire project to track down the cause of the warning. :-)

I've updated the pull request. The warning message now includes the regex (and name if it has one) of the URL pattern which triggered the warning.

comment:11 by Josh Smeaton <josh.smeaton@…>, 9 years ago

Resolution: fixed
Status: newclosed

In fe3fc52:

Fixed #23813 -- Added checks for common URL pattern errors

Thanks jwa and lamby for the suggestions, and timgraham and jarshwah
for their reviews.

comment:12 by Tim Graham <timograham@…>, 9 years ago

In f2975c0:

Refs #23813 -- Moved URLconfs into module and tidied docstrings.

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