[Django] #24201: order_with_respect_to GenericForeignKey
Django
----------------------------------------------+--------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
Component: Database layer (models, ORM) | Version:
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
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
[[https://github.com/django/django/blob/d450af8a2687ca2e90a8790eb567f9a25ebce85b/django/db/models/base.py#L320|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
[https://github.com/django/django/blob/d450af8a2687ca2e90a8790eb567f9a25ebce85b/django/db/models/base.py#L796
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))`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_docs: => 0
* has_patch: 0 => 1
* needs_tests: => 1
* needs_better_patch: => 1
Comment:
Branch illustrating the necessary changes at
https://github.com/AlexHill/django/compare/order_wrt_gfks
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Changes (by AlexHill):
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 1
Description changed by AlexHill:
Old description:
> 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
> [[https://github.com/django/django/blob/d450af8a2687ca2e90a8790eb567f9a25ebce85b/django/db/models/base.py#L320|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
> [https://github.com/django/django/blob/d450af8a2687ca2e90a8790eb567f9a25ebce85b/django/db/models/base.py#L796
> 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))`.
New description:
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
[[https://github.com/django/django/blob/d450af8a2687ca2e90a8790eb567f9a25ebce85b/django/db/models/base.py#L320|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
[https://github.com/django/django/blob/d450af8a2687ca2e90a8790eb567f9a25ebce85b/django/db/models/base.py#L796
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
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_tests: 1 => 0
Comment:
I've now implemented the `get_RELATED_order` and `set_RELATED_order`
methods where `GenericRelation`s are present, and duplicated the
order_with_respect_to tests using generic relationships.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
[https://github.com/django/django/pull/3975 Pull request] is showing as
not merging cleanly.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by AlexHill):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_better_patch: 1 => 0
Comment:
I've rebased this on top of master now that #24215 is in.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left some comments for improvement.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_better_patch: 1 => 0
Comment:
Updated to incorporate your suggestions.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
Comment:
This looks okay to me. Could an ORM expert give a +1?
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by akaariai):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
The patch needs rebase, and a little deduplication for the relational
field filter handling.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AlexHill):
* needs_better_patch: 1 => 0
Comment:
I've rebased and deduplicated that logic so it's shared by order with
respect to and the descriptors.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version:
(models, ORM) |
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"7bec480fe2ace94c8e7f0c88485442bfa74436b4" 7bec480f]:
{{{
#!CommitTicketReference repository=""
revision="7bec480fe2ace94c8e7f0c88485442bfa74436b4"
Fixed #24201 -- Added order_with_respect_to support to GenericForeignKey.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: AlexHill | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette <charette.s@…>):
In [changeset:"c0118ff80bd14fe0f65843e5df266ec5d922304d" c0118ff8]:
{{{
#!CommitTicketReference repository=""
revision="c0118ff80bd14fe0f65843e5df266ec5d922304d"
Refs #24201 -- Ignored order_with_respect_to private fields in migrations.
Thanks Tim for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Hill | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ashley Waite):
Replying to [comment:16 Simon Charette <charette.s@…>]:
> In [changeset:"c0118ff80bd14fe0f65843e5df266ec5d922304d" c0118ff8]:
> {{{
> #!CommitTicketReference repository=""
revision="c0118ff80bd14fe0f65843e5df266ec5d922304d"
> 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
[[https://github.com/django/django/blob/c0118ff80bd14fe0f65843e5df266ec5d922304d/django/db/migrations/state.py#L446-L448|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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Alex Hill | Owner: nobody
Type: New feature | Status: closed
Component: Database layer | Version:
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24201#comment:18>