Code

Opened 3 years ago

Closed 13 months ago

#15319 closed New feature (wontfix)

_get_next_or_previous_by_FIELD populated from Meta

Reported by: alexr 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

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.

Attachments (0)

Change History (10)

comment:1 Changed 3 years ago by jdunck

  • Cc jdunck@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by russellm

  • Triage Stage changed from Unreviewed to Design 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 3 years ago by alexr

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 3 years ago by alexr

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 3 years ago by russellm

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 3 years ago by alexr

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.

Last edited 3 years ago by alexr (previous) (diff)

comment:7 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to New feature

comment:8 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:10 Changed 13 months ago by aaugustin

  • Resolution set to wontfix
  • Status changed from new to 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.