Code

#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.

Attachments (0)

Change History (5)

comment:1 Changed 13 months ago by bmispelon

  • Cc bmispelon@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 13 months ago by bmispelon

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 Changed 13 months ago by valtron2000@…

  • Type changed from Bug to New 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 Changed 13 months ago by bmispelon

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

comment:5 Changed 13 months ago by bmispelon

  • Resolution set to duplicate
  • Status changed from new to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.