Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#10349 closed (fixed)

ManyToMany field does not take dynamic initial values into account

Reported by: fas Owned by: fas
Component: Database layer (models, ORM) Version: 1.0
Severity: Keywords: ManyToMany, initial, dynamic, lambda
Cc: mbencun@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

For dbfields it is possible to provide the initial value in form of a anonymous function, e.g. models.CharField(..., initial=lambda: "I am dynamic!").
The formfield method of the ManyToMany field does not take this into account:

defaults['initial'] = [i._get_pk_val() for i in defaults['initial']]

The fix is simple and provided in a patch, diffed against release 1.0.2.

Attachments (1)

manytomany_dynamic_initial_against_1.0.2.diff (777 bytes) - added by fas 6 years ago.
simple fix

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by fas

simple fix

comment:1 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:2 Changed 6 years ago by jacob

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

In the future, please provide patches against trunk if possible. We backport patches from trunk to the release branches.

comment:3 Changed 6 years ago by russellm

  • Needs tests set

It would also be helpful if you provided a testcase as part of your patch - or, at the very least, an example of the usage that is corrected by applying the patch. It isn't immediately obvious to me what problem this patch is trying to solve.

comment:4 Changed 6 years ago by fas

  • Cc mbencun@… added

When using Field.formfield you can pass a lambda function as the initial value which will be passed over to the form field. When rendering the form field, the initial value will be evaluated in case it is a function. This is done in forms/forms.py, method as_widget.

The ManyToManyField however overwrites the formfield method in order to set its initial value to be a list of IDs. It assumes that the given initial value is a list of related objects. It also assumes that the form class to be used is forms.ModelMultipleChoiceField or that the form field to be used takes a list of IDs. Both assumptions are incorrect.

    def formfield(self, **kwargs):
        defaults = {'form_class': forms.ModelMultipleChoiceField, 'queryset': self.rel.to._default_manager.complex_filter(self.rel.limit_choices_to)}
        defaults.update(kwargs)
        # If initial is passed in, it's a list of related objects, but the                                                                                                          
        # MultipleChoiceField takes a list of IDs.                                                                                                                                  
        if defaults.get('initial') is not None:
            defaults['initial'] = [i._get_pk_val() for i in defaults['initial']]
        return super(ManyToManyField, self).formfield(**defaults)

This makes it impossible to pass a function for the initial value and also makes it impossible to create a custom form field that needs the related object and not only its IDs. I revoke my initial patch and propose that we should find a solution that works without the two assumptions made above. I think ModelChoiceField (forms/models.py) might the the right place. Applying the same transformation (object->ID) there before passing the initial to the super (ChoiceField) should solve the problem.

Any suggestions?

comment:5 Changed 6 years ago by russellm

My suggestion is that you provide a test case, like I asked you to.

I completely understand what the code is trying to do. I can see the analog with what happens with normal form fields. On the face of it, the patch looks fine. However, I can't come up with a single code example that would actually exercise the patch you have provided.

If you provide a callable to the initial argument when instantiating a form, the callable is evaluated. If you define a form field with a callable initial value, it is evaluated.

Don't just explain your patch with words - you need to show me an example of code where you can provide a callable for an m2m field that doesn't work with the current Django trunk, but does work when you apply your patch. Ideally, that example would be integrated into the existing Django test suite, but at this point, I'd take any example, standalone or otherwise.

comment:6 Changed 6 years ago by fas

Ok, here is an example. We have a many-to-many relation between articles and publications.

from django.db import models

class Publication(models.Model):
    title = models.CharField(max_length=30)
    date = models.DateField()

class Article(models.Model):
    headline = models.CharField(max_length=100)
    publications = models.ManyToManyField(Publication)

Then we create a model form that has a callback for the form field. Such a callback is used in the admin.
In the following example, the initial value of 'publications' of the article form should be the ten latest publications.

from django.forms.models import modelform_factory

def formfield_for_dbfield(db_field, **kwargs):
    if db_field.name == 'publications':
        kwargs['initial'] = lambda: Publication.objects.all().order_by('-date')[:10]
    return db_field.formfield(**kwargs)

modelform_factory(Article, formfield_callback=formfield_for_dbfield)

A lambda expression is used so that the expression will not be evaluated once but every time, preventing the initial selection from getting outdated (same reason you would use date.today instead of date.today() as an initial value for a date field).

comment:7 Changed 6 years ago by russellm

  • Resolution set to fixed
  • Status changed from new to closed

(In [10652]) Fixed #10349 -- Modified ManyToManyFields to allow initial form values to be callables. Thanks to fas for the report and patch.

comment:8 Changed 6 years ago by russellm

(In [10653]) [1.0.X] Fixed #10349 -- Modified ManyToManyFields to allow initial form values to be callables. Thanks to fas for the report and patch.

Merge of r10652 from trunk.

comment:9 Changed 4 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 deleted

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