Opened 7 years ago

Last modified 5 months 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: Marc Gibbons
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Dmytro Litvinov 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 (15)

comment:1 by László Károlyi, 7 years ago

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 by László Károlyi, 7 years ago

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 by Tim Graham, 7 years ago

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.

in reply to:  3 comment:4 by László Károlyi, 7 years ago

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 by Tim Graham, 7 years ago

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.

in reply to:  5 comment:6 by László Károlyi, 7 years ago

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 by Tim Graham, 7 years ago

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

comment:8 by Tim Graham, 7 years ago

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 by László Károlyi, 7 years ago

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

comment:10 by László Károlyi, 7 years ago

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 7 years ago by László Károlyi (previous) (diff)

comment:11 by George Khaburzaniya, 6 years ago

Original PR was closed. New PR

Last edited 6 years ago by George Khaburzaniya (previous) (diff)

comment:12 by Marc Gibbons, 2 years ago

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

comment:13 by Mariusz Felisiak, 2 years ago

Owner: changed from László Károlyi to Marc Gibbons
Patch needs improvement: unset

comment:14 by Carlton Gibson, 2 years ago

Patch needs improvement: set

comment:15 by Dmytro Litvinov, 5 months ago

Cc: Dmytro Litvinov added
Note: See TracTickets for help on using tickets.
Back to Top