Code

Opened 4 years ago

Last modified 2 years ago

#12203 new New feature

ManyToManyField with explicit through model can't be used as an admin field

Reported by: dgouldin Owned by: nobody
Component: contrib.admin Version: 1.1
Severity: Normal Keywords:
Cc: dgouldin@…, glicerinu@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
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 (3)

12203-m2m-w-through-modeladmin-fields-option1.diff (2.4 KB) - added by ramiro 4 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 4 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 4 years ago.
Patch implementing fancy admin validation strategy described by Russell.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 4 years ago by dgouldin

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

comment:2 Changed 4 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 4 years ago by emulbreh

Possibly related: #9475

comment:4 Changed 4 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 4 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 4 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 4 years ago by ramiro

patch implementing first admin validation strategy described by Russell

Changed 4 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 4 years ago by ramiro

Patch implementing fancy admin validation strategy described by Russell.

comment:7 Changed 4 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 4 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 4 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 4 years ago by glic3rinu

  • Cc glicerinu@… added

comment:11 Changed 3 years ago by mattmcc

  • Severity set to Normal
  • Type set to New feature

comment:12 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.