Opened 6 years ago

Closed 2 years ago

Last modified 2 years ago

#13794 closed Bug (fixed)

Django does not respect to_field's model on an inline model admin

Reported by: sebastien@… Owned by: gautier
Component: Forms Version: master
Severity: Normal Keywords: admin inline to_field formset
Cc: ghayoun@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Aymeric Augustin)

The problem occurs in the function __unicode__ of ModelB

When editing a ModelA instance in admin site, all ModelB instances have a modela_id set to the pk of the ModelA instance, instead of the value of the to_field's field. And, when accessing to the field modela of a ModelB instance, a DoesNotExist exception is raised.

This problem does not occur in the shell :

In [3]: ModelA.objects.all()[0].modelb_set.all()[0].modela_id
Out[3]: u'TRUC'

In [6]: ModelB.objects.all()[0].modela_id
Out[6]: u'TRUC'

See below to reproduce

# models.py

class ModelA(models.Model):
    code = models.CharField(max_length=20, unique=True)

class ModelB(models.Model):
    modela = models.ForeignKey(ModelA, to_field="code")
    position = models.IntegerField()

    def __unicode__(self):
        return u"%s" % self.modela

# admin.py

class ModelBInlineAdmin(admin.TabularInline):
    model = ModelB

class ModelAAdmin(admin.ModelAdmin):
    model = ModelA
    inlines = [ModelBInlineAdmin]

admin.site.register(ModelA, ModelAAdmin)

Attachments (6)

fix_inline_model_with_to_field.diff (1.5 KB) - added by gautier 6 years ago.
Fix + regression test
ticket13794.patch (1.7 KB) - added by gautier 6 years ago.
Fix + regression test, updated for new trunk
to_field_formsets.patch (1.8 KB) - added by anonymous 4 years ago.
to_field_formsets.2.patch (741 bytes) - added by anonymous 3 years ago.
to_field_formsets.3.diff​ (1.8 KB) - added by anonymous 2 years ago.
to_field_formsets.4.diff (3.6 KB) - added by anonymous 2 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 6 years ago by anonymous

Cc: ghayoun@… added

comment:2 Changed 6 years ago by gautier

Has patch: set
Keywords: formset added
Owner: changed from nobody to gautier
Status: newassigned
Triage Stage: UnreviewedAccepted

Same cause as #10243, #11043, fixed by [10756].

But the merging of soc2009/model-validation in [12098] broke the fix, by forcing the value of the related primary key into the foreign key field.

My patch removes this behaviour, and add a regression test. All other unit tests still pass.

Changed 6 years ago by gautier

Fix + regression test

comment:3 Changed 6 years ago by Julien Phalip

Component: django.contrib.adminForms

comment:4 Changed 6 years ago by Graham King

Severity: Normal
Type: Bug

comment:5 Changed 6 years ago by patchhammer

Easy pickings: unset
Patch needs improvement: set

fix_inline_model_with_to_field.diff fails to apply cleanly on to trunk

Changed 6 years ago by gautier

Attachment: ticket13794.patch added

Fix + regression test, updated for new trunk

comment:6 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

Changed 4 years ago by anonymous

Attachment: to_field_formsets.patch added

comment:7 Changed 4 years ago by anonymous

Patch needs improvement: unset
Version: 1.21.5-beta-1

I updated the patch so that is does not change behavior at all for no to_field formsets. Any feedback or suggestions welcome.

This bug has been present for a while and would be nice to get fixed.

comment:8 Changed 4 years ago by anonymous

Status: assignednew

comment:9 Changed 4 years ago by Aymeric Augustin

Description: modified (diff)

Added a bit of markup for reaadability.

comment:10 Changed 4 years ago by Aymeric Augustin

