URL namespaces

[Marten Kenbeek]

Hi all,

URL namespaces have a few quirks that make them a bit difficult to use and understand. I hope to create a patch that clears up these issues. 

First up: the distinction between an application namespace and an instance namespace. The docs say on the application namespace:

This describes the name of the application that is being deployed. Every instance of a single application will have the same application namespace.

And on the instance namespace: 

This identifies a specific instance of an application. Instance namespaces should be unique across your entire project. However, an instance namespace can be the same as the application namespace. This is used to specify a default instance of an application. 

The current implementation requires that both are specified in the same place: either in the included url configuration by returning a 3-tuple, or in the including url configuration through the arguments to include(). The first case generally does not allow multiple deployments of the same app, unless the included url configuration contains specific logic to return different instance namespaces. The second case does not in any way enforce that multiple deployments in fact have the same application namespace. 

I'd like to get the semantics and the implementation in line, and provide one obvious way to specify namespaces. Including a set of url patterns under a namespace involves two parts: the including urlconf that calls include(), and the included urlconf that is imported by include(). The first is a specific deployment of the imported urlconf; the second is a single app. 

The obvious way as I see it would be to specify the application namespace in the app, and specify the instance namespace as a parameter to include(). This enforces the single application namespace for a single set of patterns, and allows any end-user to specify the instance namespace on a per-instance basis. To take the admin as an example:

admin.site.urls would return a 2-tuple: (patterns, 'admin'), where 'admin' is the application namespace. (An alternative to a 2-tuple could be an object with urlpatterns and app_name attributes.)

To include the admin instance, use include(admin.site.urls, namespace='admin'). This is the instance namespace. If left out, it could default to be the same as the app name.

After a deprecation period, it would be an error to specify an instance namespace if there is no application namespace. This is to ensure that the app can always reverse its own urls using <app_name>:<view_name> if it specifies an application namespace, and using <view_name> if it doesn't. (Setting and app_name would technically still be possible by passing a hardcoded (patterns, app_name) tuple, just not as an advertised feature.)

The second point is about nested namespace handling and current_app. 

At the moment, current_app is looking for an exact namespace match. Unlike the namespaced view path, it is not split into namespace parts using current_app.split(':'). Take the following (fictional) urlpatterns:

blog_patterns = [
    url(r'^comments-one/', include('comments.urls', 'comments-one', 'comments')),
    url(r'^comments-two/', include('comments.urls', 'comments-two', 'comments')),
]

urlpatterns = [
    url(r'^blog-one/', include(blog_patterns, 'blog-one', 'blog')),
    url(r'^blog-two/', include(blog_patterns, 'blog-two', 'blog')),
]

Because of how current_app is handled, it is now impossible to reverse patterns in 'blog-one:comments-one:' using current_app. To select 'blog-one', the current app needs to be the string 'blog-one', but to select 'comments-one', it needs to be 'comments-one'. The only solution is to hardcode at least one of the instance namespaces in the namespaced view path. This also means that setting request.current_app to request.resolver_match.namespace, as recommended in the docs, does not work if you have nested namespaces. 

The ResolverMatch object also has some inconsistent behaviour for app_name. resolver_match.namespace is the full namespace path, i.e. 'blog-one:comments-one' (with resolver_match.namespaces a list of individual namespaces, i.e. ['blog-one', 'comments-one']), but resolver_match.app_name is the outer-most app_name, in this case 'blog-one', with no trace whatsoever of the full app_name path. 

To illustrate how I would see it end up eventually (after the deprecation cycle), I've created a branch at https://github.com/knbk/django/tree/namespaces. I feel these changes are fairly straightforward, but any comments are appreciated. 

Marten

[Tim Graham]

Point 1 sounds like https://code.djangoproject.com/ticket/11642 -- and that ticket says it may be superseded by https://code.djangoproject.com/ticket/21927. Could you review those tickets as well as the others in the "Core (URLs)" component of Trac? It would be good if you could assign yourself any tickets that your project will address.

