order_with_respect_to GenericForeignKey

[Alex Hill]

As of 1.8, order_with_respect_to nearly works with generic relations.

First thing that's broken is the get_RELATED_order and set_RELATED_order methods. They are set in ​ModelBase._prepare. You don't know what model is going to be on the other end of the GenericForeignKey, so I think those can simply be omitted if opts.order_with_respect_to.rel is None. (Perhaps something can be done here in cases where a GenericRelation is defined.)

Next, you run into trouble ​when Django tries to save a new object's _order field, because there's no attname attribute on the GenericForeignKey. You run into the same problem in _get_next_or_previous_in_order().

This is easily solved if you're OK with sprinkling details of the GenericForeignKey implementation in Django core: you can just try attname, and if that fails, filter on GFK's ct_field and pk_field. But maybe a better idea would be to introduce a method on field-like things returning a dictionary of filter parameters matching a given object, or else a property returning a sequence of (filter name, attribute name) pairs from which the filter parameters can be constructed. In the latter case, regular fields would return ((self.name, self.attname),), and GFKs would return ((self.fk_field, self.fk_field), (self.ct_field, self.ct_field)).

Here's a branch illustrating the necessary changes: ​https://github.com/AlexHill/django/compare/order_wrt_gfks

[Alex Hill]

Branch illustrating the necessary changes at ​https://github.com/AlexHill/django/compare/order_wrt_gfks

[Alex Hill]

I've now implemented the get_RELATED_order and set_RELATED_order methods where GenericRelations are present, and duplicated the order_with_respect_to tests using generic relationships.

[Tim Graham]

​Pull request is showing as not merging cleanly.

[Alex Hill]

Hey Tim, thanks for reviewing this!

I've just rebased against current master and it now will merge cleanly. However, the code uses add_lazy_relation, which will be deprecated as soon as #24215 is merged, so I would prefer to wait until that happens and then update this patch to use the new API.

#24215 has been thoroughly reviewed by Aymeric, Carl and others and is, I think, ready to be merged pending approval of ​https://github.com/django/django/pull/4122. I would be very grateful if you could have a look over it.

Cheers,
Alex

[Alex Hill]

I've rebased this on top of master now that #24215 is in.

[Tim Graham]

Left some comments for improvement.

[Alex Hill]

Updated to incorporate your suggestions.

[Tim Graham]

This looks okay to me. Could an ORM expert give a +1?

[Anssi Kääriäinen]

The patch needs rebase, and a little deduplication for the relational field filter handling.

[Alex Hill]

I've rebased and deduplicated that logic so it's shared by order with respect to and the descriptors.

[Tim Graham <timograham@…>]

In 7bec480f:

Fixed #24201 -- Added order_with_respect_to support to GenericForeignKey.

[Simon Charette <charette.s@…>]

In c0118ff8:

Refs #24201 -- Ignored order_with_respect_to private fields in migrations.

Thanks Tim for the review.

[Ashley Waite]

Replying to Simon Charette <charette.s@…>:

In c0118ff8:

Refs #24201 -- Ignored order_with_respect_to private fields in migrations.

Thanks Tim for the review.

This commit removes the order_with_respect_to option during migration state if it's a generic foreign key.
I can see why this was done, as private fields are not present on the model state, so it cannot be resolved.

However, as the option is removed, no _order field is created in the database and no AlterOrderWithRespectTo operations can be generated for the migration either.
An AlterOrderWithRespectTo operation cannot be manually added either because the migrations ModelState does not copy private fields either, so attempting to add it results in referring to a field it cannot find.
Manually adding an AlterOrderWithRespectTo operation referring to another field will result in it being removed in any subsequent migration.

When you attempt to use the model it hard breaks because there's no _order field in the database.

psycopg2.ProgrammingError: column test_model._order does not exist
django.db.utils.ProgrammingError: column test_model._order does not exist

This requires several changes in the migration module to resolve _correctly_, but can be hackishly bypassed by replacing ​state.py#446-448 with:

        # Private fields are ignored, so remove options that refer to them.
        elif options.get('order_with_respect_to') in (field.name for field in model._meta.private_fields):
            options['order_with_respect_to'] = model._meta.pk.attname

Which leads to the creation of _order and AlterOrderWithRespectTo operations, thinking that it's ordering according to the id field, but as it migrates and functions successfully, this is the clear source of why it currently breaks.

[Simon Charette]

Thank you for reporting that.

I believe the proper way of handling that would be to add support for private fields in the migration framework. It's not clear to me why this wasn't added in the first place.