[Django] #24317: Deprecate field.rel, replace it with real field instances
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database | Version: master
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
The problem is that when using get_fields(), you'll get either a field.rel
instance (for reverse side of user defined fields), or a real field
instance (for example ForeignKey). These behave differently, so that the
user must always remember which one he is dealing with. This creates lots
of non-necessary conditioning in multiple places of Django.
For example, the select_related descent has one branch for descending
foreign keys and one to one fields, and another branch for descending to
reverse one to one fields. Conceptually both one to one and reverse one to
one fields are very similar, so this complication is non-necessary.
The idea is to deprecate field.rel, and instead add field.remote_field.
The remote_field is just a field subclass, just like everything else in
Django.
The benefits are:
- Conceptual simplicity - dealing with fields and rels is non-necessary,
and confusing. Everything from get_fields() should be a field.
- Code simplicity - no special casing based on if a given relation is
described by a rel or not
- Code reuse - ReverseManyToManyField is in most regard exactly like
ManyToManyField
The expected problems are mostly from 3rd party code. Users of _meta that
already work on expectation of getting rel instances will likely need
updating. Those users who subclass Django's fields (or duck-type Django's
fields) will need updating. Examples of such projects include django-rest-
framework and django-taggit.
Very work-in-progress code available from
https://github.com/akaariai/django/tree/virtual_rel_field
--
Ticket URL: <https://code.djangoproject.com/ticket/24317>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
My current thoughts of how relation fields should work is:
A relation in Django consits of:
- The field itself
- A descriptor to access the objects of the relation
- The descriptor might need a custom manager
- Possibly a remote field (the field to travel the relation in other
direction)
- The remote field can contain a descriptor and a manager
- For deprecation period, field.rel is something like the remote field,
but without
actually being a field instance.
The loading order is as follows:
- The relation field is created as part of loading the class (or
completely separately
by migrations for example).
- The relation field is added to the class (contribute_to_class is
called). The field is
added to model's _meta.
- When both the origin and the target classes are present, the
remote_field is created
and the descriptors are added. The remote field is added to the target
class' _meta (note:
the target class' _meta wasn't touched before the remote fields
refactor!)
- For migrations it is possible that a model is replaced live in the
app-cache. For example,
assume model Author is changed, and it is thus reloaded. Model Book
has foreign key to
Author, so its reverse field must be recreated in the Author model,
too. The way this is
done is that we collect all fields that have been auto-created as
relationships into the
Author model, and recreate the related field once Author has been
reloaded.
I also thought of making field.rel actually a field subclass. But I'm not
too enthusiastic of doing this, this ties the new implementations to the
old Rel API. It seems better to write the new remote fields from scratch,
and then make them as much as possible Rel like for deprecation period
(the problem being that field.rel instances are what you get from
_meta.get_field[s]() calls).
Note that even if this ticket is accepted, this work is still in early
stages, and it isn't that clear how we actually want to do this. Maybe a
DEP is in order for this? However, it is clear to me that we want to do
*some* cleanup. It isn't nice at all that you can get instances that
implement different APIs from get_field() and friends.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by collinanderson):
This should fix #897.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: cmawebsite@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
I've been working on this, with an approach where I completely deprecate
field.rel with new reverse field instances.
Unfortunately this seems to be an dead end for multiple reasons:
- Patch implementing this would be huge: I'm currently at around 1700
lines changed, and I still have probably at least 500 lines to change.
These changes aren't systematic or easy to verify.
- There is a huge risk of regressions, because of the size of the patch,
and because it is nearly impossible to verify changes in the related
fields code.
- 3rd party software likely relies a lot on field.rel introspection.
So, instead of trying to introduce a new field instance I think a better
approach is to do incremental changes to the code. The idea is to
continuously keep the field.rel as backwards compatible as possible while
making it more like a relation field. The end result should be a situation
where we can make field.rel a real field subclass.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
Every time I start working on any larger refactor I end up breaking
Django's own code so badly I have to restart my work.
So, in short, the only sensible way forward is to do minor changes to the
related fields API over time, and use deprecations where possible.
The main goals are:
- Clean up the main API, that is the API defined by ForeignObject
- Make the "Rel" instances respect the same API
- Finally make "Rel" instances to be full Field instances (including
registration to models)
The first step is to investigate all the "related_fields,
foreign_related_fields, ..." attributes. It might suffice we have just one
attribute: data_fields. To get what related_fields currently is, use
zip(self.data_fields, self.remote_field.data_fields). The rest of the
attributes can be constructed from the zipped version, self.data_fields or
self.remote_field.data_fields.
Any takers for this clean-up? If not, then I'll tackle it myself when time
permits.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner: nobody
Type: | Status: new
Cleanup/optimization |
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by auvipy):
Replying to [comment:6 akaariai]:
> Every time I start working on any larger refactor I end up breaking
Django's own code so badly I have to restart my work.
>
> So, in short, the only sensible way forward is to do minor changes to
the related fields API over time, and use deprecations where possible.
>
> The main goals are:
> - Clean up the main API, that is the API defined by ForeignObject
> - Make the "Rel" instances respect the same API
> - Finally make "Rel" instances to be full Field instances (including
registration to models)
>
> The first step is to investigate all the "related_fields,
foreign_related_fields, ..." attributes. It might suffice we have just one
attribute: data_fields. To get what related_fields currently is, use
zip(self.data_fields, self.remote_field.data_fields). The rest of the
attributes can be constructed from the zipped version, self.data_fields or
self.remote_field.data_fields.
>
> Any takers for this clean-up? If not, then I'll tackle it myself when
time permits.
have you started working on it? if not, then to help you to start clean up
the API what is you suggestion to start work from? I have some familiarity
with django internal code bases.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => auvipy
Comment:
I am willing to help in clean up mission with a hope that ansi will guide
me and review my small commits which will be WIP mode. I saw yours WIP
branch for virtual fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:8>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
The advice I can give is that you should:
1. Find places where rield.remote_field responds to different API than
Field.
2. Fix these one at a time while trying to have backwards compat, even
if the API isn't public.
In addition, simplifications to the APIs are welcome, as is a high level
documentation of how related fields actually work.
We need to try to keep backwards compat as many projects are forced to use
the private APIs.
But most of all, do small incremental changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:9>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by auvipy):
well should a dep regarding this should be formed?
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:10>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by auvipy):
Replying to [comment:9 akaariai]:
> The advice I can give is that you should:
> 1. Find places where rield.remote_field responds to different API than
Field.
> 2. Fix these one at a time while trying to have backwards compat, even
if the API isn't public.
>
> In addition, simplifications to the APIs are welcome, as is a high level
documentation of how related fields actually work.
>
> We need to try to keep backwards compat as many projects are forced to
use the private APIs.
>
> But most of all, do small incremental changes.
is this PR related to this ticket?
https://github.com/django/django/pull/4241/files
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:11>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by guettli):
I hope I am not coming to late here.
Example: Model `Detail` has a ForeignKey to `Master`.
If I call get_fields() on Master. I want to see class names which
everyone immediately understands.
Up to now you get something like "ManyToOneRel to Detail". The meaning if
it is correct, but not immediately clear.
What about "ReverseForeignKey"?
Related: http://dba.stackexchange.com/questions/126116/name-of-foreignkey-
back-relation/126157#126157
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:12>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by srkunze):
@guettli
Even shorter: {{{BackReference}}}?
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:13>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by timgraham):
I believe the idea is that if you access a reverse foreign key, you'll
simply get the foreign key field of the other model.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:14>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: mmitar@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: akaariai | Owner:
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: auvipy =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:16>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Asif Saifuddin Auvi):
should the virtual field #16508 need to be addressed first? My analysis
seems that should be first
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:16>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Gabriel Canto de Souza):
I also had a huge difficulty sorting out the fields of ._meta.get_fields()
, what I was trying to achieve was get ONLY the reverse fields (because in
our context every relationship is bidirectional, and everything is
generic/auto generated)
This gets really complicated with m2m that contain through tables, because
not only you get the reverse m2m remote_field, but also the m2o
relationship with the through table, so no only you have to figure out
these are reverse, but group both the m2m and m2o that revert to the same
relationship.
This is the code I managed to come up with to get only the reverse rels:
{{{#!python
#I'm sure there's a better way of doing this
#This is the documented way of getting reverse fields, but it also gets
ManyToOne rel with the through table of forward m2m rels, so we have to
filter them
reverse_fields = [f for f in opts.get_fields() if f.auto_created and not
f.concrete]
reverse_m2m = [f for f in reverse_fields if f.__class__.__name__ ==
'ManyToManyRel' and f.on_delete == None and not
getattr(model,f.field.name,False)]
#Also filtering the ManyToOne rel with the through table of the reverse
m2m rels, because we need to differentiate them from reverse ManyToOne
with other models(not through)
reverse_m2m_through = [f for f in reverse_fields if f.__class__.__name__
== 'ManyToOneRel' and f.related_model in [f.through for f in reverse_m2m
if getattr(f,'through',False)]]
forward_m2m = [f.remote_field for f in opts.get_fields() if not
f.auto_created and f.many_to_many]
#Filtering the ManyToOne rels with the through table of forwards m2m
rels, same logic as above
forward_m2m_through = [f for f in reverse_fields if f.__class__.__name__
== 'ManyToOneRel' and f.related_model in [f.through for f in forward_m2m
if getattr(f,'through',False)]]
reverse_fields_final = itertools.chain(reverse_m2m, [f for f in
reverse_fields if f not in itertools.chain(reverse_m2m,
reverse_m2m_through, forward_m2m, forward_m2m_through)])
}}}
I propose to add a new attribute ''is_reverse = True/False'' to facilitate
this, of course this solution works in my context point of view, there
might be a better approach to this
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:17>
Django
-------------------------------------+-------------------------------------
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Asif Saifuddin Auvi):
Replying to [comment:17 Gabriel Canto de Souza]:
I am preparing a Draft DEP for addressing ORM relations API related issues
here https://github.com/django/deps/pull/39
It's still work in progress but you certainly add your thoughts/feedbacks.
--
Ticket URL: <https://code.djangoproject.com/ticket/24317#comment:18>