I don't like the idea of extended usage of field.get_internal_type(). The problem is that we haven't defined what the internal_type means, and it is actually used for different meanings in different places of code currently.
As an example, AutoFields have internal type as AutoField. The AutoField value is needed so that we can create correct auto-increment SQL for the schema. On the other hand, AutoField's internal type should also be an IntegerField (because other than the autoincrement part, AutoField is just like an IntegerField), and for example expressions do value casts automatically for all fields whose internal_type ends with IntegerField. So, now AutoField which should work exactly like an integer field for expressions doesn't work like an integer field for expressions. Similar problems arise also when trying to define get_internal_type() for relational fields. For example, on one hand ForeignKey's get_internal_type() should be the same as the related field's type (because the concrete field is of that type in the database), but on the other hand it should be 'ForeignKey', so that we know it is a relation. We should define an exact meaning for get_internal_type(), and use the method for only that. I haven't seen any explanation of the actual problems usage of field flags in django.db causes. We have perfectly good flags to use for checking if a field is a relational or a m2m relation, and we should use those when needed. - Anssi On Monday, February 2, 2015 at 10:49:11 AM UTC+2, Loïc Bistuer wrote: > > Thanks Markus for the detailed report. > > On a conceptual level I think we should aim for: > > - django.db.* only relies on get_internal_type(). > - django.* only relies on field flags. > > To address the immediate regressions I suggest we backport > https://github.com/django/django/pull/4002/files as far back as 1.7. That > would address #24236 without requiring > https://github.com/django/django/pull/3998/files. > > In master: > > - Replace all isinstance(field.rel) by isinstance(field) as a first step, > this will help normalize things for future refactors (i.e. composite / > virtual fields refactor, refactor of *Rel into proper fields like > ReverseForeignKey, etc.) > > - Replace all isistance(field) by get_iternal_type() in the ORM. > > - More tricky, replace all isinstance(field) in the rest of django by > cardinality flag when possible and identify every place where we can't do > it yet because we rely on the implementation of these fields. > > -- > Loïc > > > On Jan 31, 2015, at 18:19, Markus Holtermann <[email protected] > <javascript:>> wrote: > > > > Hey all, > > > > Since Django 1.8 (currently in alpha state), model fields gained > cardinality flags as part of the _meta refactoring. So, there is > one_to_one, one_to_many, many_to_one and many_to_many. These flags are > currently only used inside user-facing APIs such as forms and the admin. > > > > Furthermore model fields have a get_internal_type() method that returns > self.__class__.__name__ if they don't explicitly override that function (as > many of the built-in fields do). This method is heavily used inside the > backends. > > > > Besides those two ways to "try" to figure out what Django has to do in a > certain situation, the code uses e.g. isinstance(field.rel, ManyToManyRel) > and isinstance(field, ManyToManyField) to check for many-to-many > relationships. > > > > This is quite confusing. At least to me. In > https://github.com/django/django/commit/38c17871bb6dafd489367f6fe8bc56199223adb8 > > I committed a patch that uses field.many_to_many in order to figure out if > a certain column needs to be copied from one table to another (it doesn't > if it's an m2m relation) when altering a table on SQLite. > > > > However changing that to use get_internal_name() and keep existing code > working is not trivial since neither ForeignKey nor ManyToManyField or > OneToOneField have those methods defined. Thus fields inheriting from > either of them need to explicitly define the method to return "ForeignKey", > "ManyToManyField" or "OneToOneField". (The backport of that patch to 1.7 > unfortunately breaks existing projects). > > > > I have a pull request open to fix the issue on 1.7 related to m2m > fields: https://github.com/django/django/pull/3998 . > > > > However, I don't really like that repetitive pattern of checking for > inheritance and get_internal_type(). I'm thinking about keeping the pattern > in 1.8 (and replacing the above checks with the one in the pull request) > and accept https://github.com/django/django/pull/4002 for 1.9. Thus all > projects and apps that rely on the class name of a related fields need to > update their code and explicitly return the class name. > > > > Thoughts and input highly welcome. > > > > /Markus > > > > > > -- > > You received this message because you are subscribed to the Google > Groups "Django developers (Contributions to Django itself)" group. > > To unsubscribe from this group and stop receiving emails from it, send > an email to [email protected] <javascript:>. > > To post to this group, send email to [email protected] > <javascript:>. > > Visit this group at http://groups.google.com/group/django-developers. > > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/b66842ee-62d6-43f2-a3df-838020cf60a2%40googlegroups.com. > > > > For more options, visit https://groups.google.com/d/optout. > > -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/348b5c83-62f2-4729-b2d4-247bbb9e9fce%40googlegroups.com. For more options, visit https://groups.google.com/d/optout.
