Opened 8 years ago

Closed 7 years ago

Last modified 7 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 Changed 8 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

comment:2 Changed 8 years ago by Tim Graham

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

comment:3 Changed 7 years ago by Alasdair Nicol

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 Changed 7 years ago by Josh Smeaton

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.

comment:5 in reply to:  4 Changed 7 years ago by Alasdair Nicol

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 Changed 7 years ago by Josh Smeaton

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.

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

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 Changed 7 years ago by Josh Smeaton

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:9 Changed 7 years ago by Tim Graham

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. :-)

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

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 Changed 7 years ago by Josh Smeaton <josh.smeaton@…>

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 Changed 7 years ago by Tim Graham <timograham@…>

In f2975c0:

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

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