ModelChoiceField to_field_name doesn't work if it's different from the model field's to_field

[László Károlyi]

Hey guys,

I have two models something along the following:

class M1(Model):
    code = CharField()
    name = CharField()

class M2(Model):
    m1 = ForeignKey(to=M1)

I have a custom modelform, something along:

class MyForm(ModelForm):
    m1 = ModelChoiceField(to_field_name='code')
    class Meta(object):
        model = M2
        exclude = ()

When I try to render this form with an instance of M2, because of to_field_name not being pk of m1, the form will render without the option pointing to M1 selected.

I managed to track the problem back to BoundField.value(), which calls self.field.prepare_value(data) with the PK passed to the instance of the ModelChoiceField. There, ModelChoiceField.prepare_value() should be able to know how to resolve the passed PK to the parameter in to_field_name, but it doesn't do it.

Because of that, I'm forced not to pass the to_field_name parameter, and let the select widget render itself with PKs instead of the wished code fields.

Please look into this, should be fairly easy to reproduce. As I could see from googling, ​this has formerly already been a problem, and now it's present again.

[László Károlyi]

Update: it seems that checking out the django project source and testing within can't reproduce this bug.

I'm trying to hunt down the exact source of it. Still, the problem is that BoundField gets an integer value instead of the looked up model in the foreign key.

[László Károlyi]

So I guess I just opened a huge can of worms. In a nutshell:

ModelForm ForeignKey data is initialized on ModelForms at __init__ time, and then, only the PKs are put into the self.initial dict. Hence, the time the BoundField wants to pass it over to ModelChoiceField.prepare_value(), it only has the PK.

There are two 'workarounds', neither of which is good looking, but fixes the problem:

1: Patch ModelChoiceField.prepare_value():

    def prepare_value(self, value):
        if hasattr(value, '_meta'):
            if self.to_field_name:
                return value.serializable_value(self.to_field_name)
            else:
                return value.pk
        if self.to_field_name:
            # Case for lookup when only a PK is passed
            selected_value = [x for x in self.queryset if x.pk == value]
            if selected_value:
                return getattr(selected_value[0], self.to_field_name)
        return super().prepare_value(value)

2: Patch BoundField.value():

    def value(self):
        """
        Return the value for this BoundField, using the initial value if
        the form is not bound or the data otherwise.
        """
        from django.forms.models import ModelChoiceField
        data = self.initial
        if self.form.is_bound:
            data = self.field.bound_data(self.data, data)
        elif hasattr(self.form, 'instance') and self.form.instance.pk and type(self.field) is ModelChoiceField:
            # First display: form non-bound but has an existing
            # instance, override any initials
            data = getattr(self.form.instance, self.name)
        return self.field.prepare_value(data)

I got a test, the whole test suite runs with both patches, posted in a pull request (the first version is commented out but it's there):
​https://github.com/django/django/pull/8650

This part of Django with the forms seems to be seriously not well thought out, to put it mildly.

Any suggestions are welcome, I spent ~8 hrs pondering on the solution, and traversing the Django source.

[Tim Graham]

You might look at #17657 which was a fix for to_field_name and ModelMultipleChoiceField. In that case, I think ModelMultipleChoiceField._check_values() is doing the conversion from initial pk values to to_field_name values. A proper fix most likely lies in ModelChoiceField rather than BoundField; the latter shouldn't have knowledge of specific fields.

[László Károlyi]

Replying to Tim Graham:

You might look at #17657 which was a fix for to_field_name and ModelMultipleChoiceField. In that case, I think ModelMultipleChoiceField._check_values() is doing the conversion from initial pk values to to_field_name values. A proper fix most likely lies in ModelChoiceField rather than BoundField; the latter shouldn't have knowledge of specific fields.

Just took a look at it, it's doing basically the same what I implemented in ModelChoiceField.value().

For me it really doesn't matter where the underlying logic is, as long as it works. Do you want me to uncomment the logic in ModelChoiceField and update the pull request?

[Tim Graham]

Yes, I'll have to examine that solution in more detail but I would say that's more on the right track.

The test can probably be more minimal as in 81963b37a92ef583b9f725f1a78dd2ca97c1ab95 -- check BoundField.value() rather than rendered HTML.

Please rebase your branch so that the unrelated commits don't appear and revert the unrelated blank line changes that I presume your IDE is making.

[László Károlyi]

Replying to Tim Graham:

Yes, I'll have to examine that solution in more detail but I would say that's more on the right track.

The test can probably be more minimal as in 81963b37a92ef583b9f725f1a78dd2ca97c1ab95 -- check BoundField.value() rather than rendered HTML.

Please rebase your branch so that the unrelated commits don't appear and revert the unrelated blank line changes that I presume your IDE is making.

OK, I'll get that done in the upcoming days. Are the new lines such a big problem? It's PEP8 enforced by Anaconda/ST3.

[Tim Graham]

We don't want unrelated whitespace changes in a patch.

[Tim Graham]

I updated ​the patch, however, I feel like the solution isn't ideal and there's still a case that doesn't work (see the failing test on the patch) -- if the form field's to_field_name is different from the model form's to_field.

[László Károlyi]

So the generic idea of the fix is to also fix the

ModelForm ForeignKey data is initialized on ModelForms at init time, and then, only the PKs are put into the self.initial dict.

part. After that, the value that will be passed to ModelChoiceField.prepare_value(). My opinion is still that this part is too complicated and should be rewritten at a later stage.

[George Khaburzaniya]

Original PR was closed. New ​PR

[Marc Gibbons]

Taking a stab at a fix for this. Looked at few different options – including ModelChoiceField and model_to_dict, all of which (including this one) feeling a bit hacky.

It really comes down to a problem of generating initial values to the ModelForm when an instance is provided and is used as the source of initial values. Getting the full context of the ForeignKey, its to_field, and whether or not the "initial" value came from the instance or from the initial kwargs of the constructor, isn't feasible from the ModelChoiceField class in isolation.

PR: ​https://github.com/django/django/pull/15025