The current diff against trunk is attached - the code will, I believe,
work as a standalone module anyway, but if people like this solution I
suggest a twiddle to db/models/base.py to add one more signal call to
make the code a bit nicer, as well as the obvious docs/tests, and
tweaking the coding style, etc.
Still, before I go for anything like that, I'd prefer people to test it
and tell me if I'm mad[1], sad, or if it's actually worth anything. With
the attached diff applied, you just write models like this:
class User(models.Model):
username = models.CharField(max_length=255)
email = models.TextField()
num_photos = models.AggregateField("Photo", "fk:user", "count")
num_all_photos = models.AggregateField("Photo", "all:", "count")
def __unicode__(self):
return "%s <%s>" % (self.username, self.email)
class Photo(models.Model):
caption = models.TextField()
user = models.ForeignKey(User, related_name="photos")
user_username = models.MirrorField("user", "username")
def __unicode__(self):
return "%s by %s" % (self.caption, self.user_username)
Apart from the lack of any actual photos on Photo objects, you get the
idea. Those remembering or reading the early discussion may remember my
original proposal for AggregateField had (queryset, column, function) -
due to various problems, like not being able to create querysets
referring to the right things or "self" (the instance), it's changed to
(model, link, function), where link is "all:" (which means 'run the
function over all members of the other model') or "fk:foo" (which means
'run the function over all members of the other model where the foo FK
points to this instance'). Obviously more link types could be added, but
I can't think of any other useful types yet.
As well as using built-in function names like "count", you're allowed to
pass in custom functions that take a queryset and a column name, and
return a result - when you use these, though, you have to also specify
the column type of the resulting column. Docs for all this will be
written in time.
So, what does everyone think? Useful, or just too much of an edge case?
Reasonable patch, or badly implemented?
Andrew
[1]: As an example of the insanity of some of this code, at one point it
changes the inheritance tree of its own subclass at runtime (line 86).
Better solutions to inheriting from an unknown-at-creation field type on
a postcard, please.
First off, details and implementation notwithstanding, I'm +1 for
adding denormalization fields of this sort. Apologies for not getting
involved during the first pass of this discussion.
I do have a slight reservation regarding the API you are proposing.
The fields you have proposed (MirrorField and AggregateField) capture
the obvious use cases, but the syntax don't feel consistent with the
rest of the Django API. In particular, AggregateField seems to be
introducing a whole new query syntax that is completely independent of
the aggregation syntax that is due for inclusion in v1.1.
I would prefer to see a syntax that demonstrated slightly better
integration with the Django query syntax. If it isn't possible to use
QuerySet itself as a way to develop the underlying query, then I would
expect to see a syntax that at the very least retained the flavour of
the Django query syntax rather than inventing "fk:" style operators
that have no real analog.
MirrorField is essentially a query that returns a related object;
AggregateField is a query that returns an aggregate of related
objects. Looking longer term, this sort of approach also has the
potential to be integrated into database views, since defining a view
requires similar querying capabilities.
Unfortunately, I haven't given this enough thought to be able to
suggest an alternate syntax that will do the job. At this point, I
just know that I would rather avoid introducing 2 different syntaxes
for aggregates. I have a hazy vision in my head of the sort of thing
that I think could work; I'll try to get it to coalesce a little and
get back to you.
Yours,
Russ Magee %-)
Indeed; I was far happier with the queryset syntax for AggregateField I
originally proposed, but I couldn't get it to work - it just won't go
inside class bodies without far, far too much hackery. The MirrorField
API I like more; I have an alternate version where you simply pass it
the model name and column name, but that then has issues if you have
more than one FK to the same model.
The AggregateField proposal is just that, and I really squirmed at the
idea of "all:" and "fk:", but it got it working pretty quickly. One
possibility for more clean integration is an approach more like
querysets, so you could have something like
photos_count = models.AggregateField(Photo.view.filter(user_self=True))
The main problem with anything like this (and indeed with my attempt to
implement this with plain old querysets) is that, as a field, you exist
only on a class, and so when you get a signal saying a model has been
updated, it's somewhat difficult to determine which instances of
yourself to update - you can't loop through them all testing, since
that's hilariously inefficient. That's why I limited the querying to
only fk: and all: types, since detecting these is far more efficient.
I'd love to see any proposal for how Aggregates should look, as my
current incarnation really isn't too django-ish. I personally like
MirrorFields (now on their third name, more welcomed) as I have them
now, but then I would, since I invented them for my own use...
Andrew
I obviously agree, but Russ' point about it being ugly still stands, and
the only improvement I've been able to think of so far is changing
"fk:foo" into fk="foo", so you get something like:
models.AggregateField("Person", fk="nation", function="count")
i.e. the same call, but different syntax.
I might just go open a ticket for this soon anyway to get it recorded
properly, and at the very least release the code as a standalone module
- it's written so it can work like that anyway.
Andrew
Sebastian
I see two problems with this:
1. This current behavior is fully documented [1]. In relevant part,
the documentation says:
> This doesn't control whether or not the user can log
> in. Nothing in the authentication path checks the
> is_active flag, so if you want to reject a login based
> on is_active being False, it is up to you to check that
> in your own login view. However, permission checking
> using the methods like has_perm() does check this
> flag and will always return False for inactive users.
2. Many people have expected the current behavior for some time and
such a change would be backward incompatible.
Although, I do see that the documentation specific to the
login_required view does not specifically mention the behavior.
Perhaps a note there would be beneficial.
[1]: http://docs.djangoproject.com/en/dev/topics/auth/#api-reference
--
----
Waylan Limberg
way...@gmail.com
I think you can write your own decorator to do that, and it would be a
backwards-incompatible change, so it isn't likely to happen :)
--
Collin Grady
but we can do this "little" modification in v1.1
i think this change will allow site administrator to ban users through change is_active flag
Waylan Limberg pisze:On Mon, Nov 10, 2008 at 2:49 PM, Sebastian Bauer <ad...@ugame.net.pl> wrote:Hello, i think login_required should check that user is not only authenticated, but also if is active. What do you think about this change?I see two problems with this: 1. This current behavior is fully documented [1]. In relevant part, the documentation says:This doesn't control whether or not the user can log in. Nothing in the authentication path checks the is_active flag, so if you want to reject a login based on is_active being False, it is up to you to check that in your own login view. However, permission checking using the methods like has_perm() does check this flag and will always return False for inactive users.
Well, callables are a lot harder to deal with; there's no efficient way
to hook onto when they've changed. It would be possible if you passed
along a foreign key to watch for changes, although at that point you
could probably also implement it using an AggregateField of some kind,
since they already take arbitary aggregate functions.
The problem is the 'self' thing, though. However, I like the
filter()-style syntax, I'll need to see if that's easy enough to roll in
so it still makes sense...
Andrew
Ah yes, ignore me. I was trying to see how you did 'efficient updates'
(where, say, only half of a model's instances are affected by the change
that just occurred) but I now see it's the function's job to do that.
I quite like this solution, it's rather clean; my own solution is far
more lengthly, and while some of that is the efficiency code I
previously mentioned, I can't help but think I have some redundancy in
there now...
I've published my code at http://code.google.com/p/django-denorm/, and
I'll 'release' it soonish, but I'm liking this decorator approach more
and more, especially as we can then just build 'AggregateFields' on top
of them.
Andrew
I still have two issues, although I'm pretty sure both can be fixed.
One: This is the MirrorField equivalent in the example:
@denormalized(models.CharField,max_length=100)
@depend_on_related(User)
def owner_username(self):
return self.owner.username
While it's really obvious what it's doing, I (for obvious reasons) would
like to have common use cases like that shortened, and the number of
things I have to type reduced (I only need one of the other model or the
foreign key attribute name, and here I need both). I've tried writing a
wrapper for MirrorField, but I'm running into the same issue I had
(which is that you have to do it on the class_prepared signal). I'll
probably have it working soon, though.
The second thing is that it doesn't play nice with South, but that's
almost certainly my fault, and nothing to do with you. If we hadn't
started scanning source code for the field definitions and ripping them
out it might have been fine, but it's proved to be the only real way...
Basically, I'm moaning as it doesn't work with my peculiar setup :)
Still, 'tis nice overall. I have a partially-complete unit test suite
for my denorm code I'd be happy to port over (it has the wonderful set
of tricks you need to unit test with a whole fake app, while still
keeping database settings) if you want.
Andrew
Yeah, it pretty much is. I did it in my original code, but only by
hooking into class_prepared to then traverse across the foreign key, and
_then_ dynamically reparent the field class to the right parent. It's
python, so it's possible, but that doesn't mean it's sensible. Besides,
I'm not sure all Python implementations like you fiddling with __bases__.
> This is relevant for me to, as i use south for my migrations.
>
Ooh, excellent :) I've fixed this, anyway; South has provision for more
magical fields, and if they have a south_field_definition function will
use that instead of source file scanning, so one more method on the
DBField subclass and it's playing very nicely. I am now happy.
> This would be great, my current test coverage is rather small and
> better/more
> tests will cerainly be exremly usefull when trying to optimise the
> code.
>
The start of this and the South stuff is over on my branch:
http://github.com/andrewgodwin/django-denorm/tree/master
The unit test 'suite' is hardly finished yet (one test...), but it does
at least do a pretty thorough use check of some of the more basic
scenarios. ./manage.py test denorm does indeed test and come out OK,
though - it even brought up an infinite recursion bug I also had in my
code, which I've fixed (there may be a cleaner fix).
Andrew
Ah, the wonderful meta-loop of fix fixes.
> i think we should really setup a trac instance somewhere...
I have a Trac setup around on my server for South, so if you want I'll
kick that slightly and install the Git plugin...
Andrew