[Marten Kenbeek]

Sure, I'll go through the list of url tickets on Trac. 

The key difference with #11642 is "default". What I propose is that urls.py specifies the app_name, period. The only reason to change app_name is conflicting app names, and there are easy workarounds that I feel shouldn't be part of the API itself. The namespace would be specified in include(), and default to the app_name. 

Op donderdag 28 mei 2015 15:37:20 UTC+2 schreef Tim Graham:

[Tim Graham]

I think it's an unlikely case anyway, but out of curiosity what are the workarounds? Working up a patch for the first issue sounds good to me. In the meantime, I'll continue refreshing myself of the rest of namespaces so I can reply to your other points.

[Tai Lee]

Thanks for taking the time to write up this proposal. I agree on pretty much all counts, but I do have a few comments:

1)

If an app has templates and view code that are reversing <app_name>:<view_name>, then changing the app name (e.g. by an undocumented feature, passing a (patterns, app_name) tuple), then all those URLs will break. Unless I'm missing something, an app *must always* be installed with the app name that is hard coded in its templates and view code, so there is no workaround for app namespace conflicts?

Does or would it work to allow two apps with the same app namespace, as long as they both are installed with customised instance names, so that the "default app" namespace lookup doesn't return anything, and Django must fallback to the `current_app` to determine which app is being reversed?

2)

I'd rather see the "app name, period" (as opposed to a default app name) defined as a variable in an app's URLconf as per #11642 or as an attribute of the `urlpatterns` object, that Django looks at when a URLconf is included with a dotted path string, instead of requiring users to import and then include a `(patterns, app_name)` tuple. Basically, the app name defined by the app should be used for both of:

include(r^foo/', 'foo.urls')

and

from foo.urls import some_tuple

include(r'^foo/', some_tuple),

3)

Not exactly related to the issue at hand, but if we are reworking URL namespaces it might be worth some consideration. One reason why I always avoid using URL namespaces these days is that it becomes impossible to override an app URL at the project level. Project authors are stuck with the exact URLs defined by the app author, all located below the same root path where the URLs are included and with the same view behaviour.

