Opened 8 years ago

Closed 6 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: master
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


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 Changed 8 years ago by Jeremy Dunck

Cc: jdunck@… added

comment:2 Changed 8 years ago by Russell Keith-Magee

Triage Stage: UnreviewedDesign decision needed

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.

comment:3 Changed 8 years ago by Alex Robbins

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 Changed 8 years ago by Alex Robbins

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 Changed 8 years ago by Russell Keith-Magee

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 Changed 8 years ago by Alex Robbins

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.

Last edited 8 years ago by Alex Robbins (previous) (diff)

comment:7 Changed 8 years ago by Łukasz Rekucki

Severity: Normal
Type: New feature

comment:8 Changed 7 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 Changed 7 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 Changed 6 years ago by Aymeric Augustin

Resolution: wontfix
Status: newclosed

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.

Note: See TracTickets for help on using tickets.
Back to Top