Opened 6 years ago

Last modified 5 months ago

#12203 new Bug

ManyToManyField with through model can't be used in admin

Reported by: dgouldin Owned by: nobody
Component: contrib.admin Version: master
Severity: Normal Keywords: M2M, admin, through, through_fields
Cc: dgouldin@…, glicerinu@…, cmawebsite@… 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)

12203-m2m-w-through-modeladmin-fields-option1.diff (2.4 KB) - added by ramiro 6 years ago.
patch implementing first admin validation strategy described by Russell
12203-m2m-w-through-modeladmin-fields-option1.2.diff (2.4 KB) - added by ramiro 6 years ago.
Updated patch for option 1 to take in account the fact that all m2m rels have through now. Thanks Alex Gaynor.
12203-m2m-w-through-modeladmin-fields-option2.diff (7.0 KB) - added by ramiro 6 years ago.
Patch implementing fancy admin validation strategy described by Russell.
12203-m2m-w-through-modeladmin-fields-option1.3.diff (1.6 KB) - added by vinaykrsharma 12 months ago.
do not report error if M2M field has through_fields defined

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by dgouldin

  • Cc dgouldin@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by ramiro

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."

comment:3 Changed 6 years ago by emulbreh

Possibly related: #9475

comment:4 Changed 6 years ago by russellm

  • Triage Stage changed from Unreviewed to 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 Changed 6 years ago by dgouldin

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 Changed 6 years ago by russellm

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.

Changed 6 years ago by ramiro

patch implementing first admin validation strategy described by Russell

Changed 6 years ago by ramiro

Updated patch for option 1 to take in account the fact that all m2m rels have through now. Thanks Alex Gaynor.

Changed 6 years ago by ramiro

Patch implementing fancy admin validation strategy described by Russell.

comment:7 Changed 6 years ago by ramiro

  • 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 Changed 6 years ago by russellm

(In [11737]) Refs #12203 -- Improved error handling for the case where a user manually specifies an m2m field with an explicit through field. Thanks to dgouldin for the report, and Ramiro Morales for the patch.

comment:9 Changed 6 years ago by russellm

  • 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 Changed 5 years ago by glic3rinu

  • Cc glicerinu@… added

comment:11 Changed 4 years ago by mattmcc

  • Severity set to Normal
  • Type set to New feature

comment:12 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Changed 12 months ago by vinaykrsharma

do not report error if M2M field has through_fields defined

comment:14 Changed 12 months ago by vinaykrsharma

  • Has patch set
  • Keywords M2M admin through through_fields added
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Resolution set to fixed
  • Status changed from new to closed
  • Type changed from New feature to Bug
  • Version changed from 1.1 to 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 Changed 12 months ago by timo

  • Resolution fixed deleted
  • Status changed from closed to new

A ticket isn't closed until the fix is committed. Also, your patch would need a test.

comment:16 Changed 12 months ago by timo

  • Needs documentation unset
  • Needs tests unset

I left comments for improvement on PR. Please uncheck "Patch needs improvement" when you update it, thanks.

Last edited 12 months ago by timo (previous) (diff)

comment:17 Changed 5 months ago by collinanderson

  • Cc cmawebsite@… added
  • Summary changed from ManyToManyField with explicit through model can't be used as an admin field to 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.

Note: See TracTickets for help on using tickets.
Back to Top