* cc: dcwatson@… (added)
* easy: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:25>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
* cc: hvdklauw (removed)
* ui_ux: => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:26>
* cc: hvdklauw@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:27>
* cc: tgecho (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:28>
* cc: fernando.sanchez@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:29>
* cc: florian+django@… (added)
* version: 1.3-beta =>
Comment:
I would keep ''get_current_site'' as is (eg don't perform any magic
regarding SITE_ID) and just add a setting to allow users to add their own
backend to resolve the current site. Looking up a Site object based on the
request is a twoliner then.
Is there anything else which needs to be done to get this ticket rolling
again?
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:30>
* owner: nobody => apollo13
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:31>
Comment (by apollo13):
Updated and minimalistic patch on
https://github.com/apollo13/django/compare/ticket15089
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:32>
* cc: krzysiumed@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:33>
* cc: tom@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:34>
* cc: net147 (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:35>
Comment (by anonymous):
I like the latest patch as it's lightweight and somehow mitigates the
issue, it'll reduce to the minimum the number of hacks required to achieve
multitenancy.
It's worth noting that there are a few places in django that use
SiteManager.get_current() instead of get_current_site(). A quick scan gave
me sitemaps, comment feeds and comment moderation. These are usually from
functions that don't have access to the request. I'd say this patch should
include some refactoring to completely remove get_current() usage. It
should probably be documented that any use of get_current() in third-party
apps will break multitenancy, instead of recommending get_current() as
it's currently the case, it should be strongly advised against.
Proper multitenancy is still not achieved, even with this patch, because
it doesn't address things like MEDIA_ROOT, MEDIA_URL,
EMAIL_SUBJECT_PREFIX, DEFAULT_FROM_EMAIL that are needed for most
multitenant usages.
For completeness I'll also mention TIME_ZONE, LANGUAGE_CODE and
ROOT_URLCONF that are important elements of multitenancy as well, even
though it's already possible to affect their value on a per request basis
without resorting to terrible hacks.
To solve this issue some people resort to wrapping settings and storing
their values in threaded locals, at one point or another it breaks as some
code don't like wrapped values. Since django is fundamentally broken when
it comes to multitenancy I suggest to people that really need it and that
can afford single-thread installations to simply modify settings on the
fly. It's frowned upon but other solutions are just as bad.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:36>
* cc: charette.s@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:37>
Comment (by Claude Paroz <claude@…>):
In [changeset:"6c2faaceb0482267cec19da0ff432984028f9d0c"]:
{{{
#!CommitTicketReference repository=""
revision="6c2faaceb0482267cec19da0ff432984028f9d0c"
Made more extensive use of get_current_site
Refs #15089
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:38>
Comment (by claudep):
I've just uploaded an intermediary patch which make get_current_site
always use the request to find out the current site if SITE_ID is not in
the settings. Of course, having a way to provide a custom method to
retrieve the current site is still useful, but maybe in a second stage?
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:39>
Comment (by Claude Paroz <claude@…>):
In [changeset:"1ce4aedcefb68086918adc4137d75a6f2c0bd1f2"]:
{{{
#!CommitTicketReference repository=""
revision="1ce4aedcefb68086918adc4137d75a6f2c0bd1f2"
Prevented flatpage view from directly accessing settings.SITE_ID
Refs #15089
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:40>
Comment (by nebstrebor):
Any chance this ends up in 1.5, or is it even on anyone's radar? Just
wondering since we're about to convert our site to multi-tenant
architecture and have been holding off a while, hoping django would become
more friendly to it.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:41>
Comment (by claudep):
Review carefully the latest patch, test it, tell us if it effectively
solve issues for you. This will help us to go forward.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:42>
Comment (by nebstrebor):
Will do. We'll be starting to implement our change-over to multitenant
probably in about 1 month's time, and I will see what the patch does and
does not provide that we need and let you know.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:43>
* stage: Design decision needed => Accepted
Comment:
Dynamic multi-tenancy is a commonly requested feature, no one is against
the concept.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:44>
Comment (by loic84):
If we are serious about having multi-tenancy in core, we need the current
site to be made a thread local and a first class citizen in ` BaseHandler
.get_response()`, just like we do for URLCONF. Indeed,
`get_current_site()` and `get_urlconf()` are the two sides of the same
coin.
There are plenty of areas of Django that need to be aware of the current
site and that aren't reached (most of the time for good reasons) by the
request object. The remaining uses of `Site.objects.get_current()` in the
codebase (instead of `get_current_site()`) hint at the problem.
Particularly troublesome if the SITE_ID is only available in the request:
- Declarative stuff like sitemaps, default values of model fields, etc.
- Random methods that don't get the request as parameter even though the
calling method had access to it.
- Methods called from templates such as `get_absolute_url()`. Being able
to pass the request as a parameter to such methods is the main reason I've
been assessing Jinja2.
`get_current_site()` would need to be swappable just like `KEY_FUNCTION`
for the cache framework, so people can define their own switching logic.
Also it would be nice if the stock `get_current_site()` had logic for
getting the `SITE_ID` from environ, so that management commands can target
a specific site. Before someone mentions that you can make a temporary
`settings.py` that overrides the`SITE_ID`, I'll say that while it works,
it's not exactly practical when you have several hundred sites on a multi-
tenant setup.
Finally, it's obvious that the perfect solution would also have to deal
with settings, but the site switching problem can be solved without
addressing it, so let's not condition this feature to a refactoring of
settings that might never happen. Developers would still be on their own
when it comes to switching settings per site, but this can be handled case
by case by people who need it.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:45>
Comment (by akaariai):
I agree that thread-local sites would make sense in many use cases. I
think the whole Site model should be swappable. Then, there could be
Site.get_current_site() method by which you can alter what is returned
from that method. The default implementation returns site by
settings.SITE_ID, but if you use a swappable model, then you can alter the
implementation as you wish (middleware + thread-locals is a likely
approach here...).
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:46>
Comment (by apollo13):
Replying to [comment:45 loic84]:
> If we are serious about having multi-tenancy in core, we need the
current site to be made a thread local and a first class citizen in `
BaseHandler .get_response()`, just like we do for URLCONF. Indeed,
`get_current_site()` and `get_urlconf()` are the two sides of the same
coin.
Absolutely not. First of making the `BaseHandler` aware of the current
site would imply strong coupling of a contrib app to core -- that's an
absolute no-go. I also don't agree with the thread local, there is no need
imo to make it one, `User` for instance works fine without thread locals
> There are plenty of areas of Django that need to be aware of the current
site and that aren't reached (most of the time for good reasons) by the
request object. The remaining uses of `Site.objects.get_current()` in the
codebase (instead of `get_current_site()`) hint at the problem.
If you API requires a Site, pass in a Site object where needed.
> Particularly troublesome if the SITE_ID is only available in the
request:
> - Declarative stuff like sitemaps, default values of model fields, etc.
I agree that this is a problem, but I don't think thread-locals are the
solution, especially since in the shell you don't have access to the
request either.
> - Random methods that don't get the request as parameter even though the
calling method had access to it.
That's a problem in the API.
> - Methods called from templates such as `get_absolute_url()`. Being able
to pass the request as a parameter to such methods is the main reason I've
been assessing Jinja2.
RequestContext?!
> `get_current_site()` would need to be swappable just like `KEY_FUNCTION`
for the cache framework, so people can define their own switching logic.
That was part of my proposal.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:47>
Comment (by loic84):
Replying to [comment:47 apollo13]:
> Replying to [comment:45 loic84]:
> > If we are serious about having multi-tenancy in core, we need the
current site to be made a thread local and a first class citizen in `
BaseHandler .get_response()`, just like we do for URLCONF. Indeed,
`get_current_site()` and `get_urlconf()` are the two sides of the same
coin.
>
> Absolutely not. First of making the `BaseHandler` aware of the current
site would imply strong coupling of a contrib app to core -- that's an
absolute no-go. I also don't agree with the thread local, there is no need
imo to make it one, `User` for instance works fine without thread locals
Django completely ignores one of the main part of an URL (the domain),
it's a blatant omission in my opinion. Having a hook in place in
BaseHandler for people or apps that care about that portion of the URL is
not the same as strong coupling a contrib app. `contrib.sites` could
simply consume that hook.
> > There are plenty of areas of Django that need to be aware of the
current site and that aren't reached (most of the time for good reasons)
by the request object. The remaining uses of `Site.objects.get_current()`
in the codebase (instead of `get_current_site()`) hint at the problem.
>
> If your API requires a Site, pass in a Site object where needed.
I'm not talking about my own API, I'm taking about areas of Django over
which I have no control.
> > Particularly troublesome if the SITE_ID is only available in the
request:
> > - Declarative stuff like sitemaps, default values of model fields,
etc.
>
> I agree that this is a problem, but I don't think thread-locals are the
solution, especially since in the shell you don't have access to the
request either.
Indeed you don't, and that's why I don't want to keep the curent SITE_ID
in the request but in a thread local. In the shell you have control over
1. the settings, and 2. the env, so you could write `SITE_ID=2 ./manage.py
whatever`.
>
> > - Random methods that don't get the request as parameter even though
the calling method had access to it.
>
> That's a problem in the API, not in the sites stuff.
>
> > - Methods called from templates such as `get_absolute_url()`. Being
able to pass the request as a parameter to such methods is the main reason
I've been assessing Jinja2.
>
> RequestContext?!
What do you mean? Right now you are out of luck if you need your
`Model.get_absolute_url()` to be site-aware, for example to return a fully
qualified URL when the site for the current request is different than the
site to which your model instance belongs. I don't see how RequestContext
helps, even if the request is present in the template context, I won't be
able to pass it as an argument to `get_absolute_url`, so
`get_absolute_url` still has no idea about the current site.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:48>
* cc: w2lkm2n@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:49>
* cc: simonkagwe (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:50>
* cc: riccardo.magliocchetti@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:51>
* cc: segfault (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:52>
* needs_better_patch: 1 => 0
Comment:
Removing the patch needs improvement flag to get the patch in the review
queue
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:53>
Comment (by timgraham):
I would like to move this ticket forward in order to address the concerns
in [https://code.djangoproject.com/ticket/21648#comment:5 comment 5] of
#21648 ('Deprecate "is_admin_site" option of
auth.views.password_reset()').
I think Claude's patch is a reasonable start.
[https://github.com/django/django/pull/2460 PR 2460] was also submitted
independently without knowledge of this ticket and is quite similar. Is
anyone against this direction?
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:54>
Comment (by apollo13):
If that helps us internally, by all means apply it.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:55>
Comment (by rm_):
↑ Forward ported the patch to apply cleanly against latest git master.
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:56>
Comment (by timgraham):
Made some updates to the patch:
[https://github.com/django/django/pull/3293 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:57>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"32c7d3c061b83e9206ef2bf13fbc302a1998f317"]:
{{{
#!CommitTicketReference repository=""
revision="32c7d3c061b83e9206ef2bf13fbc302a1998f317"
Fixed #15089 -- Allowed contrib.sites to lookup the current site based on
request.get_host().
Thanks Claude Paroz, Riccardo Magliocchetti, and Damian Moore
for contributions to the patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/15089#comment:58>