Without namespaced URLs, the project author can include their own version of a URL at any location (as long as it comes before the app's included URLs) and change the view behaviour. For namespaced URLs, I'd like to be able to do something like:

urlpatterns = patterns('',

    url(r'^my_foo_login/$', 'myapp.views.my_foo_login', name='login', app='foo', namespace='foo'),

    url(r'^foo/', include('foo.urls', app='foo', namespace='foo')),

)

Then when templates or view code in the `foo` app does reverse('foo:login') they will get `/my_foo_login/` instead of `/foo/login/` and I can change the behaviour of the view.

Cheers.

Tai.

[Marten Kenbeek]

I think it's an unlikely case anyway, but out of curiosity what are the workarounds?

In the case of a 2-tuple, just pass (admin.site.urls[0], 'dashboard'). In the case of a module, you'll have to directly import the module and pass a (patterns, app_name) tuple. 

If an app has templates and view code that are reversing <app_name>:<view_name>, then changing the app name (e.g. by an undocumented feature, passing a (patterns, app_name) tuple), then all those URLs will break. Unless I'm missing something, an app *must always* be installed with the app name that is hard coded in its templates and view code, so there is no workaround for app namespace conflicts?

 

Does or would it work to allow two apps with the same app namespace, as long as they both are installed with customised instance names, so that the "default app" namespace lookup doesn't return anything, and Django must fallback to the `current_app` to determine which app is being reversed?

 An app doesn't necessarily have to use its own patterns. With that said, it's up to the user to make sure it works if they are to mess with the app_name. Two conflicting apps can be installed with different namespaces, but there will always be a default (the last one, if there is no namespace with the same name). With a consistent use of `current_app` (even if it's not the *current* application used in the request), you can avoid problems. 

If possible, I'd like to add `current_app` to the request by default, but that has been attempted before, and backwards compatibility is a problem. That would allow an app to more reliably reverse its own urls, provided that their code and templates are called from within their own application. 

I'd rather see the "app name, period" (as opposed to a default app name) defined as a variable in an app's URLconf as per #11642 or as an attribute of the `urlpatterns` object, that Django looks at when a URLconf is included with a dotted path string, instead of requiring users to import and then include a `(patterns, app_name)` tuple. Basically, the app name defined by the app should be used for both of:

 

include(r^foo/', 'foo.urls')

 

and

 

from foo.urls import some_tuple
include(r'^foo/', some_tuple),

I also prefer the first option. That would be the "main" way of setting the app_name. The second option exists for things like `admin.site.urls`, which historically have always returned a tuple, instead of an object with an `urlpatterns` attribute. If objects like `admin.site` would have `urlpatterns` and `app_name` attributes/properties, this could be deprecated in favour of a single way to provide an app_name. Not sure if this is desirable, but it's an option. 

Not exactly related to the issue at hand, but if we are reworking URL namespaces it might be worth some consideration. One reason why I always avoid using URL namespaces these days is that it becomes impossible to override an app URL at the project level. Project authors are stuck with the exact URLs defined by the app author, all located below the same root path where the URLs are included and with the same view behaviour.

 

Without namespaced URLs, the project author can include their own version of a URL at any location (as long as it comes before the app's included URLs) and change the view behaviour. For namespaced URLs, I'd like to be able to do something like:

 

urlpatterns = patterns('',
    url(r'^my_foo_login/$', 'myapp.views.my_foo_login', name='login', app='foo', namespace='foo'),
    url(r'^foo/', include('foo.urls', app='foo', namespace='foo')),
)

 

Then when templates or view code in the `foo` app does reverse('foo:login') they will get `/my_foo_login/` instead of `/foo/login/` and I can change the behaviour of the view.

Now that's something I hadn't thought about. I don't believe there is an easy solution, at least not with the current API, so this will probably have to wait for the new API. I'll make sure to think this through, and find a way to handle this before I start working on the final implementation. 

Marten

Op vrijdag 29 mei 2015 02:52:24 UTC+2 schreef Tai Lee:

[Tai Lee]

An app doesn't necessarily have to use its own patterns. With that said, it's up to the user to make sure it works if they are to mess with the app_name. Two conflicting apps can be installed with different namespaces, but there will always be a default (the last one, if there is no namespace with the same name). With a consistent use of `current_app` (even if it's not the *current* application used in the request), you can avoid problems.

Right. So if I understand correctly, you can avoid problems even when installing two different apps into the same app namespace as long as you *always* supply `current_app` when reversing URLs? In which case, I don't think we need to worry about being able to change the app namespace to avoid conflicts (maybe we should even disallow it), and instead just make it easier to *always* supply `current_app`, which brings me to my next point.

If possible, I'd like to add `current_app` to the request by default, but that has been attempted before, and backwards compatibility is a problem. That would allow an app to more reliably reverse its own urls, provided that their code and templates are called from within their own application.

I'd take this a step further. In this comment -- https://code.djangoproject.com/ticket/21927#comment:9 -- I made a suggestion that Django set a `current_app` hint in thread local storage with a middleware class or even before middleware runs, and then have `reverse()` and `{% url %}` fallback to that default hint ONLY if no current app is explicitly provided via the current mechanisms, so it should be a relatively safe change.

This should make it much more convenient and possible to reverse URLs correctly regardless of conflicting app namespaces, even in code that doesn't have access to the request object. For example, model methods that are executed in templates and do not have access to a `request` or `RequestContext` object. As far as I know, 1 thread = 1 request, so using thread local storage should be safe to provide such a hint for any code that executes in response to a request. For management commands, it would still need to be supplied explicitly. What do you think?

Cheers.

Tai.

[Marten Kenbeek]

The admin proves problematic. Looking at the uses of `AdminSite.name`, it's not easily replaced. There are quite a few places that use the name to reverse urls, but don't have access to the request and the current namespace. I think the best solution for now is to document that you should pass `admin.site.urls` directly to `url()`. `include()` does a bunch of stuff, but none of it is needed in the case of the admin urls. This allows the new `include()` API to be as intended, but it doesn't put an unnecessary burden of providing the "correct" namespace for an admin instance on the end-developer. 

In time I'd like to see a method on the AdminSite that returns an actual resolver object, using the new API. The default URLconf template would simply look something like this:

urlpatterns = [
    admin.site.get_resolver('^admin/'),  # or get_resolver(RegexPattern('^admin/')) to be more explicit
]

This would solve the namespace issue for the admin, but it would still allow for only obvious way to set the instance namespace, and keep the `include()` API clean.

Op zaterdag 30 mei 2015 14:28:22 UTC+2 schreef Tai Lee:

Right. So if I understand correctly, you can avoid problems even when installing two different apps into the same app namespace as long as you *always* supply `current_app` when reversing URLs? In which case, I don't think we need to worry about being able to change the app namespace to avoid conflicts (maybe we should even disallow it), and instead just make it easier to *always* supply `current_app`, which brings me to my next point.

Not providing an option in the API itself is the best we can do to disallow it. But yes, always setting the current_app would avoid a lot of problems. https://code.djangoproject.com/ticket/24127 proposes just that. 

I'd take this a step further. In this comment -- https://code.djangoproject.com/ticket/21927#comment:9 -- I made a suggestion that Django set a `current_app` hint in thread local storage with a middleware class or even before middleware runs, and then have `reverse()` and `{% url %}` fallback to that default hint ONLY if no current app is explicitly provided via the current mechanisms, so it should be a relatively safe change.

This should make it much more convenient and possible to reverse URLs correctly regardless of conflicting app namespaces, even in code that doesn't have access to the request object. For example, model methods that are executed in templates and do not have access to a `request` or `RequestContext` object. As far as I know, 1 thread = 1 request, so using thread local storage should be safe to provide such a hint for any code that executes in response to a request. For management commands, it would still need to be supplied explicitly. What do you think?

Cheers.

Tai.

I don't think it's necessary to introduce another global state. At the moment there are a few places in the admin that use the name but don't have access to the request, this works fine as it is, and if need be these can be reworked so that the current app is passed down to these methods too. The other place where this would help is `get_absolute_url()`. This only adds to the list of why I don't like that method, and I personally don't find it a compelling argument for introducing a global state for the current app.

I think setting the current_app on request by default is a better solution that works in most cases. The only apparent issue is backwards compatibility, I'm still looking for the best way to solve this. 

[Marten Kenbeek]

So 2) has been fixed (#24904 and #24906), and I got a PR for 1) (https://github.com/django/django/pull/4724). 

About #24127: I see a few options here:

1. A context processor or similar that sets `current_app` if it isn't set. By default `current_app` doesn't exist, so there's a good distinction between not set and set to `None`. This would be 100% backwards compatible, but IMO it's not the best solution in the long term. 
2. Provide a new flag to {% url %} to reverse with current_app set to request.resolver_match.namespace. This feels slightly less awkward than the current method recommended by the docs.
3. Deprecating a non-existing current_app to force an explicit choice between the old and new behaviour, then add the new behaviour after the deprecation period. This would be coupled with option 1 for ease-of-use. 
4. A setting? A setting! Yay, another setting...
5. Document the slight backwards-incompatible change? And provide an easy way to retain the old behaviour, of course. 1 through 4 are all far from ideal, and I don't think there's an easy, backwards-compatible fix here. 

Thoughts?

Op dinsdag 2 juni 2015 14:32:42 UTC+2 schreef Marten Kenbeek:

[Tai Lee]

I still think that even though `get_absolute_url` might be disliked these days for various reasons, it is very common in the wild and is a perfect example of a legitimate need to be able to reverse URLs in code that executes in response to a request, but that doesn't have the request passed to it as an argument.

So I'd still like to consider the option of adding a default `current_app` hint to thread local storage (which has a life cycle of one request), and for `reverse()` and therefore also `{% url %}` to use that hint as a default. It could still be overridden by explicitly passing `current_app` to `reverse()` (and also `{% url %}`).

This could be done with a middleware class, and in fact a quick search reveals there are many django snippets and blog posts out where users have re-implemented a `ThreadLocalRequestMiddleware` or similar. But I would prefer to see it done before middleware is applied to make it non-optional, since the value will be read in `reverse()` which can and is often called completely outside the request/response cycle.

For any such code that executes outside the request/response cycle, I think we could provide a context manager that sets the `current_app` default hint on `__enter__` and restores it on `__exit__`.

Django appears to use thread locals already to set the current active language. It's been said that is more of a wart than a design pattern to follow, but clearly it can be used safely in Django, and just like the current active language, having apps that are able to resolve their own URLs properly and consistently is an important feature.

Here's a relevant conversation from a couple years back: https://groups.google.com/forum/#!topic/django-developers/VVAZMWi76nA

Cheers.

Tai.

[Marten Kenbeek]

I actually don't think this behaviour should extend to reverse(). There are good reasons to use request.current_app in the template, but I don't believe this implicit behaviour is warranted outside the templates. Not everyone might agree, but I also believe that get_absolute_url() should return a single canonical url, independent of the current app. Using the current app wouldn't even make sense in most cases, such as the admin or pretty much any app other than the model's app itself. I also plan to change or replace get_absolute_url somewhere in the next few months, so that use case of thread-local storage will soon become obsolete if it's up to me. 

If there is a consensus that thread-local storage is the better solution, I'll follow suit, but I don't plan on implementing it in this patch. It is easily implemented as a third-party app if it suits your use cases. 

I'd like to focus on the best way to solve the backwards compatibility issues. 

Op maandag 8 juni 2015 02:32:58 UTC+2 schreef Tai Lee:

[Aymeric Augustin]

> On 8 juin 2015, at 15:24, Marten Kenbeek <marte...@gmail.com> wrote:
>
> If there is a consensus that thread-local storage is the better solution

I don’t think there’s consensus when only one person brought up the idea :-)

I’m unconvinced at this time and I’ll probably argue against it if it gets more seriously considered.

--
Aymeric.

[Tim Graham]

About #24127, I'd like if you could investigate if making the backwards incompatible change breaks any tests in Django's test suite. That would offer a starting point to think about the ramifications. Wouldn't the fix for broken user code be to set "request.current_app = None" where necessary?

[Marten Kenbeek]

The change causes exactly... 1 test failure, `shortcuts.tests.ShortcutTests.test_render`. It's not even a functional test, it only fails because `self.assertFalse(hasattr(response.context.request, 'current_app'))` fails.The template tests don't even have any namespaced urls, so `request.current_app` is pretty much untested. 

"request.current_app = None" would indeed fix any broken user code. 

Op donderdag 11 juni 2015 21:02:59 UTC+2 schreef Tim Graham:

[Aymeric Augustin]

2015-06-12 0:38 GMT+02:00 Marten Kenbeek <marte...@gmail.com>:

The change causes exactly... 1 test failure, `shortcuts.tests.ShortcutTests.test_render`. It's not even a functional test, it only fails because `self.assertFalse(hasattr(response.context.request, 'current_app'))` fails.The template tests don't even have any namespaced urls, so `request.current_app` is pretty much untested. 

Some of these tests may not be up to modern standards of writing good tests.

At least that's what I remember from refactoring them when I turned the template engine into a library.

Sometimes git blame helps find the original author's intentions and improve the tests.

--

Aymeric.