Proposal: validate urlconfs when the server starts
After discussing this on IRC, I decided that this idea could be worthwhile, but I thought I'd post it here before creating a ticket for it. Disclaimer: I am using version 1.0, so I can't say whether the behavior I observed is exactly the same in the dev version. Since nobody told me that anything had changed on IRC, I'm assuming that it didn't. When passing strings as view names in urlconfs, the corresponding modules will be loaded when the views are called (and not when the server starts). I am a newbie, I am currently building my first web app with django, and I made the mistake not to define a function inside a view that I referenced from an urlconf (as a string). The result was that I got weird and seemingly random errors when I loaded pages sometimes - but not always, and I thought that I was dealing with some strange bug. It did occur to me that I might have messed up my config _somewhere_, but I am pretty sure that it would have taken me a long time to figure out what was going on if I hadn't gone on IRC - because I was assuming that the urlconf had to be valid (if suboptimal), because it didn't fail loudly, and considering to remove that assumption from my mindset would have taken me a while. I think that this issue has potential for tripping up newbies quite easily. While I was told on IRC that passing strings instead of modules/functions as views in urlconfs is a bad practice, the tutorial does this, and I think that it can be expected that many newbies will imitate that. My proposal is to validate urlconfs whenever the server starts, for example by making the mapping from urlconf to view non-lazy, or by doing a check (a test import, for instance) for every urlconf. This would have the added benefit that the validation routine could also check for other potential sources of bugs (for instance, two url patterns having the same name). I see no reason not to fail if the configuration is broken - the web app is broken and will have to be fixed anyway. However, I am new to django, and understand that I might be missing something implementation-specific. Well, thanks for reading :) If I get positive feedback about this, I'll write a ticket, and at least look into implementing it in some way. cheers, Thomas -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Status of InstalledAppsRevision, soc2010/app-loading branch?
It looks like the last discussion on this was back in Apr 2010, and the last trunk-merge was 4 months ago. The last comment on #3591 by russelm was that the GSOC branch "is looking reasonably good".So, my question is, is there still any interest in getting this merged into trunk, or is there still a design decision needed? -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: contrib.sites and multitenancy
Thanks legutierr for all your work on this ticket and patch, and everyone for the comments. I just took some time to review the patch on #15089 and had a long conversation on IRC with legutierr, and here's what I'm thinking: It does appear that there is some code (CurrentSiteManager, for instance), that wants a Site object and simply cannot pass in the request (barring threadlocals, which is not an option for core/ contrib). I don't think the addition of multitenancy justifies breaking currently-working code. So I think we need to add support for multitenancy (which I'm using here as shorthand for a hook that takes a request and returns a Site object or something compatible) in such a way that it is explicitly enabled by setting, and calling code that fails to provide the request only fails (and in a clear way) when multitenancy has been explicitly enabled. There's also the issue of what third-party code does when it depends on contrib.sites and needs an actual Site model instance, not a RequestSite or some other replacement. Contrib.flatpages is an example of this -- the flatpage model has an FK to Site, so it doesn't make sense for flatpages to call a method which might return a Site object or might return something else. It needs a real Site object. In my mind, this use case is why deprecating Site.objects.get_current() is not a good option. The current patch on #15089 deprecates Site.objects.get_current() and then adds a "require_site_object" flag to get_current_site(), but this seems backwards to me. There's already a well-established indicator that you want an instance (or queryset) of a certain model, and that is to call a manager method on that model. I'd rather keep Site.objects.get_current() as the supported API for when the calling code depends on the Site model (because, e.g., it's querying the database for another model with an FK to Site), and get_current_site() as the API for when the calling code doesn't care whether contrib.sites is used or not and just wants something with a domain name. (I think this distinction is already somewhat implied by how the two methods are currently used in the Django codebase, but could be better documented. And they could probably named more clearly as well, but that ship may have sailed). In my mind, the introduction of a multitenancy hook (in the 1.4 timeframe) could then look something like this: * Introduce a SITES_BACKEND setting, which defaults to None but can be set to a dotted path to a callable that takes the request and returns a Site object (or something else entirely, which should quack like a Site/RequestSite if the dev wants to use third-party code, but isn't required to). * Add an optional "request" argument to Site.objects.get_current(). If SITES_BACKEND is set and Site.objects.get_current() is not passed the request, it's an error. But existing code, with no SITE_BACKEND, continues to work fine calling Site.objects.get_current() with no request. * Both Site.objects.get_current() and get_current_site() call SITE_BACKEND, if its set, and return whatever it returns. Site.objects.get_current() should check this return value and error if its not an instance of the Site model. get_current_site() doesn't care what it returns. * If SITE_BACKEND is not set, both Site.objects.get_current() and get_current_site() behave exactly as they do now. The result is that it's fully backwards-compatible, with no need to deprecate anything that works now. If you choose to set a SITE_BACKEND, then you must use third-party code that's been updated to either call get_current_site() or pass a request to Site.objects.get_current(). Thoughts? Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Ticket #15124: BooleanField should not use False as default (unless provided)
Models with a BooleanField are instantiated with that field's value set to False if no default or initial value is provided. Instead, like most other fields, the field's initial value should be set to None. This None should be left uncoerced when attempting to save the instance, so attempting to save should result in a database error for trying to set null on a non-null field. Although this change is backwards incompatible, that depends how far backwards you go. Apparently the behavior was as I think it should be at the time #2855 was created. The facts that (1) the behavior has changed, and (2) #2855 was wontfix'd together make a strong case that this should be considered a bug, and therefore should be changeable immediately despite being backwards incompatible. Links: http://code.djangoproject.com/ticket/15124 http://code.djangoproject.com/ticket/2855 -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Status of InstalledAppsRevision, soc2010/app-loading branch?
I haven't been tracking it closely, but last I heard on the subject is that it's on target for the 1.4 release cycle. So basically once we get 1.3 pushed out (and have a little rest) that'll be towards the top of the heap to be wrapped up and worked into trunk. There's almost certainly still some polishing that needs to happen on it, but like Russ said, it's looking reasonably good. All the best, - Gabriel -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: contrib.sites and multitenancy
On Wednesday, January 19, 2011 12:01:57 PM UTC-8, Carl Meyer wrote: > > Contrib.flatpages is an example > of this -- the flatpage model has an FK to Site, so it doesn't make > sense for flatpages to call a method which might return a Site object > or might return something else. It needs a real Site object. In my > mind, this use case is why deprecating Site.objects.get_current() is > not a good option.The current patch on #15089 deprecates I'm very much in agreement here. This is what I ran into when I first tried to replace all the calls to Site.objects.get_current_site() with get_current_site()... there simply *are* cases where a Site object is required, and using the object manager is the right way to do it, as you pointed out. > In my mind, the introduction of a multitenancy hook (in the 1.4 > timeframe) could then look something like this: > Also agreed on the 1.4 timeframe. While there is a valid concern for making sure that get_current_site() (introduced in 1.3) doesn't end up getting changed in 1.4, this whole task seems like it's snowballing a small feature whose main purpose was to be more DRY into a large feature with a totally different goal, and doing so very late in the game for the 1.3 release cycle. > * Add an optional "request" argument to Site.objects.get_current_site(). If > > SITES_BACKEND is set and Site.objects.get_current() is not passed the > request, it's an error. But existing code, with no SITE_BACKEND, > continues to work fine calling Site.objects.get_current() with no > request. > I'm a little uneasy about this... I think it would be an error for Site.objects.get_current() to ever return an object which is not a Site instance, and if we're dealing with arbitrary callables from a SITES_BACKEND setting being responsible for returning a Site-like object it seems like this could break in non-obvious ways. What that means to me is that while you might want Site.objects.get_current_site() to have multiple ways of retrieving the current Site object based on data in the request, I don't think those should be conflated with the arbitrary SITES_BACKEND callables. I'd rather see Site.objects.get_current_site() try more ways of getting the current site if a request is passed in, and/or have a separate method on the Site manager that can take a request and do request-based lookups. Expand the options on the built-in manager but keep them finite. I'd say all the SITES_BACKEND functionality should remain tied to get_current_site() and out of the Site manager. > * Both Site.objects.get_current() and get_current_site() call > SITE_BACKEND, if its set, and return whatever it returns. > Site.objects.get_current() should check this return value and error if > its not an instance of the Site model. get_current_site() doesn't care > what it returns. > See my previous comment. Raising an exception if the SITES_BACKEND callable returns an object of the wrong type is one way to handle it, I'm just not sure it's my favorite solution. Seems brittle given that a developer using a custom callable might not think about how other apps (especially in contrib) are going to interact with the return value of their callable. That's what I've got for now. Thanks to legutierr for the work on the patch so far. All the best, - Gabriel -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Proposal: validate urlconfs when the server starts
If you try this in any of the more recent versions (sorry I don't know exactly when this was changed) you'll get a ViewDoesNotExist exception telling you which view in which module it couldn't find. If you have DEBUG = True you'll get a nice Django 500 error debug page giving you everything you need to diagnose the problem. If you have DEBUG = False you'll get an exception in your error log that you can use to debug. This will happen regardless of whether or not the invalid function is actually called or referenced by your templates or other code. And for the record, there's absolutely nothing wrong with using strings instead of actual functions in your URLConf. In fact, I would go so far to say that it is the *preferred* method, and that's why it's in the tutorial. The tutorial (to the best of our abilities) does not contain anything that the core team and/or the community considers "bad practice". Try it out with a current version of Django. If you think there's something more that should be added, feel free to write back. All the best, - Gabriel -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: contrib.sites and multitenancy
Hi Gabriel, On Jan 19, 5:55 pm, Gabriel Hurley wrote: > > In my mind, the introduction of a multitenancy hook (in the 1.4 > > timeframe) could then look something like this: > > Also agreed on the 1.4 timeframe. While there is a valid concern for making > sure that get_current_site() (introduced in 1.3) doesn't end up getting > changed in 1.4, this whole task seems like it's snowballing a small feature > whose main purpose was to be more DRY into a large feature with a totally > different goal, and doing so very late in the game for the 1.3 release > cycle. Yes, I agree that it's worth thinking about what we'll want to do in 1.4, and consider making necessary changes to the new get_current_site() API before 1.3. At this point I don't foresee needing backwards-incompatible API changes to get_current_site() in order to add multitenancy, so I don't think I see anything that needs to be done pre-1.3. > > * Add an optional "request" argument to Site.objects.get_current_site(). If > > > SITES_BACKEND is set and Site.objects.get_current() is not passed the > > request, it's an error. But existing code, with no SITE_BACKEND, > > continues to work fine calling Site.objects.get_current() with no > > request. > > I'm a little uneasy about this... I think it would be an error for > Site.objects.get_current() to ever return an object which is not a Site > instance, and if we're dealing with arbitrary callables from a SITES_BACKEND > setting being responsible for returning a Site-like object it seems like > this could break in non-obvious ways. > > What that means to me is that while you might want > Site.objects.get_current_site() to have multiple ways of retrieving the > current Site object based on data in the request, I don't think those should > be conflated with the arbitrary SITES_BACKEND callables. I'd rather see > Site.objects.get_current_site() try more ways of getting the current site if > a request is passed in, and/or have a separate method on the Site manager > that can take a request and do request-based lookups. Expand the options on > the built-in manager but keep them finite. > > I'd say all the SITES_BACKEND functionality should remain tied to > get_current_site() and out of the Site manager. Hmm. I agree that the interaction of SITES_BACKEND and Site.objects.get_current() is a relatively weak spot in this proposal, but I like this approach even less. I very much want to confine all the new behaviors to SITES_BACKEND (some common sample backends can be provided), and leave current behavior untouched if you don't define SITES_BACKEND, rather than add new builtin behaviors unrelated to SITES_BACKEND (as the current patch does). Otherwise things get more confusing, it's harder to know how to signal what behavior you want, and maintaining backwards compatibility is trickier. I also think it's important to maintain compatibility between Site.objects.get_current() and get_current_site(); just have the former be a stricter version of the latter. I think it's much better for people who define an unusual SITES_BACKEND to get explicit immediate errors if they have code calling Site.objects.get_current(), telling them in plain terms that they can't use code that depends on the Site model if their SITES_BACKEND returns something else, rather than have a situation where people define SITES_BACKEND and then it only takes effect some of the time (when get_current_site is called). The latter is less explicit, doesn't fail fast, and most importantly doesn't let you define a custom SITES_BACKEND that _does_ return a Site object, and have it take effect when Site.objects.get_current() is called, which you'd almost certainly want in that case. > > * Both Site.objects.get_current() and get_current_site() call > > SITE_BACKEND, if its set, and return whatever it returns. > > Site.objects.get_current() should check this return value and error if > > its not an instance of the Site model. get_current_site() doesn't care > > what it returns. > > See my previous comment. Raising an exception if the SITES_BACKEND callable > returns an object of the wrong type is one way to handle it, I'm just not > sure it's my favorite solution. Seems brittle given that a developer using a > custom callable might not think about how other apps (especially in contrib) > are going to interact with the return value of their callable. I think it's less brittle and less confusing than the alternatives. It's a pretty clear distinction: the only code that should ever use Site.objects.get_current() is code that explicitly depends on the Site model being used. If you're defining a SITES_BACKEND that returns something other than a Site model, you aren't using the Site model, and therefore you can't use code that explicitly depends on the Site model. Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscr
Re: contrib.sites and multitenancy
Carl and I had a long discussion in IRC about all the issues he raises above. I am still digesting that conversation, but there are some things that already spring to mind. 1. I can see the merits of defining a SITE_BACKEND api that would allow users to choose from different site-retrieval implementations that sites framework would contain. I wonder, would it make sense to make SITE_BACKEND the central aspect of the framework? SITE_BACKEND could be the required setting (instead of SITE_ID), and a "get_orm_site_by_id" backend, a "get_orm_site_by_request" backend, and a "get_request_site" backend could all be defined. That way, the documentation could say: "Always define a SITE_BACKEND, here are the different implementations to choose from." It would provide a good opportunity to discuss the different architectures in parallel, and would require users to explicitly choose from among mutually-explicit specific strategies, with only one setting. As it stands now with both of our proposals, documenting how all this works requires describing a very intricate and confusing conditional tree. Making the SITE_BACKEND control everything would simplify documentation significantly. 2. One difference between our proposals is that I propose that the framework's API define a *single*, unitary facade function--"get_current_site()", which takes a an optional "require_site_object" as a parameter--while Carl proposes that the framework define two facade functions--"get_current_site()", and the "Site.objects.get_current()" manager method. I see these two approaches as logically equivalent. Both proposals recognize the need to differentiate between the case where any generic site object may be returned (RequestSite, django.contrib.sites.models.Site, or some user-defined object), and the case where only django.contrib.sites.models.Site may be returned. Carl's proposal is more consistent with the Django practice of putting orm-object-retrieval logic inside managers, and avoids having to deprecate Site.objects.get_current(). My proposal more closely follows the facade design pattern, and I believe simplifies the documentation and thus simplifies things for users. I would favor my proposal, obviously, but I can understand the merits of Carl's. Also, Carl's proposal has the benefit of being what is already checked in. 3. In spite of the fact that having two facade functions does abrogate the need to define a "require_site_object" parameter in get_current_site, I think an argument can be made to include the "require_site_object" parameter in the API for SITE_BACKEND. Carl proposes verifying the object returned by SITE_BACKEND before returning from the manager method, to make sure it is in fact a Site object. If a user-defined SITE_BACKEND is coded to retrieve a custom object from the db, then an unnecessary database call can be prevented if "require_site_object" is passed in and used to return None at the top of the function. 4. I think it is important to keep in mind what the original motivation was behind #15089 and the post that started this thread: to remedy architectural inadequacies that forced the Satchmo devs, in their implementation of django-threaded-multihost, to monkey-patch SiteManager. Although using thread-locals to store the current request is something that Django frowns upon, it is the only way to achieve certain multitenancy behaviors, and seems to be required by a decent number of users (between them, django-threaded-multihost and django-multihost have 37 followers and 4 forks, and I am sure there are a number of other implementations out there). If SITE_BACKEND is called only when a request is supplied as an optional argument to Site.objects.get_current(), then the thread locals strategy employed by django-threaded-multihost et al. will still require monkey-patching SiteManager. If SITE_BACKEND is being called inside Site.objects.get_current(), it *should* be called *regardless* of whether a request is passed into it. If a particular SITE_BACKEND requires that a request be passed in for it to do properly retrieve a site, then that backend should raise an exception; it should not be that such a SITE_BACKEND is simply ignored. This is distinct from Carl's proposal, but I think not a significant deviation from it. Regards, Eduardo -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
Re: Ticket #15124: BooleanField should not use False as default (unless provided)
This seems right to me. Does anyone with more history on BooleanField know of any reason why the "wontfix" resolution on #2855 was incorrect and the current default-to-False behavior is correct? Carl -- You received this message because you are subscribed to the Google Groups "Django developers" group. To post to this group, send email to django-developers@googlegroups.com. To unsubscribe from this group, send email to django-developers+unsubscr...@googlegroups.com. For more options, visit this group at http://groups.google.com/group/django-developers?hl=en.
