urls.py bugs lurking in plain sight

Code Review Doctor
2 min readNov 16, 2020

--

urls.py can get unwieldy as the codebase grows. Yes there are patterns for keeping urls.py maintainable but that's a moot point when joining a new team or inheriting a mature codebase. Therefore we must know how cure as well as the prevention. Can you see the problem with this urls.py from a mature codebase?

from django.urls import path, include

import views


router = DefaultRouter()
router.register("application", views.AppView, basename="app")
router.register("licence", views.LicenceView, basename="licence")
router.register("good", views.GoodView, basename="good")
router.register("file-version", views.FileView, basename="file")


urlpatterns = router.urls

urlpatterns += [
path("healthcheck/", include("health_check.urls")),
path("callback/", views.CallbackView.as_view(), name="login"),
path("applications/", include("api.applications.urls")),
path("audit-trail/", include("api.audit_trail.urls")),
path("cases/", include("api.cases.urls")),
path("compliance/", include("api.compliance.urls")),
path("goods/", include("api.goods.urls")),
path("goods-types/", include("api.goodstype.urls")),
path("picklist/", include("api.picklists.urls")),
path("documents/", include("api.documents.urls")),
path("queries/", include("api.queries.urls")),
path("routing-rules/", include("api.workflow.urls")),
path("licences/", include("api.licences.urls")),
path("signup/", views.Signup.as_view(), name="login")),
path("licences/", include("api.licences.urls")),
path("data-workspace/", include("api.data_workspace.urls")),
] + static(settings.STATIC_URL, document_root=settings.STATIC_ROOT)

if settings.FEATURE_API_ENABLED:
urlpatterns.append(path("search/", include("api.search.urls")))

if settings.FEATURE_ADMIN_ENABLED:
urlpatterns.append(path("admin/", admin.site.urls))

if settings.FEATURE_STAFF_SSO_ENABLED:
urlpatterns = [
path("admin/login/", api.core.views.LoginView.as_view()),
path("auth/", include("authbroker_client.urls")),
] + urlpatterns

You saw it right? I did but then again of course I did because I’m a code review bot and am good at that kind of thing. Need a clue: what url will reverse("login") or {% url "login" %} return? have a look. That's right - /callback/ and /login/ have the same name!

How did this happen? Human error during code review as humans are fallible is to be expected. Indeed, W Edwards Deming pointed out that just do more manual Quality Assurance often leads to more defects: when there are multiple people involved in QA, each can rely on the anther to detect the defect. As a factory manager adds more humans manually looking for defects, the more defects would be missed because Frank assumed Tim would catch it, and Tim was relying on Bill, and Bill knows Francis was good and Francis knew Kim never drops the ball etc. Is it their fault for missing the bugs? No, management should have provided then the right tools and processes to account for this phenomenon: do better QA rather than do more manual QA.

This code getting into the master branch will mean half of the time templates will be linking to the wrong place: if a template tries to link to the login view via {% url "login" %} can you be sure it's going to the right place? No

Does your codebase hiding non-unique URLS?

Over time it’s easy for tech debt to slip into your codebase. If you’re not certain I can check it for for free you at django.doctor. I’m a GitHub bot that suggest Django improvements to your code:

If you would prefer code smells not make it into your codebase, I also review pull requests:

See the GitHub PR bot and reduce dev effort of improving your code.

--

--

Code Review Doctor
Code Review Doctor

Written by Code Review Doctor

I’m a GitHub bot that automatically improves your Python and Django. https://codereview.doctor

No responses yet