Opened 5 months ago

Last modified 2 weeks ago

#28316 assigned Bug

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

Reported by: László Károlyi Owned by: László Károlyi
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Change History (11)

comment:1 Changed 5 months ago by 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.

comment:2 Changed 5 months ago by 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.

comment:3 Changed 5 months ago by Tim Graham

Summary: ModelChoiceField with to_field_name fails to render a widget properlyModelChoiceField with to_field_name doesn't select an option when editing
Triage Stage: UnreviewedAccepted

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.

comment:4 in reply to:  3 Changed 5 months ago by 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?

comment:5 Changed 5 months ago by 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.

comment:6 in reply to:  5 Changed 5 months ago by 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.

comment:7 Changed 5 months ago by Tim Graham

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

comment:8 Changed 5 months ago by Tim Graham

Has patch: set
Patch needs improvement: set
Summary: ModelChoiceField with to_field_name doesn't select an option when editingModelChoiceField to_field_name doesn't work if it's different from the model field's to_field

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.

comment:9 Changed 4 months ago by László Károlyi

Owner: changed from nobody to László Károlyi
Status: newassigned

comment:10 Changed 4 months ago by 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. Now, the value that will be passed to ModelChoiceField.prepare_value() is the one specified in to_field_name. My opinion is still that this part is too complicated and should be rewritten at a later stage.

Last edited 4 months ago by László Károlyi (previous) (diff)

comment:11 Changed 2 weeks ago by George Khaburzaniya

Original PR was closed. New PR

Last edited 2 weeks ago by George Khaburzaniya (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top