Usage of field cardinality flags in database schema backends

[Markus Holtermann]

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

[soka...@yandex.ru]

Yes, it seems reasonable, because schema should deal with internal type and not with

field flags such as field.many_to_many.

So, the summary is:

For 1.8:

https://github.com/django/django/pull/4014

Then

https://github.com/django/django/pull/3998 (for 1.7 and 1.8 and current master)

For 1.9:

https://github.com/django/django/pull/4002

And remove isinstance(field, ManyToManyField) checks in the database schema.

(maybe it will be included to 4002)

суббота, 31 января 2015 г., 13:19:37 UTC+2 пользователь Markus Holtermann написал:

[Loïc Bistuer]

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

> --
> 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 django-develop...@googlegroups.com.
> To post to this group, send email to django-d...@googlegroups.com.
> 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.

[Anssi Kääriäinen]

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

[Tim Graham]

Seems to me it would be useful to work with some external projects (like taggit) to convert their projects to use only public APIs -- otherwise we are stumbling around in the dark trying to figure out theoretical use cases.