Getting full URL using django.urls.base.reverse - PR
2,873 views
Skip to first unread message
Mislav Cimperšak
Nov 5, 2016, 10:41:09 AM11/5/16
to Django developers (Contributions to Django itself)
During the sprint in Amsterdam I created a pull request https://github.com/django/django/pull/7484
In it I'm adding an optional argument `request` to `django.urls.base.urls.reverse`method.
The idea is that using `request` object one can get a fully qualified URL.
The idea is taken from Django Rest Framework (https://github.com/tomchristie/django-rest-framework/blob/master/rest_framework/reverse.py#L55-L59).
I've talked to Tim Graham in person and he said that there are some possibilities that django-hosts will be merged into main django code base with it's own implementation of `reverse` method.
To move this issue forward I need an input from core team members regarding my implementation and future plans. If necessary I'll trash my current implementation if it will just bring on more potentials problems in the future.
My arguments for suggested implementation is that it's much more simple and easier for the enduser than django-hosts implementation and it's already Tom Christie approved(TM).
Florian Apolloner
Nov 5, 2016, 11:40:46 AM11/5/16
to Django developers (Contributions to Django itself)
To be honest, I think I do not really see the usecase here. I know that sometimes it is required to generate full urls (especially in emails), but quite often in those you do not have access to the request at all. Also, how would this play together with the url tag? Personally I would set the current domain on the local "urlconf" (we have one per thread) and add a "full" kwarg to reverse (and something similar to the urltag).
Cheers,
Florian
Cheers,
Florian
Mislav Cimperšak
Nov 5, 2016, 12:53:49 PM11/5/16
to Django developers (Contributions to Django itself)
For me use cases were callback urls sent to some 3rd party and of course - emails.
Yeah, I wasn't thinking about url tag.
I'm not 100% sure I understand what you are suggesting.
If we choose to go down the path of using kwarg I could change this in the `reverse` method itself. That way calling `reverse('foo', kwargs={'full': True})` would return a full URL. But that opens a whole other can of issues. For example if someone already has in their code `full` as a parameter for url.
Florian Apolloner
Nov 5, 2016, 12:59:11 PM11/5/16
to Django developers (Contributions to Django itself)
What I was trying to suggest is that the base domain gets stored in one of our threadlocals so that we can generate the full URL without having access to the domain (though I just realize that this wouldn't work in the case of celery etc :/)
Sjoerd Job Postmus
Nov 5, 2016, 1:56:29 PM11/5/16
to django-d...@googlegroups.com
If you go with storing the base domain in the threadlocals, why not go full in and store the request itself in the locals? [1]
As far as using it in the templates... We have a RequestContext right? So in most cases that should not be an issue I presume.
But yes, Celery would be a problem, unless we push the request object to celery as well, but I presume that makes mostly no sense except for this.
[1] I must confess I greatly prefer being explicit with passing the request around, but sometimes you suddenly need the request object 10 layers deep but nowhere in the middle. Or, you want to use the request object in your AuthenticationBackend (for security counter-measures). For instance, django-brutebuster stores the request in a threadlocal to get the IP address.
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/613aacf1-95de-43ac-8cc6-a54c09e22aca%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
Florian Apolloner
Nov 5, 2016, 6:04:06 PM11/5/16
to Django developers (Contributions to Django itself)
On Saturday, November 5, 2016 at 6:56:29 PM UTC+1, Sjoerd Job Postmus wrote:
If you go with storing the base domain in the threadlocals, why not go full in and store the request itself in the locals? [1]
Because that opens a whole new can of worms, if possible we want less threadlocals, not more…
As far as using it in the templates... We have a RequestContext right? So in most cases that should not be an issue I presume.
Still leaves the question of how to nicely pass this to the url tag.
But yes, Celery would be a problem, unless we push the request object to celery as well, but I presume that makes mostly no sense except for this.
Yeah, I didn't think that through :)
Mislav Cimperšak
Nov 11, 2016, 3:15:30 AM11/11/16
to django-d...@googlegroups.com
Thinking about url tag and threadlocals is a step in the wrong direction.
The original PR is just a simple addition (that is still backwards compatible) to the `reverse` method; how people choose to use it (and when) is up to them. Adding something to the `url` tag is almost bound to brake something.
This implementation does not brake anything regarding `url` tag and can stand on it's own.
I've talked to several people on sprints in Amsterdam before submitting a PR and it sounded like a good idea to them and reassured with DRF implementation I went with it :)
--
You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/-rbLcdJkIpk/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/84fc3b7a-4263-46bb-a292-e27215952e63%40googlegroups.com.
Marc Tamlyn
Nov 11, 2016, 4:16:32 AM11/11/16
to django-d...@googlegroups.com
Given we already have `request.build_absolute_uri(path)`, I'm not 100% sure what extra this gives us. It's a bit of syntactic sugar for sure, but it solves a pretty narrow use case. The only times I've actually needed this has been when sending emails and I'm using a `Site` object instead of request.
In a way, this fits in with the work on a new URL resolving system, especially if like the "URL" resolution in channels it can take other attributes of the request. This would make things like Django hosts easier to build, as well as nice things like resolution based on URL scheme. If we go down that route, then it makes sense for `reverse` to take kwargs like `host` or `scheme` as it may be needed to work out which route to use anyway.
I'm definitely -1 on a threadlocal request.
Marc
--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CACcAD09g-yCWw6U5WS-ykKJRG5poWnrBCEOTGFO6YrQKgCKePQ%40mail.gmail.com.
Jannis Leidel
Nov 11, 2016, 5:14:27 AM11/11/16
to Django developers
Hi all,
Full disclosure: I’m one of the authors of django-hosts. But I’ve thought about this problem a lot and maybe that’s useful for this discussion, maybe not :)
# Passing the request to reverse
I'm not convinced that reverse should be directly passed the request to build a full URL since it removes the separation of concern between request parameter handling and URL reversal and therefore be a step in the wrong direction. Having to hand around the request on application level is basically a waste of CPU cycles since that information is already be known to Django on the URL resolver level.
Of course I acknowledge that the use case of generating full URLs on application level exists and that there are multiple ways to achieve this goal. E.g. django-hosts has been designed to mimic the URL pattern system to follow a recognizable and well-established pattern outside of Django’s core, and does more than just single-host reversal.
# django-hosts
It allows a single Django project to respond to requests with different HTTP_HOST headers (e.g. docs.example.com, api.example.com, admin.example.com) and maps each host to different URL configurations if configured so in "host patterns”, conventionally defined in hosts.py next to a project’s urls.py.
Since it was developed as a 3rd party app it obviously had to implement its own reverse method to take the host into account when doing the reversal, since the actual lookup happens with the predefined host patterns, while the URL path reversal happens with Django’s own reverse. It uses a ROOT_HOSTCONF setting to locate the host patterns (similar to ROOT_URL) and defaults to the host that is defined by the DEFAULT_HOST setting if not provided during the reversal. That way it is API-compatible when used via its reverse function or template tag.
I wasn't around when the inclusion of django-hosts or similar ideas was discussed at the DuTH sprint so I’m not sure how much of django-hosts can and should be ported to Django. I let Tim elaborate on that.
# Hosts in Django
What I personally would see as the right direction is to find a common ground for apps like django-hosts and simpler use cases such as generating the full URL when reversing a URL. From a user perspective I like where the DEP (https://github.com/django/deps/pull/27) for simpler URL patterns goes, e.g. it proposes something like this:
urlpatterns = [
path('articles/2003/', views.special_case_2003),
path('articles/<int:year>/', views.year_archive),
path('articles/<int:year>/<int:month>/', views.month_archive),
path('articles/<int:year>/<int:month>/<int:day>/', views.article_detail),
]
So what we could do is allow optionally wrapping some paths with a new, optional host function:
from django.urls import path, host
urlpatterns = [
host('blog.example.com', [
path('articles/2003/', views.special_case_2003, name='blog-special-2003'),
path('articles/<int:year>/', views.year_archive, name='blog-year'),
path('articles/<int:year>/<int:month>/', views.month_archive, name='blog-month'),
path('articles/<int:year>/<int:month>/<int:day>/', views.article_detail, name='blog-day'),
], scheme='//'),
host('api.example.com', [
path('v1/', include(api.urls), namespace='api'),
], scheme='https://')
]
The host function adds the given host and optional scheme to each URL pattern so it's available in the URL resolver reverse function as an optional parameter when constructing the URL. Of course that also means that the URL resolver requires the request host when it resolves the URL pattern callback and not only the request path. But since that’s an internal API I don’t see any backward-compatibility issues with that.
Both the main reverse function and the URL tag would simply get new parameters whether or not to return the full URL (defaulting to False for backward compatibility) and maybe an optional scheme parameter. All those would be possible calls:
reverse('blog-year', kwargs={'year': 2016}) -> /articles/2016/
reverse('blog-year', kwargs={'year': 2016}, host=True) -> //blog.example.com/articles/2016/
reverse('blog-year', kwargs={'year': 2016}, host=True, scheme='https://') -> https://blog.example.com/articles/2016/
reverse('api:user-list') -> /v1/users/
reverse('api:user-list', host=True) -> https://api.example.com/v1/users/
reverse('api:user-list', host=True, scheme='//') -> //api.example.com/v1/users/
{% url 'blog-year' year=2016 %} -> /articles/2016/
{% url 'blog-year' year=2016 with host=True %} -> //blog.example.com/articles/2016/
{% url 'blog-year' year=2016 with host=True scheme='https://' %} -> https://blog.example.com/articles/2016/
{% url 'api:user-list' %} -> /articles/2016/
{% url 'api:user-list' with host=True %} -> https://api.example.com/v1/users/
{% url 'api:user-list' with host=True scheme='//' %} -> //api.example.com/v1/users/
Tim, does that follow your ideas about adding django-hosts like features to Django?
Jannis
> On 11 Nov 2016, at 09:14, Mislav Cimperšak <mislav.c...@gmail.com> wrote:
>
> Thinking about url tag and threadlocals is a step in the wrong direction.
>
> The original PR is just a simple addition (that is still backwards compatible) to the `reverse` method; how people choose to use it (and when) is up to them. Adding something to the `url` tag is almost bound to brake something.
> This implementation does not brake anything regarding `url` tag and can stand on it's own.
>
> I've talked to several people on sprints in Amsterdam before submitting a PR and it sounded like a good idea to them and reassured with DRF implementation I went with it :)
>
> On Sat, Nov 5, 2016 at 11:04 PM, Florian Apolloner <f.apo...@gmail.com> wrote:
> On Saturday, November 5, 2016 at 6:56:29 PM UTC+1, Sjoerd Job Postmus wrote:
> If you go with storing the base domain in the threadlocals, why not go full in and store the request itself in the locals? [1]
>
>
> Because that opens a whole new can of worms, if possible we want less threadlocals, not more…
>
> As far as using it in the templates... We have a RequestContext right? So in most cases that should not be an issue I presume.
>
>
> Still leaves the question of how to nicely pass this to the url tag.
>
> But yes, Celery would be a problem, unless we push the request object to celery as well, but I presume that makes mostly no sense except for this.
>
>
> Yeah, I didn't think that through :)
>
> --
> You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/-rbLcdJkIpk/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
Full disclosure: I’m one of the authors of django-hosts. But I’ve thought about this problem a lot and maybe that’s useful for this discussion, maybe not :)
# Passing the request to reverse
I'm not convinced that reverse should be directly passed the request to build a full URL since it removes the separation of concern between request parameter handling and URL reversal and therefore be a step in the wrong direction. Having to hand around the request on application level is basically a waste of CPU cycles since that information is already be known to Django on the URL resolver level.
Of course I acknowledge that the use case of generating full URLs on application level exists and that there are multiple ways to achieve this goal. E.g. django-hosts has been designed to mimic the URL pattern system to follow a recognizable and well-established pattern outside of Django’s core, and does more than just single-host reversal.
# django-hosts
It allows a single Django project to respond to requests with different HTTP_HOST headers (e.g. docs.example.com, api.example.com, admin.example.com) and maps each host to different URL configurations if configured so in "host patterns”, conventionally defined in hosts.py next to a project’s urls.py.
Since it was developed as a 3rd party app it obviously had to implement its own reverse method to take the host into account when doing the reversal, since the actual lookup happens with the predefined host patterns, while the URL path reversal happens with Django’s own reverse. It uses a ROOT_HOSTCONF setting to locate the host patterns (similar to ROOT_URL) and defaults to the host that is defined by the DEFAULT_HOST setting if not provided during the reversal. That way it is API-compatible when used via its reverse function or template tag.
I wasn't around when the inclusion of django-hosts or similar ideas was discussed at the DuTH sprint so I’m not sure how much of django-hosts can and should be ported to Django. I let Tim elaborate on that.
# Hosts in Django
What I personally would see as the right direction is to find a common ground for apps like django-hosts and simpler use cases such as generating the full URL when reversing a URL. From a user perspective I like where the DEP (https://github.com/django/deps/pull/27) for simpler URL patterns goes, e.g. it proposes something like this:
urlpatterns = [
path('articles/2003/', views.special_case_2003),
path('articles/<int:year>/', views.year_archive),
path('articles/<int:year>/<int:month>/', views.month_archive),
path('articles/<int:year>/<int:month>/<int:day>/', views.article_detail),
]
So what we could do is allow optionally wrapping some paths with a new, optional host function:
from django.urls import path, host
urlpatterns = [
host('blog.example.com', [
path('articles/2003/', views.special_case_2003, name='blog-special-2003'),
path('articles/<int:year>/', views.year_archive, name='blog-year'),
path('articles/<int:year>/<int:month>/', views.month_archive, name='blog-month'),
path('articles/<int:year>/<int:month>/<int:day>/', views.article_detail, name='blog-day'),
], scheme='//'),
host('api.example.com', [
path('v1/', include(api.urls), namespace='api'),
], scheme='https://')
]
The host function adds the given host and optional scheme to each URL pattern so it's available in the URL resolver reverse function as an optional parameter when constructing the URL. Of course that also means that the URL resolver requires the request host when it resolves the URL pattern callback and not only the request path. But since that’s an internal API I don’t see any backward-compatibility issues with that.
Both the main reverse function and the URL tag would simply get new parameters whether or not to return the full URL (defaulting to False for backward compatibility) and maybe an optional scheme parameter. All those would be possible calls:
reverse('blog-year', kwargs={'year': 2016}) -> /articles/2016/
reverse('blog-year', kwargs={'year': 2016}, host=True) -> //blog.example.com/articles/2016/
reverse('blog-year', kwargs={'year': 2016}, host=True, scheme='https://') -> https://blog.example.com/articles/2016/
reverse('api:user-list') -> /v1/users/
reverse('api:user-list', host=True) -> https://api.example.com/v1/users/
reverse('api:user-list', host=True, scheme='//') -> //api.example.com/v1/users/
{% url 'blog-year' year=2016 %} -> /articles/2016/
{% url 'blog-year' year=2016 with host=True %} -> //blog.example.com/articles/2016/
{% url 'blog-year' year=2016 with host=True scheme='https://' %} -> https://blog.example.com/articles/2016/
{% url 'api:user-list' %} -> /articles/2016/
{% url 'api:user-list' with host=True %} -> https://api.example.com/v1/users/
{% url 'api:user-list' with host=True scheme='//' %} -> //api.example.com/v1/users/
Tim, does that follow your ideas about adding django-hosts like features to Django?
Jannis
> On 11 Nov 2016, at 09:14, Mislav Cimperšak <mislav.c...@gmail.com> wrote:
>
> Thinking about url tag and threadlocals is a step in the wrong direction.
>
> The original PR is just a simple addition (that is still backwards compatible) to the `reverse` method; how people choose to use it (and when) is up to them. Adding something to the `url` tag is almost bound to brake something.
> This implementation does not brake anything regarding `url` tag and can stand on it's own.
>
> I've talked to several people on sprints in Amsterdam before submitting a PR and it sounded like a good idea to them and reassured with DRF implementation I went with it :)
>
> On Sat, Nov 5, 2016 at 11:04 PM, Florian Apolloner <f.apo...@gmail.com> wrote:
> On Saturday, November 5, 2016 at 6:56:29 PM UTC+1, Sjoerd Job Postmus wrote:
> If you go with storing the base domain in the threadlocals, why not go full in and store the request itself in the locals? [1]
>
>
> Because that opens a whole new can of worms, if possible we want less threadlocals, not more…
>
> As far as using it in the templates... We have a RequestContext right? So in most cases that should not be an issue I presume.
>
>
> Still leaves the question of how to nicely pass this to the url tag.
>
> But yes, Celery would be a problem, unless we push the request object to celery as well, but I presume that makes mostly no sense except for this.
>
>
> Yeah, I didn't think that through :)
>
> --
> You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/-rbLcdJkIpk/unsubscribe.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/84fc3b7a-4263-46bb-a292-e27215952e63%40googlegroups.com.
>
> For more options, visit https://groups.google.com/d/optout.
>
>
> --
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/84fc3b7a-4263-46bb-a292-e27215952e63%40googlegroups.com.
>
> For more options, visit https://groups.google.com/d/optout.
>
>
> You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> Visit this group at https://groups.google.com/group/django-developers.
> To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CACcAD09g-yCWw6U5WS-ykKJRG5poWnrBCEOTGFO6YrQKgCKePQ%40mail.gmail.com.
Florian Apolloner
Nov 11, 2016, 6:39:24 AM11/11/16
to Django developers (Contributions to Django itself)
On Friday, November 11, 2016 at 10:16:32 AM UTC+1, Marc Tamlyn wrote:
Given we already have `request.build_absolute_uri(path)`, I'm not 100% sure what extra this gives us. It's a bit of syntactic sugar for sure, but it solves a pretty narrow use case. The only times I've actually needed this has been when sending emails and I'm using a `Site` object instead of request.
Jupp, and when sending an email you do not necessarily have access to request. So I'd prefer to pass in a hostname instead of the request object (if at all) -- I am -0/-1 on the PR since it just solves a really narrow usecase and the solution is only one line of syntatic sugar.
Cheers,
Florian
Florian Apolloner
Nov 11, 2016, 6:52:28 AM11/11/16
to Django developers (Contributions to Django itself)
On Friday, November 11, 2016 at 9:15:30 AM UTC+1, Mislav Cimperšak wrote:
The original PR is just a simple addition (that is still backwards compatible) to the `reverse` method; how people choose to use it (and when) is up to them.
This still does not makes it a good addition, especially when the benefit is close to zero. And thinking about the reverse function without thinking about how to use the equivalent in a template is imo a nogo. Especially given the narrow usecase, I'd say that the usability of the feature is even more diminished by the fact that most people reverse in templates as opposed to views. Even if the addition is as small as it is, we are committing ourself to support the request argument then for further versions of django to come…
I've talked to several people on sprints in Amsterdam before submitting a PR and it sounded like a good idea to them and reassured with DRF implementation I went with it :)
While sprints are a good place to gather ideas, that does not mean that there would not be a better approach -- which is why we are having this discussion after all.
Cheers,
Florian
Florian Apolloner
Nov 11, 2016, 7:00:22 AM11/11/16
to Django developers (Contributions to Django itself)
Hi Jannis,
this is very much in line with what I would like to see as outcome of DEP 201. If you manage to find some time, I'd very much like to see your input/guidance on the DEP (given your hands on experience with django-hosts)!
Cheers,
Florian
this is very much in line with what I would like to see as outcome of DEP 201. If you manage to find some time, I'd very much like to see your input/guidance on the DEP (given your hands on experience with django-hosts)!
Cheers,
Florian
Mislav Cimperšak
Nov 11, 2016, 10:28:35 AM11/11/16
to django-d...@googlegroups.com
Since general consensus is that this PR is not the best idea in the world I'm closing it along with corresponding issue on Trac.
To unsubscribe from this group and all its topics, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/dc786c3e-a4e4-4038-9844-ddf37b25a807%40googlegroups.com.
Reply all
Reply to author
Forward
0 new messages