Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#24325 closed Bug (fixed)

Cannot reference FK relation from inline ModelForm.save()

Reported by: Stanislas Guerra Owned by: Tim Graham
Component: Documentation Version: 1.8alpha1
Severity: Release blocker Keywords: inline ModelForm
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi devs,

Not sure that's a bug but that's a regression. In Django-1.7 with the following models:

from django.db import models


class Author(models.Model):
    name = models.TextField()


class Book(models.Model):
    author = models.ForeignKey("Author")
    title = models.TextField()

You can access book.author in the ModelForm.save() method:
admin.py:

from django import forms
from django.contrib import admin
from library import models


class BookForm(forms.ModelForm):
    class Meta:
        model = models.Book
        exclude = ()

    def save(self, *args, **kwargs):
        book = super(BookForm, self).save(*args, **kwargs)
        book.title = "%s by %s" % (book.title, book.author.name)
        return book


class BookInline(admin.TabularInline):
    model = models.Book
    extra = 1
    form = BookForm


class AuthorAdmin(admin.ModelAdmin):
    inlines = [BookInline]


admin.site.register(models.Author, AuthorAdmin)

And since commit 45e049937d you get a RelatedObjectDoesNotExist exception: Book has no author.

Is it considered a normal behaviour?

Thanks.

Attachments (1)

24325.diff (1.3 KB) - added by Tim Graham 5 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 5 years ago by Stanislas Guerra

comment:2 Changed 5 years ago by Stanislas Guerra

A small clarification: This exception happens when you add a new Author and create a Book with the inline in the same request.

comment:3 Changed 5 years ago by Marten Kenbeek

Triage Stage: UnreviewedAccepted

comment:4 Changed 5 years ago by Marten Kenbeek

Type: UncategorizedBug

comment:5 Changed 5 years ago by Tim Graham

Severity: NormalRelease blocker

This might need to be documented on a backwards incompatible change, but I haven't fully investigated the issue.

comment:6 Changed 5 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

Changed 5 years ago by Tim Graham

Attachment: 24325.diff added

comment:7 Changed 5 years ago by Tim Graham

Has patch: set

Hi Starou, could you take a look at the attached documentation note and let me know if addresses your issue?

comment:8 Changed 5 years ago by Stanislas Guerra

Hi Tim,

I am affraid not. In the Book/Author example yes but not in my project because I use the pk of the parent object to create a path to store some images on the file system. And the pk isn't in the cleaned-data.

Here a pseudo-function to illustrate (imagine Book having a cover ImageField attribute with upload_to parameter bound to the next):

def image_upload_path(instance, filename):
    return "authors/%d/books/%d/%s" % (
        instance.author.pk,
        instance.pk,
        filename
    )


I agree with the concept of not binding an unsaved object in a relation but I think the exception is raised because of the parent instance being saved later in the process than before commit @5e049937d (because if you get pk, the object was saved, right?)
I am not experienced enough with the Django code to be 100% positive, just an intuition :-)

comment:9 Changed 5 years ago by Tim Graham

That code isn't working on 1.7 for me:

def image_upload_path(instance, filename):
    return "authors/%d/books/%d/%s" % (
        instance.author.pk,
        instance.pk,
        filename
    )

class Author(models.Model):
    name = models.TextField()

class Book(models.Model):
    author = models.ForeignKey("Author")
    title = models.TextField()
    cover = models.ImageField(blank=True, upload_to=image_upload_path)

Exception:

TypeError at /admin/library/author/add/

%d format: a number is required, not NoneType

because instance.pk is None.

This is as documented: "In most cases, this object will not have been saved to the database yet, so if it uses the default AutoField, it might not yet have a value for its primary key field."

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:10 Changed 5 years ago by Stanislas Guerra

You are rigtht about the ImageField. The code in my project is a bit more complex than that with some nested forms and inlines etc and it runs under 1.7 and not 1.8.

I will investigate next week if I can find some time.

By the way, using book.author.pk instead of book.author.name in my previous demonstration gives the same result (works before commit @5e049937d)

Thank you for your time!

comment:11 Changed 5 years ago by Tim Graham

Component: FormsDocumentation

You're welcome. I'll commit the doc patch and close this. Please open a new ticket if you find something else.

comment:12 Changed 5 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 0af3822dc362b6253bda1c9699466dd0bbbf6066:

Fixed #24325 -- Documented change in ModelForm.save() foreign key access.

comment:13 Changed 5 years ago by Tim Graham <timograham@…>

In 8657e7caaac41266d9ac0b73a21af64edc681613:

[1.8.x] Fixed #24325 -- Documented change in ModelForm.save() foreign key access.

Backport of 0af3822dc362b6253bda1c9699466dd0bbbf6066 from master

comment:14 Changed 5 years ago by Tim Graham <timograham@…>

In d298b1ba5043eaa40f3f4bebe3c7634b359ba34b:

Reverted "Fixed #24325 -- Documented change in ModelForm.save() foreign key access."

This reverts commit 0af3822dc362b6253bda1c9699466dd0bbbf6066.
It's obsoleted by refs #24395.

comment:15 Changed 5 years ago by Tim Graham <timograham@…>

In 81911f29b70710d8f72916c49a69aa5df5cd7df8:

[1.8.x] Reverted "Fixed #24325 -- Documented change in ModelForm.save() foreign key access."

This reverts commit 0af3822dc362b6253bda1c9699466dd0bbbf6066.
It's obsoleted by refs #24395.

Backport of d298b1ba5043eaa40f3f4bebe3c7634b359ba34b from master

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