Opened 14 years ago
Closed 12 years ago
#15319 closed New feature (wontfix)
_get_next_or_previous_by_FIELD populated from Meta
Reported by: | Alex Robbins | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | jdunck@… | Triage Stage: | Design decision needed |
Has patch: | no | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Right now date field and datetime fields use contribute_to_class to add a 'get_next_by_<field name>'. If you look at the code in _get_next_or_previous_by_FIELD, it is pretty general, and should work with any field with a stable sort order.
Maybe instead of creating get_next_by_FIELD and get_previous_by_FIELD based on field type, we could create them based on a Meta attribute in the class. I'm thinking we could call the Meta attribute seekable_fields or something like that.
For instance:
class Person(models.Model): first_name = models.CharField(max_length=255) last_name = models.CharField(max_length=255) class Meta: seekable_fields = ('last_name', )
The resulting person objects would have methods named 'get_next_by_last_name' and 'get_previous_by_last_name'.
I think the benefit here is that we are able to specify the fields we want to seek by, instead of only being able to use date fields.
Eventually, we could drop the contribute_to_class methods for the date field, since it is just a special case that could be handled by including the name of any date field you wanted to be able to sort by.
Change History (10)
comment:1 by , 14 years ago
Cc: | added |
---|
comment:2 by , 14 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 14 years ago
Yeah, I'd be just as happy if I were able to specify seekable (or whatever if gets called in the end) on the fields.
We'd also need to check that it wasn't set for a field that was the primary key. (That would have been an issue in my original proposal too.) I think the lookup adds some pk fallback code that might get weird if it was already seeking by pk.
comment:4 by , 14 years ago
Oh, I think the order_with_respect_to meta attribute is similar to this, and it lives in the Meta class. Similar in that it references a field, and then adds extra seeking methods on the model. Not sure if we want to try and remain consistent with that.
It is different in that it can only be ordered with respect to one field, so it really is more of a 'Model' level option instead of a field level option.
comment:5 by , 14 years ago
Like you note, order_with_respect_to is slightly different, because it deals with full-model ordering. It wouldn't make sense to define order_with_respect_to on two different fields -- it needs to be defined as a property of the entire model. So I'm not so concerned about API consistency with that particular feature.
Not sure I see why this would be a problem with PKs, though -- get_next_by_FOO is really just MyObj.objects.filter(FOO__gt=self.FOO)[0], so I can't see any reason that PK values need to be excluded.
comment:6 by , 14 years ago
http://code.djangoproject.com/browser/django/trunk/django/db/models/base.py#L596
I think the idea is to fall back to ordering by pk so that nothing is missed if the seekable field has duplicate values.
If we ordered by FOO, which was also the pk field, we'd end up with a query like this (for get_next):
MyObj.objects.filter(Q(FOO__gt=self.FOO)|Q(FOO__gt=self.FOO)).order_by('FOO', 'FOO')
Looking at it, I guess that query would run ok, it just looks weird.
comment:7 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:10 by , 12 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Actually get_next/previous_by is an operation on a QuerySet, not on instances. I'd prefer an API like #16505. This doesn't require contribute_to_class
nor special casing date fields.
I accept the broad principle that it should be easier to easily add "get_X_by_Y" style fields for any field type, not just date fields.
However, my inclination isn't to take this away from individual fields, but to add this as an extra feature for fields -- i.e., for me, this ins't a model-level issue, it's a field configuration issue, and you should be able to say
last_name = models.CharField(max_length=255, seekable=True)
(bikeshedding the attribute name), false by default for most fields, but True by default for DateFields et al.