Displaying (some) M2M fields with through models specified

[Alexander Hill]

Hi all,

Currently, only M2M fields without custom intermediary models can use the normal Django M2M form machinery. If you define your own intermediary models, you need to use inlines instead.

I would like to allow fields with custom through models to use the regular M2M field/widget automatically, if the through model meets the requirement that all its extra fields either have defaults or are nullable. I'll refer to these as "auto-compatible" models.

Making this work right now is as easy as adding "auto_created = True" to the intermediary model's Meta. If your model is not auto-compatible, you'll get a error when you try to add to the relation.

I would like to add official support for this. My suggestion would be:

1. Add a model Meta attribute that better describes these models, e.g. "simple_m2m". Auto-created intermediary models should have this set to True.

2. Wherever auto_created appears in a condition, decide for each whether or not auto-compatible models should also pass the test - if so, replaced with a check to "simple_m2m".

3. Add code to validate that user-created models with simple_m2m True actually do meet the above requirements.

As a note to step 2, if there is no test of auto_created that auto-compatible models shouldn't also pass, auto_created could simply be renamed. I doubt that though.

Thoughts?

Alex

[Russell Keith-Magee]

Hi Alex,

Short version - Broadly speaking, this is a feature we've discussed many times over many years, and we've accepted as a good idea in principle. It's logged as ticket #9475 [1]. There is some additional discussion on the original ticket that introduced m2m through models (#6095 [2]). 

The devil is in the details. Your analysis is largely correct, but there are a couple of details in your analysis that need tweaking.

On Mon, May 26, 2014 at 1:55 PM, Alexander Hill <al...@hill.net.au> wrote:

Hi all,

Currently, only M2M fields without custom intermediary models can use the normal Django M2M form machinery. If you define your own intermediary models, you need to use inlines instead.

I would like to allow fields with custom through models to use the regular M2M field/widget automatically, if the through model meets the requirement that all its extra fields either have defaults or are nullable. I'll refer to these as "auto-compatible" models.

It's not *just* about the admin interface, however. Admin only works because there's an underlying API that can be used; we need an API proposal to underpin the m2m widget used by admin. This was the sticking point that prevented this feature being added 7 years ago.

In your case, you're punting on the API case where extra m2m fields aren't optional, but the underlying issues are fundamentally the same.

 

Making this work right now is as easy as adding "auto_created = True" to the intermediary model's Meta. If your model is not auto-compatible, you'll get a error when you try to add to the relation.

If this works, it's accidental, and I'd be *very* surprised if the final fix is as simple as this. auto_created is involved with the process that determines whether a table is synchronised; so while it probably works if you add `auto_created = True` to a table that has already been synchronised, you'll probably find that it breaks if you do this on an unsynchronised model (with "can't find table" type errors).

 

I would like to add official support for this. My suggestion would be:

1. Add a model Meta attribute that better describes these models, e.g. "simple_m2m". Auto-created intermediary models should have this set to True.

This should be possible to auto detect - a manual flag shouldn't be required. There's enough data in the Options object on a model to determine if an m2m instance can be saved without any data beyond the PKs of the two related objects.

 

2. Wherever auto_created appears in a condition, decide for each whether or not auto-compatible models should also pass the test - if so, replaced with a check to "simple_m2m".

3. Add code to validate that user-created models with simple_m2m True actually do meet the above requirements.

If the flag can be auto detected, this bit isn't required.

 

As a note to step 2, if there is no test of auto_created that auto-compatible models shouldn't also pass, auto_created could simply be renamed. I doubt that though.

As do I :-)

 

Thoughts?

In summary - you're on the right path. There's a few edge cases that need to be addressed, and from a "greedy core developer" point of view, it would be nice to see the *whole* problem solved, not just the subset you've restricted yourself to. However, I'm sure we'll take whatever patch we can get :-)

 

[1] https://code.djangoproject.com/ticket/9475

[2] https://code.djangoproject.com/ticket/6095

Yours,

Russ Magee %-)

[Alex Hill]

Thanks Russ, I missed #9475 in my search. I'll have a read through those tickets...

...and back. I'm leaning towards keeping the API as-is, with add/create/remove simply unavailable or raising exceptions if the intermediate model doesn't meet the requirements.

A few reasons:

1. As the discussions show, the best form of this API is not obvious, which makes me think maybe there isn't a best form of this API.

2. create and add/remove would either have to have different ways of going about this or inherent corner cases, since create uses arbitrary keyword args, while add/remove use arbitrary positional args. How do you create a relationship to a new model which has a field called "extra"?

3. Although an inconsistency would remain in that only some m2m fields can use add/create/remove, I think this is a reasonable one. The difference between the presence or absence of at least one non-default field is quite easy to grasp and explain: either you need to provide extra data, or you don't. Having behaviour split along a clear line like that is acceptable IMO. The m2m relationships which can and can't be used with the default admin would match this.

4. Nothing is possible with this that isn't possible in roughly the same amount of code using the existing methods provided by related managers and intermediate model managers (albeit arguably less clearly).

All that said I'm not really against changing the API. I just don't see a compelling reason to do so, especially absent an API everyone can agree on.

For now, I'm keen to fix what I see as an unreasonable inconsistency: the fact that auto-compatible intermediate models can't do everything auto-created models can :)

If this works, it's accidental, and I'd be *very* surprised if the final fix is as simple as this. auto_created is involved with the process that determines whether a table is synchronised; so while it probably works if you add `auto_created = True` to a table that has already been synchronised, you'll probably find that it breaks if you do this on an unsynchronised model (with "can't find table" type errors).

You're 100% right - it "works" in the sense that once your model is set up correctly, you can delete all your custom admin form code and just add auto_created = True to your intermediate model's Meta. That's what I just did, and it felt great.

This should be possible to auto detect - a manual flag shouldn't be required. There's enough data in the Options object on a model to determine if an m2m instance can be saved without any data beyond the PKs of the two related objects.

I thought having the user specify their intention might be a good idea because:

- you can check at startup time whether or not things are going to work as they expect

- existing projects' behaviour won't change in any way

- these fields would end up being rendered with no way to modify the through model's extra fields, which might be a bit baffling. Then again, no more baffling than having them just not show up at all like they do now - either way it's a trip to Google.

These are all pretty minor so I guess in order to reduce churn they could just be ignored. I'm all for fewer pointless options; if you're OK with no flag then so am I.

In summary - you're on the right path. There's a few edge cases that need to be addressed, and from a "greedy core developer" point of view, it would be nice to see the *whole* problem solved, not just the subset you've restricted yourself to. However, I'm sure we'll take whatever patch we can get :-)

Thanks again for taking a look at this, Russ.

Alex