Extending ModelForm.save_m2m

[James Pic]

Hi all,

Many apps provide new related managers to extend your django models with. For example, django-tagulous provides a TagField which abstracts an M2M relation with the Tag model, django-gm2m provides a GM2MField which abstracts an relation, django-taggit provides a TaggableManager which abstracts a relation too, django-generic-m2m provides RelatedObjectsDescriptor which abstracts a relation again.

While that works pretty well, it gets a bit complicated when it comes to encapsulating the business logic for saving such data in a form object. This is two-part problem:

- getting initial data,

- saving relations.

Currently in Django, getting initial data is done in model_to_dict, which call's the model field's value_from_object() method, bypassing form fields completely, I suggest that we add ability to do that in the Form field too.

As for saving relations, this is currently done in ModelForm.save_m2m(), which calls the model field's save_form_data() directly, for each field that's many to many or virtual, completely ignoring the form field.

This means that if one wants to customize how a form field works in terms of getting the initial data for its widget and saving relations, then they should override the model field, instead of just the form field. What could go wrong with that ? :)

Do you think it would be OK to decouple that a bit and allow the form field to have value_from_object() and save_form_data() ?

Best

James

[James Pic]

Hi all,

My understanding of the design issue I'm facing was too approximative
at the time I opened this topic, sorry about that.

The way I understand it now, is that we have only 2 moving parts but 3 features:

- input validation in the form field,
- business logic in the model field,
- problem: validated input to business logic is in the model field.

It seems like decoupling the third feature from the model field into
its own component would be the best way to achieve loose coupling and
make it replaceable.

This would enable a user to replace the input-business behaviour
(value_from_object(), save_form_data()). This Having such an
input-business in a model field **or a modelform field** would allow
changing it on a per-form basis while keeping it in the right place.

Examples would be:

class YourModel(models.Model):
test = GenericForeignKey('content_type', 'object_id',
form_business=YourFormBusiness)

Alternatively:

class YourModelForm(models.Model):
test = YourField(form_business=YourFormBusiness)

Advantages:

- Model fields still have default save_form_data/value_from_object but
it's decoupled in another class,
- Form field still don't do any business logic, they may just provide
an alternative one,
- Allows providing form fields for model fields which are not editable
by default,
- Allows encapsulating form field specific business logic in its own class,

What do you think ?

Best

[Tim Graham]

Your description is a bit abstract for me to understand. Could you put together a before (how the code looks now) and after (how the code would look with this feature) for an example use case?

[James Pic]

Thanks for the advice, I started trying moving form-related stuff from
FieldBase into a new ModelFormFieldBase class, and it turns out that
there's a lot more we could, or should, move than just
value_from_object and save_form_data.

Why not move out everything that couples the form field and the model
field in from FieldBase into ModelFormFieldBase while we're at it ?

I like this idea, are you more interrested in seeing a before / after
of 0. just moving value_from_object + save_form_data or 1. moving form
stuff from FieldBase into ModelFormFieldBase ?

[Tim Graham]

I'm trying to understand what you're trying to accomplish at a very basic level. I'm not familiar with any of the third-party packages you mentioned. How will the proposed changes in Django simplify them? In your second mail you proposed a "form_business" kwarg with a "YourFormBusiness" argument without a description of what that class looks like. I'm sure I could understand more if I spent a lot of time looking into it, but please try to make it very easy -- you'll get more feedback this way.

p.s. not sure you meant by "FieldBase" -- there is no class in Django by that name.

[James Pic]

Sure, my bad, this is the example with form_business, a GFK, before /
after: https://gist.github.com/jpic/c6a16723db62f779848f

If we make it to encapsulate value_from_object / save_form_data
elsewhere than in the model field then it will allow form fields and
forms used in the admin to do more. One might raise the point that
there should only be one way for a form to get / provide data for a
model field. While that makes sense, perhaps a pragmatic way to change
it would be useful.

Note that currently, I've tested the first idea (extension to
modelform api) and maybe it's not that bad
https://github.com/yourlabs/django-autocomplete-light/blob/v3/src/dal/forms.py#L60

Perhaps another way (deviated from the extension to modelform api)
would be to have forms.models.ModelField classes, which would add the
retrieve / provide data for a models.fields.Field and have precedence
over the corresponding
models.fields.Field.{save_form_data,value_for_object}:
https://gist.github.com/jpic/c30724a5a90b258a312a

Please let me know if there's anything else I can do to help getting feedback

Thanks for your feedback