I asked Florian (who reported #11043) to have a look; he didn't have much time but here are his initial thoughts:

that's confusing code; when you look at django/forms/models.py line 707 -- do you have any idea where self.fk should come from?

    FormSet = modelformset_factory(model, **kwargs)
    FormSet.fk = fk

both patches seem to fix the problem (or at least the attached testcase), which makes me think that the testcase isn't good enough, might have to trigger validation
I can't figure out why get_queryset is affected by _construct_form
ah constructing a formset executes the query and then changes the instances, now on to figure out if there could be a problem if that data is deleted :/
https://code.djangoproject.com/attachment/ticket/13794/fix_inline_model_with_to_field.diff this patch looks better, the updated one seems wrong or useless

comment:11 Changed 4 years ago by anonymous

The patch 'to_field_formsets.patch' was an attempt to fix the problem in the safest way possible. All tests pass with line 719 removed, but the comment above suggests that line should not be removed. If you are convinced that line isn't needed then fix_inline_model_with_to_field.diff is fine. I just wanted to provide an option that is guaranteed not to break any working code but the fix the problem.

If you think this needs more investigation please let me know and I'll try to find out if that line is just a relic of the old way or if it is actually used.

comment:12 Changed 4 years ago by anonymous

After further investigation it seems that main functional difference between the two is that not setting the fk value on form construction (fix_inline_model_with_to_field.diff) results in the relationship field not being available on a new instance until after full_clean, specifically construct_instance, is called on the formset. Setting the fk value on construction (to_field_formsets.patch), means that the fk value and subsequently the relationship is available when dealing with a new item.

Adding this line to the test in to_field_formsets.path will make the test pass for that patch but fail for fix_inline_model_with_to_field.diff:

self.assertEqual(formset[1].instance.user_id, "guido")

So there is a slight chance of fix_inline_model_with_to_field.diff breaking currently working code while to_field_formsets.patch will not. But as far as I can tell this undocumented so it's really your call either way.

The problem is that right now, this does have potential to create invalid relationships, leading to data corruption. If the value set in _construct_form happens to be the same as a valid pk value of that same table, then the save happens silently resetting all your relationships to point to the wrong record.

comment:13 Changed 4 years ago by anonymous

Severity: NormalRelease blocker

comment:14 Changed 4 years ago by Aymeric Augustin

Severity: Release blockerNormal

Why would this be a release blocker?

comment:15 Changed 4 years ago by anonymous

Because it corrupts your data.

comment:16 Changed 4 years ago by Aymeric Augustin

This bug was filed three years ago and Django has had several major releases since then.

If you think it should block the next release please explain why on the django-developers mailing list, with a little bit more than 5 words. Thank you!

comment:17 Changed 4 years ago by anonymous

Actually I did but I'm having a hard time getting any response. I did the 5 for one, but I think that part of the problem is that the title of the bug makes it seem like a minor defect. Granted, like most of the to_field related bugs, it is a bit of an edge case and probably not too many people have run into it, however for those that do it can silently and irreparably replace your relationships. Leading to very hard to track down bugs and real data loss. My understanding is that these sorts of bugs are the ones that are release blockers.

There are two possible patches here and I think I outlined the differences and potential problems with each above. If more clarification is needed I am happy to help or improve the patches in any way I can.

Changed 3 years ago by anonymous

Attachment: to_field_formsets.2.patch added

comment:18 Changed 3 years ago by anonymous

Version: 1.5-beta-1master

Updated patch to apply against master. Would be really nice to get this fixed. Again this bug can have the nasty side effect of reseting your relationships to point to the wrong record.

comment:19 Changed 2 years ago by Tim Graham

Patch needs improvement: set

The latest version of of the patch doesn't include the test from the earlier version. Please uncheck "Patch needs improvement" if someone can update it. Thanks.

Changed 2 years ago by anonymous

Attachment: to_field_formsets.3.diff​ added

comment:20 Changed 2 years ago by anonymous

Patch needs improvement: unset

Ok patch updated with the test.

comment:21 Changed 2 years ago by Tim Graham

No tests fail if I remove the line fk_value = getattr(fk_value, 'pk', fk_value). Could you clarify if that's needed and add a test if so?

Changed 2 years ago by anonymous

Attachment: to_field_formsets.4.diff added

comment:22 Changed 2 years ago by anonymous

It's an edge case of an edge case. It was done to match what happens in save_new. Added a test.

comment:23 Changed 2 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In 5e2c4a4bd1f86962842783e0b42ada7b9c14c247:

Fixed #13794 -- Fixed to_field usage in BaseInlineFormSet.

Thanks sebastien at clarisys.fr for the report and gautier
for the patch.

comment:24 Changed 2 years ago by anonymous

Thanks for the commit, any chance of this making it into 1.7? It is a potentially destructive bug so it would be nice to see it in a stable version sooner rather than later.

comment:25 Changed 2 years ago by Claude Paroz

+1 to backport.

comment:26 Changed 2 years ago by Tim Graham <timograham@…>

In 736e289445aea5083da04ddd8734c3ff20310408:

[1.7.x] Fixed #13794 -- Fixed to_field usage in BaseInlineFormSet.

Thanks sebastien at clarisys.fr for the report and gautier
for the patch.

Backport of 5e2c4a4bd1 from master

comment:27 Changed 2 years ago by Ramiro Morales

IMO, as the fixed issue can cause data loss in formsets (and not only in contrib.admin inlines like its summary mentions) it qualifies for getting backported to all the currently supported branches.

I've pushed the backport commits to:

Opinions?

comment:28 Changed 2 years ago by Tim Graham

1.4 and 1.5 are in security-fix only mode and I don't think 1.5 will get another release unless there are security issues before 1.7 is released. I'd be fine with backporting to 1.6, although it's funny to consider it a critical bug when it's been open for 4 years.

comment:29 Changed 2 years ago by Ramiro Morales <cramm0@…>

In b44519072e8a0ef56a0ae9e6e4a1fb04273eb0eb:

[1.4.x] Fixed #13794 -- Fixed to_field usage in BaseInlineFormSet.

Thanks sebastien at clarisys.fr for the report and gautier
for the patch.

Backport of 5e2c4a4bd1 from master

comment:30 Changed 2 years ago by Ramiro Morales <cramm0@…>

In 4ae68f677b3348765d8649d8b57beffa18fe8d3d:

[1.5.x] Fixed #13794 -- Fixed to_field usage in BaseInlineFormSet.

Thanks sebastien at clarisys.fr for the report and gautier
for the patch.

Backport of 5e2c4a4bd1 from master

comment:31 Changed 2 years ago by Ramiro Morales <cramm0@…>

In 685582940bf4dfb5caf09bd8a73e6f74745190a8:

[1.6.x] Fixed #13794 -- Fixed to_field usage in BaseInlineFormSet.

Thanks sebastien at clarisys.fr for the report and gautier
for the patch.

Backport of 5e2c4a4bd1 from master

comment:32 Changed 2 years ago by Ramiro Morales <cramm0@…>

In aa9c45c2e425662d980cca82974da0986fdbe406:

[1.4.x] Revert "Fixed #13794 -- Fixed to_field usage in BaseInlineFormSet."

This reverts commit b44519072e8a0ef56a0ae9e6e4a1fb04273eb0eb.

stable/1.4.x branch is in security-fixes-only mode.

comment:33 Changed 2 years ago by Ramiro Morales <cramm0@…>

In 291e837bda016d8556c2f6a4712729955a1667d0:

[1.5.x] Revert "Fixed #13794 -- Fixed to_field usage in BaseInlineFormSet."

This reverts commit 4ae68f677b3348765d8649d8b57beffa18fe8d3d.

stable/1.5.x branch is in security-fixes-only mode.

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