Opened 2 months ago

Last modified 7 weeks ago

#35965 assigned Bug

GenericForeignKeys lose the assigned unsaved object after the object is saved

Reported by: Willem Van Onsem Owned by: YashRaj1506
Component: contrib.contenttypes Version: 5.1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no
Pull Requests:How to create a pull request

Description

As mentioned on StackOverflow, a ForeignKey can still save an object if the object was not saved when setting the model object, like:

one = Model_One()
two = Model_Two(model_one=one)
one.save()
two.save()

But this is not the case for a GenericForeignKey with:

one = Model_One()
two = Model_With_GFK(content_object=one)
one.save()
two.save()

This is because the content_object *immediately* sets the content type field and the foreign key field when setting the object, and then leaves is that way.

Perhaps overkill, but it might be worth to do the same for a ForeignKey and thus check if the object has been given .pk, and if so, set the foreign key field.

According to the ticket's flags, the next step(s) to move this issue forward are:

  • To provide a patch by sending a pull request. Claim the ticket when you start working so that someone else doesn't duplicate effort. Before sending a pull request, review your work against the patch review checklist. Check the "Has patch" flag on the ticket after sending a pull request and include a link to the pull request in the ticket comment when making that update. The usual format is: [https://github.com/django/django/pull/#### PR].

Change History (5)

comment:1 by Sarah Boyce, 2 months ago

Component: Database layer (models, ORM)contrib.contenttypes
Summary: Let GenericForeignKeys behave like a ForeignKey w.r.t. unsaved objectsGenericForeignKeys lose the assigned unsaved object after the object is saved
Triage Stage: UnreviewedAccepted

Thank you for the report!

Here is a potential test. The tagged_item.save() call currently raises an error django.db.utils.IntegrityError: NOT NULL constraint failed: generic_relations_taggeditem.object_id

  • TabularUnified tests/generic_relations/tests.py

    diff --git a/tests/generic_relations/tests.py b/tests/generic_relations/tests.py
    index e0c6fe2db7..32c90f2290 100644
    a b class GenericRelationsTests(TestCase):  
    626626        with self.assertRaisesMessage(ValueError, msg):
    627627            tagged_item.save()
    628628
     629    def test_unsaved_generic_foreign_key_save(self):
     630        quartz = Mineral(name="Quartz", hardness=7)
     631        tagged_item = TaggedItem(tag="shiny", content_object=quartz)
     632        quartz.save()
     633        tagged_item.save()
     634        self.assertEqual(tagged_item.content_object, quartz)
     635
    629636    @skipUnlessDBFeature("has_bulk_insert")
    630637    def test_unsaved_generic_foreign_key_parent_bulk_create(self):

comment:2 by Willem Van Onsem, 2 months ago

Probably we should also do a .refresh_from_db() since the GenericForeignKey probably does some caching :).

in reply to:  1 ; comment:3 by Brock Smickley, 2 months ago

Replying to Sarah Boyce:

Thank you for the report!

Here is a potential test. The tagged_item.save() call currently raises an error django.db.utils.IntegrityError: NOT NULL constraint failed: generic_relations_taggeditem.object_id

  • TabularUnified tests/generic_relations/tests.py

    diff --git a/tests/generic_relations/tests.py b/tests/generic_relations/tests.py
    index e0c6fe2db7..32c90f2290 100644
    a b class GenericRelationsTests(TestCase):  
    626626        with self.assertRaisesMessage(ValueError, msg):
    627627            tagged_item.save()
    628628
     629    def test_unsaved_generic_foreign_key_save(self):
     630        quartz = Mineral(name="Quartz", hardness=7)
     631        tagged_item = TaggedItem(tag="shiny", content_object=quartz)
     632        quartz.save()
     633        tagged_item.save()
     634        self.assertEqual(tagged_item.content_object, quartz)
     635
    629636    @skipUnlessDBFeature("has_bulk_insert")
    630637    def test_unsaved_generic_foreign_key_parent_bulk_create(self):

wait, do we actually want this test to pass? or do we want a similar test with a ForeignKey to fail?

comment:4 by YashRaj1506, 8 weeks ago

Owner: set to YashRaj1506
Status: newassigned

in reply to:  3 comment:5 by YashRaj1506, 7 weeks ago

My question here is, having Foreignkey keep a unsaved object, is this a behaviour we should keep? should this raise a error too when saved with save() ? or we need to adapt the behaviour of genericforeignkeys so that it can also keep an unsaved object? I just looked at this ticket, so i lack a bit more deeper depth into this problem. I will do my research but i am open to other peoples opinion on this.

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