Implement urls module with reverse and reverse_lazy#234
Conversation
…lt urls module with reverse and reverse_lazy wrappers. notes: monkey patches new functions into django internals allowing url template tags to resolve. auto-resolved names are slugified versions of their functions
📝 WalkthroughWalkthroughThis PR adds URL reversing support for Bolt routes, enabling Django's ChangesDjango-Bolt URL Reversing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Could definitely use some tests, but want to be sure about the auto-resolve naming conventions first |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/django_bolt/decorators.py (1)
50-57: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd the repo-standard future import to this file.
This change adds more modern annotations, but
python/django_bolt/decorators.pystill lacksfrom __future__ import annotations. Please add it at the module top while touching this file.Suggested change
+from __future__ import annotations + from collections.abc import Callable from typing import AnyAs per coding guidelines,
**/*.py: Usefrom __future__ import annotationsat the top of Python files for cleaner type hints across the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/django_bolt/decorators.py` around lines 50 - 57, Add the repo-standard future import by inserting "from __future__ import annotations" at the very top of python/django_bolt/decorators.py (above any other imports or code) so the new forward-style type annotations used in the Decorator class and its __init__ signature (fn: Callable, methods: list[str], detail: bool, path: str | None, name: str | None, auth: list[Any] | None) are evaluated as postponed annotations across the module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/django_bolt/_bolt_urlconf.py`:
- Around line 78-90: The code currently appends a bare root-level alias for
every (namespace, name) in chosen which creates duplicate bare names when
different namespaces share the same name; update the loop that builds bare and
by_namespace so you first compute global counts of each name from chosen, then
only call _django_path(route, _reverse_only_view, name=name) and append it to
bare when that name's global count == 1 (otherwise skip creating the bare alias
for that (namespace,name)); still register the namespaced entry into
by_namespace for any namespace using the existing
by_namespace.setdefault(...).append(...) logic so _django_path and
_reverse_only_view usages remain unchanged.
In `@python/django_bolt/api.py`:
- Around line 129-133: The namespace logic in _route_namespace is too coarse;
replace the simple top-level segment rule with detection of the Django app
boundary by checking django.apps.apps.get_app_configs() to find an AppConfig
whose module path is a prefix of getattr(fn, "__module__", "") and use that
app's label (or app.module split) as the namespace; if no AppConfig matches,
fall back to the original _slugify_route_name(top) behavior. Update
_route_namespace to iterate django.apps.apps.get_app_configs(), compare
app.module to the handler module prefix, pick the longest matching app.module to
handle nested packages (e.g., project.apps.missions vs project.apps.billing),
and then slugify that chosen app identifier.
- Around line 1281-1292: The decorator call currently passes only
base_name_explicit to _name_explicit which ignores when an action itself has an
explicit name; update the logic around action_suffix/action_route_name to
compute an action_name_explicit flag (e.g., action_name_explicit = attr.name is
not None) and pass _name_explicit=(base_name_explicit or action_name_explicit)
into self._route_decorator so explicit action names are respected; adjust
references to attr.name, action_route_name, _route_decorator and
base_name_explicit accordingly.
In `@python/django_bolt/apps.py`:
- Around line 9-13: Move the inline import out of ready(): add "from __future__
import annotations" at the top of the module and hoist "from .urls import
patch_django_reverse" to module scope, then simplify ready() to just call
patch_django_reverse(); ensure no other inline imports remain and keep import
ordering consistent with existing style.
---
Outside diff comments:
In `@python/django_bolt/decorators.py`:
- Around line 50-57: Add the repo-standard future import by inserting "from
__future__ import annotations" at the very top of
python/django_bolt/decorators.py (above any other imports or code) so the new
forward-style type annotations used in the Decorator class and its __init__
signature (fn: Callable, methods: list[str], detail: bool, path: str | None,
name: str | None, auth: list[Any] | None) are evaluated as postponed annotations
across the module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c64532f-f584-418b-9831-9924c0f3a83a
📒 Files selected for processing (6)
python/django_bolt/_bolt_urlconf.pypython/django_bolt/api.pypython/django_bolt/apps.pypython/django_bolt/decorators.pypython/django_bolt/typing.pypython/django_bolt/urls.py
| for (namespace, name), (route, _explicit) in chosen.items(): | ||
| # Bare (root) name so {% url 'name' %} works without a prefix, like a | ||
| # non-namespaced Django include. Every route also gets a namespaced alias. | ||
| bare.append(_django_path(route, _reverse_only_view, name=name)) | ||
| if namespace: | ||
| by_namespace.setdefault(namespace, []).append( | ||
| _django_path(route, _reverse_only_view, name=name) | ||
| ) | ||
|
|
||
| # Bare names first; namespaced aliases follow for disambiguation. | ||
| urlpatterns: list = list(bare) | ||
| for namespace, patterns in by_namespace.items(): | ||
| urlpatterns.append(_django_path("", include((patterns, namespace)))) |
There was a problem hiding this comment.
Don't emit bare aliases for namespaced collisions.
This appends a root-level name= entry for every route before adding the namespaced include. If two namespaces both define the same route name, the synthetic urlconf now contains two bare patterns with that name as well, so the unqualified reverse stays ambiguous and can resolve the wrong route. Only create the bare alias when a name is globally unique, or skip the bare alias for namespaced routes entirely.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/django_bolt/_bolt_urlconf.py` around lines 78 - 90, The code currently
appends a bare root-level alias for every (namespace, name) in chosen which
creates duplicate bare names when different namespaces share the same name;
update the loop that builds bare and by_namespace so you first compute global
counts of each name from chosen, then only call _django_path(route,
_reverse_only_view, name=name) and append it to bare when that name's global
count == 1 (otherwise skip creating the bare alias for that (namespace,name));
still register the namespaced entry into by_namespace for any namespace using
the existing by_namespace.setdefault(...).append(...) logic so _django_path and
_reverse_only_view usages remain unchanged.
There was a problem hiding this comment.
I believe this mimics Django behavior (masking).. Namespacing is also automatic/implicit by module with the autoresolving logic so it should be available when names may mask eachother. However, yes there should be a more concrete spec for how these are handled
There was a problem hiding this comment.
@dbrun3, that's a fair point — Django itself lets bare names coexist alongside namespaced ones (e.g. {% url 'list' %} and {% url 'missions:list' %} both resolve), so the current approach is a reasonable parallel.
The residual concern is purely the first-match wins behaviour for unqualified lookups: if two namespaces register the same bare name, the route that lands first in chosen.items() (dict insertion order, i.e. discovery order) silently wins. That's also Django-like — but it's easy to miss for users who don't expect it and don't use the qualified namespace:name form.
A concrete spec clarifying that behaviour would remove the ambiguity. Would you like me to open a follow-up issue to track that?
✏️ Learnings added
Learnt from: dbrun3
Repo: dj-bolt/django-bolt PR: 234
File: python/django_bolt/_bolt_urlconf.py:78-90
Timestamp: 2026-06-02T22:26:00.116Z
Learning: In `python/django_bolt/_bolt_urlconf.py`, bare (root-level) URL aliases are intentionally emitted for every named route, including those that also have a namespace, to mimic Django's behavior where `{% url 'name' %}` and `{% url 'namespace:name' %}` can coexist. When two namespaces share the same bare name, first-match (dict/discovery order) wins for unqualified reversal — this is accepted by design, but a concrete spec for collision resolution is still pending.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: dbrun3
Repo: dj-bolt/django-bolt PR: 234
File: python/django_bolt/api.py:129-133
Timestamp: 2026-06-02T22:19:34.181Z
Learning: In `python/django_bolt/api.py`, the `_route_namespace()` helper is intended to return the **penultimate** module segment — the segment immediately before the last one (e.g. `api`). For `project.apps.missions.api`, the namespace should be `missions`; for `project.apps.billing.api` it should be `billing`. The current implementation (`module.split(".", 1)[0]`) is a known bug that returns the top-level package instead.
Learnt from: CR
Repo: dj-bolt/django-bolt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-26T17:31:31.163Z
Learning: Applies to python/django_bolt/management/commands/runbolt.py : Use Django management command autodiscovery pattern to find `api.py` files in Django project root and installed apps
Learnt from: CR
Repo: dj-bolt/django-bolt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-26T17:31:31.163Z
Learning: Applies to {python/django_bolt/api.py,python/django_bolt/router.py} : Guarantee route metadata keys at registration time: required keys are `mode`, `is_async`, `default_status_code`, `response_type` with direct access in dispatch path
Learnt from: CR
Repo: dj-bolt/django-bolt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-26T17:31:31.163Z
Learning: Applies to {python/django_bolt/api.py,python/django_bolt/router.py} : All route metadata keys must be guaranteed at registration time so the dispatch path uses direct `meta["key"]` access instead of `meta.get("key", default)`. Required keys: `mode`, `is_async`, `default_status_code`, `response_type`
| def _route_namespace(fn: Callable) -> str: | ||
| """Derive the URL-reverse namespace for a handler from its module.""" | ||
| module = getattr(fn, "__module__", "") or "" | ||
| top = module.split(".", 1)[0] | ||
| return _slugify_route_name(top) |
There was a problem hiding this comment.
Namespace derivation is too coarse for nested app layouts.
module.split(".", 1)[0] only keeps the top-level package. In layouts like project.apps.missions.api and project.apps.billing.api, both handlers end up in the project namespace, so the ambiguity this feature is meant to resolve is still there. The namespace needs to be derived from the discovered Bolt app/module boundary, not the first module segment.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/django_bolt/api.py` around lines 129 - 133, The namespace logic in
_route_namespace is too coarse; replace the simple top-level segment rule with
detection of the Django app boundary by checking
django.apps.apps.get_app_configs() to find an AppConfig whose module path is a
prefix of getattr(fn, "__module__", "") and use that app's label (or app.module
split) as the namespace; if no AppConfig matches, fall back to the original
_slugify_route_name(top) behavior. Update _route_namespace to iterate
django.apps.apps.get_app_configs(), compare app.module to the handler module
prefix, pick the longest matching app.module to handle nested packages (e.g.,
project.apps.missions vs project.apps.billing), and then slugify that chosen app
identifier.
There was a problem hiding this comment.
Should always be second to last (module name prior to api). Noted
There was a problem hiding this comment.
@dbrun3, got it — the namespace should always be the penultimate module segment (the one immediately before api), so project.apps.missions.api → missions and project.apps.billing.api → billing. That's a much cleaner rule than walking AppConfigs.
The fix would be something like:
def _route_namespace(fn: Callable) -> str:
"""Derive the URL-reverse namespace for a handler from its module."""
module = getattr(fn, "__module__", "") or ""
parts = module.rsplit(".", 1)
# Use the segment immediately before the last one (e.g. "missions" from "project.apps.missions.api")
segment = parts[0].rsplit(".", 1)[-1] if len(parts) > 1 else parts[0]
return _slugify_route_name(segment)I'll record this for future reviews.
✏️ Learnings added
Learnt from: dbrun3
Repo: dj-bolt/django-bolt PR: 234
File: python/django_bolt/api.py:129-133
Timestamp: 2026-06-02T22:19:34.181Z
Learning: In `python/django_bolt/api.py`, the `_route_namespace()` helper is intended to return the **penultimate** module segment — the segment immediately before the last one (e.g. `api`). For `project.apps.missions.api`, the namespace should be `missions`; for `project.apps.billing.api` it should be `billing`. The current implementation (`module.split(".", 1)[0]`) is a known bug that returns the top-level package instead.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: dj-bolt/django-bolt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-26T17:31:31.163Z
Learning: Applies to {python/django_bolt/api.py,python/django_bolt/router.py} : All route metadata keys must be guaranteed at registration time so the dispatch path uses direct `meta["key"]` access instead of `meta.get("key", default)`. Required keys: `mode`, `is_async`, `default_status_code`, `response_type`
Learnt from: CR
Repo: dj-bolt/django-bolt PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-05-26T17:31:31.163Z
Learning: Applies to {python/django_bolt/api.py,python/django_bolt/router.py} : Guarantee route metadata keys at registration time: required keys are `mode`, `is_async`, `default_status_code`, `response_type` with direct access in dispatch path
| # Derive the action's reverse name: base + suffix. The suffix is | ||
| # the @action name override (explicit) or the function name | ||
| # (derived). attr.path is the URL segment, not a name source. | ||
| action_suffix = attr.name or unbound_fn.__name__ | ||
| action_route_name = f"{base_name}-{action_suffix}" if base_name else action_suffix | ||
|
|
||
| # Register using existing route decorator | ||
| decorator = self._route_decorator( | ||
| http_method, | ||
| action_path, | ||
| name=action_route_name, | ||
| _name_explicit=base_name_explicit, |
There was a problem hiding this comment.
Keep @action(name=...) explicit even when the viewset base name is derived.
_name_explicit only reflects whether the viewset itself was named. If the viewset uses its derived base but a custom action sets name=, the final route is still marked as derived. That breaks the explicit-name precedence/collision handling in _bolt_urlconf — e.g. an explicit custom action named "list" on an unnamed viewset is treated the same as the generated -list route.
Suggested change
decorator = self._route_decorator(
http_method,
action_path,
name=action_route_name,
- _name_explicit=base_name_explicit,
+ _name_explicit=base_name_explicit or attr.name is not None,
response_model=attr.response_model,
status_code=attr.status_code,
validate_response=final_validate_response,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Derive the action's reverse name: base + suffix. The suffix is | |
| # the @action name override (explicit) or the function name | |
| # (derived). attr.path is the URL segment, not a name source. | |
| action_suffix = attr.name or unbound_fn.__name__ | |
| action_route_name = f"{base_name}-{action_suffix}" if base_name else action_suffix | |
| # Register using existing route decorator | |
| decorator = self._route_decorator( | |
| http_method, | |
| action_path, | |
| name=action_route_name, | |
| _name_explicit=base_name_explicit, | |
| # Derive the action's reverse name: base + suffix. The suffix is | |
| # the `@action` name override (explicit) or the function name | |
| # (derived). attr.path is the URL segment, not a name source. | |
| action_suffix = attr.name or unbound_fn.__name__ | |
| action_route_name = f"{base_name}-{action_suffix}" if base_name else action_suffix | |
| # Register using existing route decorator | |
| decorator = self._route_decorator( | |
| http_method, | |
| action_path, | |
| name=action_route_name, | |
| _name_explicit=base_name_explicit or attr.name is not None, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/django_bolt/api.py` around lines 1281 - 1292, The decorator call
currently passes only base_name_explicit to _name_explicit which ignores when an
action itself has an explicit name; update the logic around
action_suffix/action_route_name to compute an action_name_explicit flag (e.g.,
action_name_explicit = attr.name is not None) and pass
_name_explicit=(base_name_explicit or action_name_explicit) into
self._route_decorator so explicit action names are respected; adjust references
to attr.name, action_route_name, _route_decorator and base_name_explicit
accordingly.
There was a problem hiding this comment.
Should also decide whether or not a custom action should still use a derived base or not... I'll let @FarhanAliRaza make the judgement call here.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def ready(self): | ||
| # Install django_bolt.urls.reverse patch. | ||
| from .urls import patch_django_reverse | ||
|
|
||
| patch_django_reverse() |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Move the patch import to module scope.
The new ready() uses an inline import, which violates the repo import rule and looks unnecessary here. Please hoist patch_django_reverse to the module top and add the standard future import while touching this file.
Suggested change
+from __future__ import annotations
+
from django.apps import AppConfig
+from .urls import patch_django_reverse
@@
- def ready(self):
+ def ready(self) -> None:
# Install django_bolt.urls.reverse patch.
- from .urls import patch_django_reverse
-
patch_django_reverse()As per coding guidelines, **/*.py: Use from __future__ import annotations at the top of Python files for cleaner type hints across the codebase; place all imports at the module top level, never use inline imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/django_bolt/apps.py` around lines 9 - 13, Move the inline import out
of ready(): add "from __future__ import annotations" at the top of the module
and hoist "from .urls import patch_django_reverse" to module scope, then
simplify ready() to just call patch_django_reverse(); ensure no other inline
imports remain and keep import ordering consistent with existing style.
1 participant
Fixes #114
Implements a bolt urls module with reverse and reverse_lazy, wrapping the original functionality while handling bolt routes with a synthetic URLconf. Monkey patches new implementations over Django's, which allows {% url %} tags to resolve to their correct bolt routes. Adds the ability to add names to routes, but will also auto-resolve by function name, including namespace support (module containing api.py) for cases with ambiguity. Additionally handles viewsets with action suffixes like '-list' and '-retrieve'.
Note: Auto-resolved names are slugs such that CamelCase and snake_case class names and functions get 'resolved-to-this-format'. Totally cool with changing if controversial.
How was this tested?
just lint-libandjust test-pypassjust rebuildsucceedspython/example/)Performance consideration
Should cost the same as 'native' Django reversal, which is cached.
Hot Request Path Impact: YES, but Efficiently Designed
Does This PR Touch the Hot Request Path?
YES — The
reverse()function is called during every request that uses the{% url %}template tag or explicitly callsreverse()in views. This PR monkey-patches Django's globalreverse()function, so the new code executes in these scenarios.Is the Work Required and Justified?
YES — This PR solves issue
#114by making Bolt routes discoverable through Django's standard URL reversing mechanism. Without this, the template tag would fail for any Bolt routes.Performance Verdict: EFFICIENT — Minimal Hot-Path Overhead
The implementation demonstrates excellent performance design with clear separation between startup and request-time costs:
REQUEST-TIME OVERHEAD (per
reverse()call)Minimal: Just 3-4 lightweight operations:
urlconf is None(variable comparison)_bolt_routes_present()(returns cached boolean after first call)_django_reverse()against pre-built Bolt urlconf (Django's resolver caches this)django_mount_prefixstring on Django fallbackModule-level caching:
_django_mount_prefixand_has_bolt_routesare computed once at first use and cached as module variables, not recomputed per requestDjango's resolver cache: The synthetic
bolt_urlconfis referenced by name so Django'sget_resolver()caches it via LRU, eliminating pattern-matching overhead after first reversalSTARTUP-TIME WORK (One-time, not on request path)
At app initialization (
apps.pyready()method):_bolt_urlconf.pyimports and runs:autodiscover(), API merging, pattern building, mount-prefix detectionPERFORMANCE CHARACTERISTICS
Best Case (Bolt route matches):
Average Case (Django route fallback):
reverse()+ string concatenationWorst Case (no match):
NoReverseMatchraisedSMART SHORT-CIRCUITS
urlconfexplicitly provided → skip Bolt, delegate directly to Djangourlpatternscauses fast fallback to Django every callConclusion
✅ The PR correctly balances functionality and performance. Expensive work (autodiscovery, pattern building) happens at startup, while request-time operations are lightweight and fully cached. This is the right architectural approach for solving URL resolution without impacting application throughput.