Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#34084 closed Bug (invalid)

ModelForms always set self.instance even when none passed in

Reported by: Steven Mapes Owned by: nobody
Component: Forms Version: 4.1
Severity: Normal Keywords: modelform
Cc: David Kwong Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I came across this bug this morning when running my testsuite against Django 3.2, 4.0 and 4.1 (4.1.0, 4.1.1 and 4.1.2). The issue is with ModelForms when they are used without passing in an existing saved instance. I thought this was related to the regression that was then fixed in 4.1.2 and mentioned in both 33984 and 33952 but the issue is still occurring even though both of those are closed.

To test you can use the below models, form and unit test

# models.py
from django.db import models


class Tag(models.Model):
    tag = models.SlugField(max_length=64, unique=True)


class Thing(models.Model):
    tag = models.ForeignKey(Tag, on_delete=models.CASCADE, related_name="things")
    active = models.BooleanField(default=True)

# forms.py
from django import forms
from example.models import Tag


class TagForm(forms.ModelForm):
    class Meta:
        model = Tag
        fields = ["tag"]

    def __init__(self, *args, **kwargs):
        super(TagForm, self).__init__(*args, **kwargs)

        if self.instance and self.instance.things:
            inactive_things = self.instance.things.filter(active=False)
            # Do something with it

from unittest.case import TestCase
from example.forms import TagForm


class TagFormCase(TestCase):
    def test_required(self):
        """Test required fields"""
        form = TagForm({})
        self.assertFalse(form.is_valid())
        self.assertEqual(
            {
                "tag": ["This field is required."],
            },
            form.errors,
        )


If you run the test on Django versions <4.1 including Django 2.*, Django 3.* and 4.0.* then it'll pass but on 4.1.* it'll fail with ValueError: 'Tag' instance needs to have a primary key value before this relationship can be used.

In order to pass the if statement needs to be changed to if self.instance.pk and self.instance.things as self.instance is no longer None if no instance is passed in.

If your model uses a custom primary key such as a uuid4 then it'll work as expected as the uuid4 is already called

Change History (4)

comment:1 by Steven Mapes, 2 years ago

Component: Database layer (models, ORM)Forms

comment:2 by David Kwong, 2 years ago

Cc: David Kwong added

comment:3 by Mariusz Felisiak, 2 years ago

Resolution: invalid
Status: newclosed
Summary: Regression in Django 4.1 so ModelForms always set self.instance even when none passed inModelForms always set self.instance even when none passed in

Thanks for the report, however this behavior was intentionally changed in 7ba6ebe9149ae38257d70100e8bfbfd0da189862 and it's documented in release notes. Previously an empty queryset was implicitly returned for unsaved instances now you need to protect your code and check self.instance.pk.

There is no change in the ModelForm behavior, self.instance was always set.

in reply to:  3 comment:4 by Steven Mapes, 2 years ago

Replying to Mariusz Felisiak:

Thanks for the report, however this behavior was intentionally changed in 7ba6ebe9149ae38257d70100e8bfbfd0da189862 and it's documented in release notes. Previously an empty queryset was implicitly returned for unsaved instances now you need to protect your code and check self.instance.pk.

There is no change in the ModelForm behavior, self.instance was always set.

Ah yes I missed that entry in the release notes when I was checking. Sorry about that

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