Opened 11 years ago

Closed 11 years ago

#20202 closed New feature (duplicate)

BaseModelForm uses model_to_dict to provide initial, causes problems for ModelChoiceField with non-pk to_field_name

Reported by: valtron2000@… Owned by: nobody
Component: Forms Version: 1.5
Severity: Normal Keywords:
Cc: bmispelon@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

class M(models.Model):
  slug = models.CharField(max_length = 5, unique = True)
  other = models.ForeignKey('M', blank = True, null = True)

class F(forms.ModelForm):
  class Meta:
    model = M
    fields = ('other',)
  
  other = forms.ModelChoiceField(to_field_name = 'slug')

def example():
  m1 = M.objects.create(slug = 'xyzzy')
  m2 = M.objects.create(slug = 'abcde', other = m1)
  form = F(instance = m2)
  
  # Prints m1.pk; should contain the model instance, instead,
  # so that the ModelChoiceField can actually get the to_field_name attribute
  print form.initial['other']
  
  # Prints this: (note that xyzzy isn't selected="selected")
  # <tr><th><label for="id_other">Other:</label></th><td><select id="id_other" name="other">
  # <option value="">---------</option>
  # <option value="xyzzy">M object</option>
  # <option value="abcde">M object</option>
  # </select></td></tr>
  print form

There is a workaround: manually provide the correct field in the initial.

Change History (5)

comment:1 by Baptiste Mispelon, 11 years ago

Cc: bmispelon@… added
Triage Stage: UnreviewedAccepted

This definitely looks like a bug but it might be a bit tricky to fix (it will at least require some changes to model_to_dict's signature).

As noted, the issue is that model_to_dict will always return the primary key of the model, not taking into account the field's to_field_name attribute.

The problem is that model_to_dict does not have access to the form's fields: it only gets passed the instance, a list of strings corresponding to the model's fields present in the form, and a list of strings of the fields to exclude. Therefore, it has no way to access the to_field_name attribute and adjust the value accordingly.

On a side-note, it seems that ModelChoiceField.to_field_name is completely undocumented, though there seem to be a few tests for it.

This ticket could be a good excuse to finally document it.

comment:2 by Baptiste Mispelon, 11 years ago

I've been digging a bit and now I'm not sure if this is technically a bug or not.

The main problem is that since the to_field_name attribute is undocumented, what it's supposed to do is not exactly clear.

The fact is that there is a (passing) test the issue you describe: https://github.com/django/django/blob/master/tests/model_forms/tests.py#L1508-L1513

Without documentation, a test is probably the next best thing to know the intended behavior.

From the test, it looks like the form field's to_field_name should match the model field's to_field, which in your example is not the case (and causes the issue).

Note that the test models is quite similar to your example (the difference being the existence of the to_field attribute): https://github.com/django/django/blob/master/tests/model_forms/models.py#L174

So I'm leaving this as "Accepted", but I think the first thing to do would be to document ModelChoiceField.to_field_name and then make sure the implementation matches the documentation.

comment:3 by valtron2000@…, 11 years ago

Type: BugNew feature

I see, to_field_name is supposed to be mostly internal, because it's usually set by ForeignKey.formfield (https://github.com/django/django/blob/master/django/db/models/fields/related.py#L1244). The only time the user has to provide it is if they're defining the field manually rather than though Meta.fields. I agree that this should be documented.

However, my use case is: my FKs use the PK, but my models also have a uuid field. I don't want to expose PKs in the front end, so I was in the process of (as of now, incorrectly) using to_field_name to use the uuid for a bunch of fields. Therefore, I'm turning this into a feature request.

I know that a lot of people currently use model_to_dict as it stands, so changing its behaviour isn't an option. It would be better to make a private _get_initial_from_instance that works the same but returns the instance instead of the PK for FK fields, and then it becomes the form field's responsibility to turn the instance into an HTML form field value.

comment:4 by Baptiste Mispelon, 11 years ago

I made a first attempt at documenting the existing behavior: https://github.com/bmispelon/django/compare/ticket-20202

comment:5 by Baptiste Mispelon, 11 years ago

Resolution: duplicate
Status: newclosed

Well as it turns out, this issue has already been reported.

I'm marking this one as a duplicate. All further discussion should be done in the original ticket: #17657

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