Opened 15 years ago
Last modified 2 months ago
#12203 assigned Bug
ManyToManyField with through model can't be used in admin
Reported by: | David Gouldin | Owned by: | Rosana Rufer |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | M2M, admin, through, through_fields |
Cc: | dgouldin@…, glicerinu@…, cmawebsite@…, frnhr | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Using the following models:
class Author(models.Model): name = models.CharField(max_length=100) class Book(models.Model): name = models.CharField(max_length=100) authors = models.ManyToManyField(Author, through='AuthorsBooks') class AuthorsBooks(models.Model): author = models.ForeignKey(Author) book = models.ForeignKey(Book)
... with this admin:
class BookAdmin(admin.ModelAdmin): fields = ['authors']
... results in a TemplateSyntaxError. Removing the through kwarg in Book.author field definition, the same admin class works.
Attachments (4)
Change History (29)
comment:1 by , 15 years ago
Cc: | added |
---|
comment:2 by , 15 years ago
comment:4 by , 15 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Ramiro has picked the problem correctly - this is a case of doing something you shouldn't. If you don't have a fields specifier on the ModelAdmin, you won't get an m2m widget because of the explicit m2m through model. Explicitly specifying one should be an error. However, the error handling for this case could certainly be improved.
The simple solution is to raise an ImproperlyConfigured error if an m2m with an explicit through model is specified in the fields list.
The fancy solution would be to loosen this condition slightly and allow an m2m field to be in the fields list (and therefore allow an m2m widget be used) *iff* the through model is just 2 foreign keys with a unique_together pairing (i.e., if the through model is exactly the same as the m2m model that is be automatically generated for the non-explicit through case).
Longer term, when a solution to #9475 is worked out, it should be possible loosen the condition even further, and allow the use of a the default m2m widget for cases where the m2m through model can be saved without any additional details.
comment:5 by , 15 years ago
I understand the difficulty the added complexity of extra m2m fields creates. In my case, I simply want to port a non-django db schema to django models, and I don't know any other way to create a m2m relation other than using through. That seemed to me the intent of #6095: to be able to specify your own model for m2m and use it just as you'd use a regular m2m model field. Unless you can point me in an alternate direction for legacy schema porting, I think it's a reasonable request to be able to use the simplest case m2m through model just as a regular django m2m.
comment:6 by , 15 years ago
The original intent of #6095 was to allow for data to be stored as part of the m2m relation - for example, in a 'Membership" relation between "Person" and "Group", you might want to store the level of membership. The ability to support legacy m2m tables was a bonus, since it's the redundant case of an m2m relation without any intermediate data.
I completely agree that this is a reasonable request - it's just not a trivial request. If I wasn't in favour of the request, I would have closed the ticket; instead, I marked it accepted. That doesn't mean I'm personally going to work on it any time soon, but if someone were to develop a solution, it would be a candidate for trunk.
For the benefit of anyone chasing this ticket - make sure you check all the edge cases. For example, the model described in the ticket actually isn't strictly analogous with Django's m2m, because it doesn't enforce uniqueness between author and book.
by , 15 years ago
Attachment: | 12203-m2m-w-through-modeladmin-fields-option1.diff added |
---|
patch implementing first admin validation strategy described by Russell
by , 15 years ago
Attachment: | 12203-m2m-w-through-modeladmin-fields-option1.2.diff added |
---|
Updated patch for option 1 to take in account the fact that all m2m rels have through now. Thanks Alex Gaynor.
by , 15 years ago
Attachment: | 12203-m2m-w-through-modeladmin-fields-option2.diff added |
---|
Patch implementing fancy admin validation strategy described by Russell.
comment:7 by , 15 years ago
Has patch: | set |
---|---|
Needs documentation: | set |
I've attached first iterations of patches implementing the two strategies outlined by Russell.
In the option2 one I've tried to cover all the corner cases. Something I'm not sure is the through-using, non-symmetrical, recursive m2m case. Can we allow the admin add/change forms to show the plain widget?.
Also, for both patches the exact text shown next to the ImproperlyConfigured
exception are open to be enhanced.
I will add documentation changes in (s) future interation(s).
See also tickets #10010 and #11126.
Replying to dgouldin:
to be able to specify your own model for m2m and use it just as you'd use a regular m2m model field. Unless you can point me in an alternate direction for legacy schema porting, I think it's a reasonable request to be able to use the simplest case m2m through model just as a regular django m2m.
If your simplest legacy intermediate table complies with the Django m2m restrictions/limitations: e.g. it has just one PK then one possible alternative could be to use the db_table
ManyToManyField field option (http://docs.djangoproject.com/en/dev/ref/models/fields/#django.db.models.ManyToManyField.db_table) and don't use a explicit through model.
comment:8 by , 15 years ago
comment:9 by , 15 years ago
Has patch: | unset |
---|---|
Needs documentation: | unset |
I've committed Ramiro's patch that improves error handling, but I'll leave the ticket open as a placeholder for the feature request of allowing m2m fields if the manually specified model is compatible with Django's requirements.
comment:10 by , 14 years ago
Cc: | added |
---|
comment:11 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
by , 10 years ago
Attachment: | 12203-m2m-w-through-modeladmin-fields-option1.3.diff added |
---|
do not report error if M2M field has through_fields defined
comment:14 by , 10 years ago
Has patch: | set |
---|---|
Keywords: | M2M admin through through_fields added |
Needs documentation: | set |
Needs tests: | set |
Patch needs improvement: | set |
Resolution: | → fixed |
Status: | new → closed |
Type: | New feature → Bug |
Version: | 1.1 → master |
Have fixed!
Do not add any error if M2M field has through_fields and through model is not auto_created!
Attachment: 12203-m2m-w-through-modeladmin-fields-option1.3.diff
comment:15 by , 10 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
A ticket isn't closed until the fix is committed. Also, your patch would need a test.
comment:16 by , 10 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
I left comments for improvement on PR. Please uncheck "Patch needs improvement" when you update it, thanks.
comment:17 by , 10 years ago
Cc: | added |
---|---|
Summary: | ManyToManyField with explicit through model can't be used as an admin field → ManyToManyField with through model can't be used in admin |
We need to fix #6707 first, otherwise it will revert to the default extra fields every time you hit save.
comment:19 by , 5 years ago
If you also waiting for the fix and have a simple through model for some reason you may use auto_created = True
to trick Django Admin:
class JobTitleExperienceThrough(models.Model): title = models.ForeignKey('JobTitle', on_delete=models.CASCADE, related_name='experiences_through') experience = models.ForeignKey('Experience', on_delete=models.CASCADE, related_name='titles_trough') class Meta: # TODO(dmu) MEDIUM: Remove `auto_created = True` after these issues are fixed: # https://code.djangoproject.com/ticket/12203 and # https://github.com/django/django/pull/10829 auto_created = True class JobTitle(models.Model): name = models.CharField(max_length=64) role = models.ForeignKey('JobRole', on_delete=models.CASCADE, related_name='titles') experiences = models.ManyToManyField('Experience', through='JobTitleExperienceThrough', related_name='titles', blank=True) class Meta: ordering = ('id',)
comment:20 by , 5 years ago
Unfortunately, the auto_created=True
hack doesn't work if you want to use foreign keys to the through
model. You'll get a (fields.E300) Field defines a relation with model 'JobTitleExperienceThrough', which is either not installed, or is abstract
.
comment:21 by , 5 years ago
Cc: | added |
---|
follow-up: 23 comment:22 by , 4 years ago
For some reason I ended up with this code:
class JobTitleAdmin(admin.ModelAdmin): def formfield_for_manytomany(self, *args, **kwargs): # pylint: disable=arguments-differ # TODO(dmu) MEDIUM: Remove `auto_created = True` after these issues are fixed: # https://code.djangoproject.com/ticket/12203 and # https://github.com/django/django/pull/10829 # We trick Django here to avoid `./manage.py makemigrations` produce unneeded migrations JobTitleExperienceThrough._meta.auto_created = True # pylint: disable=protected-access return super().formfield_for_manytomany(*args, **kwargs) class JobTitle(models.Model): name = models.CharField(max_length=64) role = models.ForeignKey('JobRole', on_delete=models.CASCADE, related_name='titles') experiences = models.ManyToManyField('Experience', through='JobTitleExperienceThrough', related_name='titles', blank=True) class Meta: ordering = ('id',)
comment:23 by , 4 years ago
Replying to Dmitry Mugtasimov:
The trick of setting JobTitleExperienceThrough._meta.auto_created = True
in formfield_for_manytomany
does indeed enable the ModelMultipleChoiceField
on the admin page without causing migration issues (as far as I can see).
However, there is a dangerous side-effect of using this, in case you have other models with a "cascading" relation directly to your explicit through-model, e.g. models.ForeignKey(to=JobTitleExperienceThrough, on_delete=models.CASCADE)
:
If the initial queryset for the m2m field JobTitle.experiences
is filtered (e.g. as in the docs https://docs.djangoproject.com/en/3.0/ref/contrib/admin/#django.contrib.admin.ModelAdmin.formfield_for_manytomany), and, for whatever reason, the filter excludes some Experience
objects that have previously been assigned, then the corresponding records from JobTitleExperienceThrough
will be silently deleted, including any records for other models pointing to them.
This means there is a serious potential for "silent" data loss.
This is because the many-to-many relation will now be updated using ManyRelatedManager.set()
(via ModelAdmin.save_related()
-> form.save_m2m()
-> BaseModelForm._save_m2m()
-> ManyToManyField.save_form_data()
).
Silent deletion could be prevented by setting on_delete=models.PROTECT
on any relation to the explicit through-model, but then you would first have to be aware of the necessity.
When using a "real" auto-created through
table (i.e. an implicit one) the issue does not arise, because there is no way to set a ForeignKey
to the implicit through
, as far as I know.
When using an inline for JobTitleExperienceThrough
, this issue does not occur.
comment:24 by , 5 months ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:25 by , 2 months ago
Similar to #9475, I think this "through" restriction is pretty artificial at this point (since #6707 and #9475) and the restriction can probably be removed.
Example PR here: https://github.com/django/django/pull/18594 (I don't plan on pushing this through myself but feel free to take it over.)
Everything should just work. Though, yes, if you filter the admin queryset and save, it's going to delete all of the existing rows that don't match your queryset. That's the behavior I would expect, though maybe I could see that being confusing if Inlines/FormSets don't delete instances that don't match your queryset? Is that the issue?
M2M docs say "For example, if an owner can own multiple cars and cars can belong to multiple owners – a many to many relationship – you could filter the Car foreign key field to only display the cars owned by the User"
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.formfield_for_manytomany
Inline docs refer to just ModelAdmin.get_queryset()
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#inlinemodeladmin-options
ModelAdmin.get_queryset
says: " One use case for overriding this method is to show objects owned by the logged-in user"
https://docs.djangoproject.com/en/dev/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_queryset
I don't know about the exact
TemplateSyntaxError
exception being thrown, but the fact that no widget can be used is sensible and documented behavior, with a documented alternative that involves inlines:citing:
"...when you specify an intermediary model using the through argument to a ManyToManyField, the admin will not display a widget by default. This is because each instance of that intermediary model requires more information than could be displayed in a single widget, and the layout required for multiple widgets will vary depending on the intermediate model."