This namespace allows things like {% url blog:article ... %} which is very nice. But reversing doesn't work when an app itself tries to reverse its own urls having a callable at hand:
reverse(views.article, args=[...])
It doesn't work because namespaced URLs are basically excluded from resolver's reverse_dict and are put in their own resolvers under namespace_dict. And reverse doesn't traverse those when it gets a callable.
On one hand I can see logic here: trying to reverse a namespaced URL without specifying a namespace can lead to ambiguous results. But in practice it means that one can break any third-party app that reverses its URLs (using models.permalink, redirect, or reverse) just by including its urlconf under a namespace. Because an app doesn't know its namespace and then has no way of constructing a correct URL.
> This namespace allows things like {% url blog:article ... %} which is > very nice. But reversing doesn't work when an app itself tries to > reverse its own urls having a callable at hand:
> reverse(views.article, args=[...])
> It doesn't work because namespaced URLs are basically excluded from > resolver's reverse_dict and are put in their own resolvers under > namespace_dict. And reverse doesn't traverse those when it gets a callable.
> On one hand I can see logic here: trying to reverse a namespaced URL > without specifying a namespace can lead to ambiguous results. But in > practice it means that one can break any third-party app that reverses > its URLs (using models.permalink, redirect, or reverse) just by > including its urlconf under a namespace. Because an app doesn't know its > namespace and then has no way of constructing a correct URL.
> Am I missing something or is it an evil bug?
A little bit of both.
On the 'missing something' front, reverse() now takes a 'current_app' argument that gives context to the app lookup, which resolves the ambiguity from the point of view of the reverse() function.
This solves the problem as long as applications have been built to provide a namespace. However, this doesn't fully address the 'bug' you describe - an application that hasn't been built to accept namespaces will have problems if it is deployed in a namespace. contrib.comments is one example of an application that you just can't deploy in a namespace. It hasn't been written to allow this mode of deployment.
I'm not sure of the best solution to this problem, though.
This is at least partially a documentation problem. contrib.comments simply isn't namespace-ready. The docs should probably say so, as should the docs for any other app that provides an URLpattern that needs be deployed.
However, it isn't *just* a documentation problem. We should probably also provide some API-level protection to make sure that mistakes like this aren't made by accident.
One possible approach would be to allow an application to mark its URL patterns as "namespace safe"; include() would then raise an error if the URLpattern that is included with a namespace doesn't have this flag set (and/or raise an error if the urls are included without a namespace and the flag *is* set).
The default would need to be 'not safe', so this would be a backwards incompatible change for anyone that is currently using include with a namespace on an app that doesn't have the flag set. This would be slightly backwards incompatible for any existing uses that don't use reverse() internally, or ship with sample templates that use {% url %}.
Russell Keith-Magee wrote: > On the 'missing something' front, reverse() now takes a 'current_app' > argument that gives context to the app lookup, which resolves the > ambiguity from the point of view of the reverse() function.
I saw this one. It doesn't work in this case exactly beacuse of this:
> This solves the problem as long as applications have been built to > provide a namespace.
Which means an app knows its exact namespace.
> However, this doesn't fully address the 'bug' you > describe - an application that hasn't been built to accept namespaces > will have problems if it is deployed in a namespace.
There's important detail here.
One thing is a namespace-ignorant application that just uses reversing the old way. I don't think we can fix it in any way except saying somehow "don't install this app under a namespace" (as you suggested).
Another thing is an application that is being written now and that wants to be namespace-aware. The problem is that while an application knows that it can be included under a namespace it doesn't care under which one exactly.
As far as I can tell it can be solved by keeping views in resolver.reverse_dict as it was before namespaces were introduced as well as in namespace_dict and app_dict. This way they could be found both by specifying a name with or without namespace. However it doesn't solve the problem when you have two instances of the same application. For this we should have a way for an application to know which instance of it is "current". Which means tying reversing to a request (right?). At this point I didn't come up with anything more useful...
> Russell Keith-Magee wrote: >> On the 'missing something' front, reverse() now takes a 'current_app' >> argument that gives context to the app lookup, which resolves the >> ambiguity from the point of view of the reverse() function.
> I saw this one. It doesn't work in this case exactly beacuse of this:
>> This solves the problem as long as applications have been built to >> provide a namespace.
> Which means an app knows its exact namespace.
>> However, this doesn't fully address the 'bug' you >> describe - an application that hasn't been built to accept namespaces >> will have problems if it is deployed in a namespace.
> There's important detail here.
> One thing is a namespace-ignorant application that just uses reversing > the old way. I don't think we can fix it in any way except saying > somehow "don't install this app under a namespace" (as you suggested).
> Another thing is an application that is being written now and that wants > to be namespace-aware. The problem is that while an application knows > that it can be included under a namespace it doesn't care under which > one exactly.
> As far as I can tell it can be solved by keeping views in > resolver.reverse_dict as it was before namespaces were introduced as > well as in namespace_dict and app_dict. This way they could be found > both by specifying a name with or without namespace. However it doesn't > solve the problem when you have two instances of the same application. > For this we should have a way for an application to know which instance > of it is "current". Which means tying reversing to a request (right?). > At this point I didn't come up with anything more useful...
Apologies for taking so long to get back to you on this.
For this last case - building a new app that you want to be namespace aware - I think the Django admin already implements the model that needs to be followed here. I can't think of any obvious way to allow for an application to be 'namespaceless', as well as potentially one of many in a namespace. Admin solves this by *always* requiring a namespace - even if that's just the default application namespace. Django's URL namespace lookup makes the simple case easy, (i.e., admin:named_url works, regardless of how you have deployed admin), while allowing for multiple namespaces.
Personally, I don't think it's too onerous for us to say that namespaces won't work unless you write an app to use them, and if you write an app to use namespaces, then you have to use them when you deploy.
All this leaves is catching (and preventing) the case of deploying applications that aren't namespace aware inside a namespace. For this... patches welcome :-)
Russell Keith-Magee wrote: > Apologies for taking so long to get back to you on this.
No problem :-) Have busy schedule myself :-)
> For this last case - building a new app that you want to be namespace > aware - I think the Django admin already implements the model that > needs to be followed here. I can't think of any obvious way to allow > for an application to be 'namespaceless', as well as potentially one > of many in a namespace. Admin solves this by *always* requiring a > namespace - even if that's just the default application namespace. > Django's URL namespace lookup makes the simple case easy, (i.e., > admin:named_url works, regardless of how you have deployed admin), > while allowing for multiple namespaces.
Ahha! This is the catch I was missing. So in order for an app to place itself generally in the "namespace-aware" category it should define a function that will return a triple of (patterns, app_namespace, instance_namespace) and document it as the only way of including itself in a project's urlconf.
This makes sense now. Thanks!
> All this leaves is catching (and preventing) the case of deploying > applications that aren't namespace aware inside a namespace. For > this... patches welcome :-)
I'll try to look into it when someone reminds of a concept known to other people as "free time"...
Ivan Sagalaev wrote: > Ahha! This is the catch I was missing. So in order for an app to place > itself generally in the "namespace-aware" category it should define a > function that will return a triple of (patterns, app_namespace, > instance_namespace) and document it as the only way of including itself > in a project's urlconf.
> This makes sense now. Thanks!
And now with my newly found understanding I think I've found a small bug (and may be a bigger one). Looks like this:
is supposed to override whatever myapp provides as an app_name with user-specified value 'custom_app_name', right?
But now it doesn't. Here's an excerpt from `include`:
def include(arg, namespace=None, app_name=None): if isinstance(arg, tuple): # callable returning a namespace hint if namespace: raise ImproperlyConfigured('Cannot override ...') urlconf_module, app_name, namespace = arg
A final `app_name` here is extracted from `arg` that an application provides, not from `app_name` parameter. If this is really a bug I'll happily make a patch (it's small).
A bigger (possible, provided I understand things right) bug is that include explicitly forbids redefinition of `namespace` for an app that *does* provide its own namespace. But as we just concluded a namespace-ignorant app just can't be included under a namespace. So specifying a namespace in include is a) not possible for "old" apps and forbidden for "new" ones. How come?
> Ivan Sagalaev wrote: >> Ahha! This is the catch I was missing. So in order for an app to place >> itself generally in the "namespace-aware" category it should define a >> function that will return a triple of (patterns, app_namespace, >> instance_namespace) and document it as the only way of including itself >> in a project's urlconf.
>> This makes sense now. Thanks!
> And now with my newly found understanding I think I've found a small bug > (and may be a bigger one). Looks like this:
> is supposed to override whatever myapp provides as an app_name with > user-specified value 'custom_app_name', right?
> But now it doesn't. Here's an excerpt from `include`:
> def include(arg, namespace=None, app_name=None): > if isinstance(arg, tuple): > # callable returning a namespace hint > if namespace: > raise ImproperlyConfigured('Cannot override ...') > urlconf_module, app_name, namespace = arg
> A final `app_name` here is extracted from `arg` that an application > provides, not from `app_name` parameter. If this is really a bug I'll > happily make a patch (it's small).
> A bigger (possible, provided I understand things right) bug is that > include explicitly forbids redefinition of `namespace` for an app that > *does* provide its own namespace. But as we just concluded a > namespace-ignorant app just can't be included under a namespace. So > specifying a namespace in include is a) not possible for "old" apps and > forbidden for "new" ones. How come?
I think this falls on the fine lines between bug, badly documented intentional limitation, and poorly thought out idea :-)
The documentation hints at this problem, if you read it the way I originally intended - but I'll admit that it isn't clear:
""" URL Namespaces can be specified in two ways.
Firstly, you can provide the application and instance namespace as arguments to include() when you construct your URL patterns. ... Secondly, you can include an object that contains embedded namespace data. """
My original intention was that this was an either-or situation - you can provide namespace arguments to include, *or* you can provide an embeddable object. What isn't clear from this is what happens if you specify both. The 'embedded namespace overrides include()' that you have noticed was intentional.
Of course, the flaw in this plan (which we have already covered) is the extent to which the include() case is useful, given the difficulty in finding the 'current' app. It should be possible to determine the current application during the call to resolve(); the question is how to return that current application data to the view in a useful form -- and for an added degree of difficulty, do so without breaking backwards compatibility on the resolve() API.
However, as noted previously, this requires someone to take the time to sort of the issues. In the interim, I suppose I'd be advising people to avoid using the include() technique to build multi-deployable reusable apps, and use class-based reusable applications instead.
It feels a little cumbersome to me to require users to either manually
specify a namespace or import a different object (than just the usual
urls module) which includes an embedded namespace. The attached patch
allows app authors to specify an "app_name" in their urls module which
will be used as the default app_name and namespace if none are
specified in the users urls module. The user can still specify their
own namespace or tuple with embedded namespace.