Opened 5 years ago

Closed 5 months ago

Last modified 5 months ago

#17657 closed Bug (fixed)

ModelForm does not respect ModelMultipleChoiceField's to_field_name attribute

Reported by: Andrey Fedoseev Owned by: nobody
Component: Forms Version: 1.3
Severity: Normal Keywords:
Cc: marc.tamlyn@…, bmispelon@…, valtron2000@…, nav@…, andrey.fedoseev@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When extracting data from existing model instance ModelForm ignores to_field_name attribute of ModelMultipleChoiceField fields and always extracts values as a list of primary keys (see https://code.djangoproject.com/browser/django/trunk/django/forms/models.py?rev=17434#L127)

One of the problems caused by this behaviour is that options in multiple select widget are not selected. Here's a small test case to illustrate this:

# models.py

from django.db import models


class Foo(models.Model):

    slug = models.CharField(max_length=40, unique=True)
    title = models.CharField(max_length=40, unique=True)


class Bar(models.Model):

    foos = models.ManyToManyField(Foo)

# tests.py

from django.test.testcases import TestCase
from django import forms
from models import Foo, Bar


class TestModelMultipleChoiceWithFieldName(TestCase):

    def setUp(self):
        self.instance = Bar.objects.create()

        self.instance.foos.add(Foo.objects.create(title="Spam", slug="spam"))
        self.instance.foos.add(Foo.objects.create(title="Ham", slug="ham"))
        self.instance.foos.add(Foo.objects.create(title="Eggs", slug="eggs"))


    def test_without_field_name(self):

        class Form(forms.ModelForm):

            foos = forms.ModelMultipleChoiceField(Foo.objects.all())

            class Meta:
                model = Bar

        form = Form(instance=self.instance)

        self.assertEquals(
            str(form["foos"]),
            '<select multiple="multiple" name="foos" id="id_foos">\n'
            '<option value="1" selected="selected">Spam</option>\n'
            '<option value="2" selected="selected">Ham</option>\n'
            '<option value="3" selected="selected">Eggs</option>\n'
            '</select>'
        )

    def test_with_field_name(self):

        class Form(forms.ModelForm):

            foos = forms.ModelMultipleChoiceField(Foo.objects.all(), to_field_name="slug")

            class Meta:
                model = Bar

        form = Form(instance=self.instance)

        # Fails! Options aren't selected.
        self.assertEquals(
            str(form["foos"]),
            '<select multiple="multiple" name="foos" id="id_foos">\n'
            '<option value="spam" selected="selected">Spam</option>\n'
            '<option value="ham" selected="selected">Ham</option>\n'
            '<option value="eggs" selected="selected">Eggs</option>\n'
            '</select>'
        )

I am preparing a patch now.

Attachments (1)

issue17657.diff (3.3 KB) - added by Andrey Fedoseev 5 years ago.

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by Andrey Fedoseev

Attachment: issue17657.diff added

comment:1 Changed 5 years ago by Andrey Fedoseev

Has patch: set

comment:2 Changed 5 years ago by Aymeric Augustin

Triage Stage: UnreviewedDesign decision needed

I'm not sure what to do with this ticket.

On one hand, to_field_name is an internal API. I'd like to know if it's possible to trigger a bug by using only public APIs. On the other hand, the patch appears to be an improvement.

This needs more thought.

Last edited 5 years ago by Aymeric Augustin (previous) (diff)

comment:3 Changed 4 years ago by Marc Tamlyn

Cc: marc.tamlyn@… added

I came across this today - I would like the patch to be applied. I know I'm messing with an internal API but it'd be good if it 'just worked'.

comment:4 Changed 4 years ago by Paul Oswald

I think there is a pretty strong case to be made that this kind of functionality should be documented and added to Django's public API. Even without going that far, it certainly seems more clean to not make the assumption that the pk is the way field names are looked up, if an attribute is being used elsewhere in the code.

comment:5 Changed 4 years ago by Jacob

Triage Stage: Design decision neededAccepted

The patch is an improvement, so internal API or not I think it should go in.

comment:6 Changed 4 years ago by Baptiste Mispelon

Cc: bmispelon@… added
Patch needs improvement: set

This was reported again recently with a slightly different perspective (the goal was to hide the pk from the end-user): #20202

In that ticket, I came to the conclusion that it was a documentation issue and proposed the following patch: https://github.com/bmispelon/django/compare/ticket-20202

The existing patch does not apply cleanly anymore because of commit e44ab5bb4fd3aa826ca4243a8ea9fd7125800da2 which was done as an optimization.

The optimization is unfortunately not compatible with the OP's proposed fix.

From my understanding, model_to_dict needs access to the form's field's to_field_name which is a problem because the current implementation is only passed a model instance and two lists of field names (one to include and one to exclude).

It's possible to hack something that makes the OP's test pass but it's quite ugly: https://gist.github.com/bmispelon/5329758

comment:7 Changed 4 years ago by valtron2000@…

Cc: valtron2000@… added

comment:8 Changed 3 years ago by nav@…

Cc: nav@… added

comment:9 in reply to:  6 Changed 2 years ago by Thomas Chaumeny

Replying to bmispelon:

From my understanding, model_to_dict needs access to the form's field's to_field_name which is a problem because the current implementation is only passed a model instance and two lists of field names (one to include and one to exclude).

I just run on that bug on production (Django 1.6) with a ModelChoiceField and after looking at the code I came to the same conclusion. It would be nice to do something about it.

comment:10 Changed 8 months ago by Tim Graham

#26405 is a duplicate.

comment:11 Changed 8 months ago by drepo

(Since #26405 is referenced here) I'd say to_field_name should not be an internal API, it is a very natural use case for a developer building on Django. Think of an input[type=text] with an autocomplete as you type feature. Not uncommon and very user-friendly when the selection list is more than 20-30 items (thousands in our case and not unheard of in most enterprise(y) use cases).

In such a scenario one would have to maintain a dual state - store the id of the foreign key somewhere (possibly input[type=hidden]) and display the text in the original. Also significant client side script would be needed to synchronize changes between the two fields. Furthermore, the developer experience of rendering the form may not be as simple as {{ form }}.

The problem gets more complicated in case of M2M relations, where more state has to be managed on client side. More code -> more bugs and reduced productivity.

One could argue that [type=hidden] may be eliminated by using transformations at the View level, but it is not a DRY approach and does not give the same automation as most of the forms framework does. And I don't like "injecting" random code snippets in an otherwise pretty elegant and nearly declarative Django forms.

comment:12 Changed 5 months ago by Andrey Fedoseev

Cc: andrey.fedoseev@… added

Unfortunately, the proposed patch doesn't work anymore. The original idea was to have model_dict to return "raw" values provided by f.value_from_object() and let the form fields to the rest. Now it's impossible because ManyToManyField.value_from_object was changed so that it always returns a list of pks instead of a QuerySet. See https://github.com/django/django/commit/67d984413c9540074e4fe6aa033081a35cf192bc

I've created a new pull request GitHub with the following changes:

  1. Revert ManyToManyField.value_from_obj back to return a QuerySet instead of a list of pks
  2. Remove the obsolete tests for model_to_dict performance optimization.
  3. Add tests to ensure that to_field_name is handled correctly.

I don't know why the special case with pks existed in model_to_dict in the first place.

Last edited 5 months ago by Andrey Fedoseev (previous) (diff)

comment:13 Changed 5 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 81963b37:

Fixed #17657 -- Made ModelForm respect ModelMultipleChoiceField's to_field_name.

Follow up to 67d984413c9540074e4fe6aa033081a35cf192bc.

comment:14 Changed 5 months ago by Tim Graham <timograham@…>

In ded5020:

[1.10.x] Fixed #17657 -- Made ModelForm respect ModelMultipleChoiceField's to_field_name.

Follow up to 67d984413c9540074e4fe6aa033081a35cf192bc.

Backport of 81963b37a92ef583b9f725f1a78dd2ca97c1ab95 from master

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