Opened 8 years ago

Closed 7 years ago

Last modified 6 years ago

#4438 closed (invalid)

Middleware for contrib.sites to get the site matching the current host name

Reported by: Max Battcher <me@…> Owned by: adrian
Component: Contrib apps Version: master
Severity: Keywords:
Cc: ferringb@…, xphuture@…, joesaccount@…, flosch@… Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Scenario: A (contrived) blog host (or any other sort of user-content-based provider) wants to easily let users sign up for myblogname.example.com rather than sticking them at example.com/myblogname/ or worse.

SetSiteFromHost is a very simple middleware option perfect for django.contrib.sites.middleware that changes django.conf.settings.SITE_ID to the id of the Site with the same site.domain as request.META['HTTP_HOST'] if such a site exists (otherwise it leaves it as set in settings, which is useful for a default site (for errors or whatnot)). This allows a wildcard VirtualHost setup to be easily created for a django website whose applications support django.contrib.sites. To add a new supported site, ready for content, the registration form should just need to create a new Site object. No mucking with generating VirtualHost configs, settings files, or even burdening existing views with the overhead of site discovery. The trade-off here is that an additional query is run by the middleware than for a site that has an explicit SITE_ID that doesn't vary on the host.

I apologize for not having explicit tests or a documentation patch, but I wanted to get a feel for the comments/interest in adding this to Django SVN before seeking out the test exmaples and best practices for Middleware. I thought this was an almost obvious middleware for django.contrib.sites to have and am almost surprised it doesn't. One improvement to my simple first attempt that is attached is that it should be easy enough to add support to cache the request.META['HTTP_HOST'] -> site.id look up for subsequent page views in the hopes that might even further mitigate the performance trade-off of using this middleware over an explicit and unchanging SITE_ID.

Attachments (3)

middleware.py (521 bytes) - added by Max Battcher <me@…> 8 years ago.
SetSiteFromHost middleware
get_from_host.patch (2.9 KB) - added by SmileyChris 8 years ago.
get_from_host.2.patch (3.2 KB) - added by SmileyChris 7 years ago.

Download all attachments as: .zip

Change History (18)

Changed 8 years ago by Max Battcher <me@…>

SetSiteFromHost middleware

comment:1 Changed 8 years ago by Simon G. <dev@…>

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 follow-up: Changed 8 years ago by SmileyChris

We don't ever change settings so I'm not sure this is an accepted way to handle this problem.

A more explicit way to handle this may be to have another helper function on SiteManager. Something like:

    def get_current_from_request(self, request):
        from django.conf import settings
        try:
            return self.get(domain=request.META.get('HTTP_HOST'))
        except Site.DoesNotExist:
            return self.get(pk=settings.SITE_ID)

...which isn't as transparent (and doesn't affect CurrentSiteManager) but it doesn't mess with settings.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 8 years ago by Max Battcher <me@…>

Replying to SmileyChris:

We don't ever change settings so I'm not sure this is an accepted way to handle this problem.

Is there any specific reason for this policy? Is it spelled out explicitly anywhere? I have to admit, this is the first time I've heard it put that way, and I wouldn't know where to search for anything like that in the documentation. I'd appreciate a link if you have one...

I realize that settings shouldn't be modified lightly, but disallowing all modifications seems too strict. Also, if that were the case, why not make them read only properties on import into django.conf? Regardless, IMHO, SetRemoteAddrFromForwardedFor munges request.META and to me that seems it should be more sacrosanct (data from the request environment/http server) from middleware than settings...

Note: I'm not suggesting this middleware should be installed by default, only that it be provided as a possible helpful tool, with perhaps a warning that it affects settings.SITE_ID.

A more explicit way to handle this may be to have another helper function on SiteManager. Something like:

    def get_current_from_request(self, request):
        from django.conf import settings
        try:
            return self.get(domain=request.META.get('HTTP_HOST'))
        except Site.DoesNotExist:
            return self.get(pk=settings.SITE_ID)

...which isn't as transparent (and doesn't affect CurrentSiteManager) but it doesn't mess with settings.

I had and was unsatisfied with several similar solutions to that in the past few months. The above solution just doesn't feel Pythonic-enough to me. Why should a Manager need to know anything about Request? Not to mention that the above solution can't be used with standard generic views; you have to wrap them in a special view to build your QuerySet from the request context, which isn't very DRY having to wrap every single view you call in some view wrapper. When that is the case it is usually a sign to start looking to middleware (or context processors) to perform that task, and hence the decision that I came to that a middleware would be the preferred approach.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 8 years ago by SmileyChris

Replying to Max Battcher <me@worldmaker.net>:

Replying to SmileyChris:

We don't ever change settings so I'm not sure this is an accepted way to handle this problem.

Is there any specific reason for this policy? Is it spelled out explicitly anywhere? I have to admit, this is the first time I've heard it put that way, and I wouldn't know where to search for anything like that in the documentation. I'd appreciate a link if you have one...

http://www.djangoproject.com/documentation/settings/#altering-settings-at-runtime

comment:5 in reply to: ↑ 4 Changed 8 years ago by Max Battcher <me@…>

Replying to SmileyChris:

http://www.djangoproject.com/documentation/settings/#altering-settings-at-runtime

Thanks.

I still think that this is enough of an edge case to warrant bending the advice there. It does say "should not" rather than "must not". I would assume this process_request middleware is a fine exception, particularly for a setting that is used by a contrib app, rather than a core setting.

comment:6 Changed 8 years ago by SmileyChris

Well we've entered the design decision stage, take the discussion to the developers group.

comment:7 Changed 8 years ago by mtredinnick

  • Resolution set to invalid
  • Status changed from new to closed

The ticket system is a really bad place to talk about hypothetical features. Please start a thread on django-developers about this, as Chris mentions in the previous comment.

I'm going to close this as invalid for now, becuase although the problem itself is perfectly valid, you are proposing a particular solution that has a few problems and can probably be solved in other ways. Let's talk about the problem and possible solutions on the mailing list and then.

comment:8 Changed 8 years ago by DvD <birrafondaio@…>

Please, do not let this middleware lie forgotten somewhere, i think it should be added to the django code

comment:9 Changed 8 years ago by mir

djangosnippets.org is a good place for stuff like this.

Changed 8 years ago by SmileyChris

comment:10 Changed 8 years ago by SmileyChris

  • Resolution invalid deleted
  • Status changed from closed to reopened
  • Summary changed from Proposed additon to django.contrib.sites: middleware.SetSiteFromHost to Middleware for contrib.sites to get the site matching the current host name

I'm reopening because my patch is a valid solution without the problems of changing settings.

comment:11 Changed 7 years ago by (removed)

  • Cc ferringb@… added

Changed 7 years ago by SmileyChris

comment:12 Changed 7 years ago by jacob

  • Resolution set to invalid
  • Status changed from reopened to closed

Malcom's note to take this to the mailing list still applies.

comment:13 Changed 7 years ago by anonymous

  • Cc xphuture@… added

comment:14 Changed 6 years ago by jos3ph

  • Cc joesaccount@… added

comment:15 Changed 6 years ago by anonymous

  • Cc flosch@… added
Note: See TracTickets for help on using tickets.
Back to Top