Fixed #36235 -- Fixed RelatedManager queryset losing related instance in get_or_create() and update_or_create().

[JaeHyuckSa]

Trac ticket number

ticket-36235

Branch description

Fixed an issue where calling get_or_create() or update_or_create() on a queryset obtained from RelatedManager.all() lost the context of the related instance, causing an IntegrityError.

@ngnpope Thank you for providing the test code for this ticket.

Checklist

• This PR targets the main branch.
• The commit message is written in past tense, mentions the ticket number, and ends with a period.
• I have checked the "Has patch" ticket flag in the Trac system.
• I have added or updated relevant tests.
• I have added or updated relevant docs, including release notes if applicable.
• I have attached screenshots in both light and dark modes for any UI changes.

[ngnpope]

Thanks for picking this up.

One thing I didn't mention on the ticket was that .get_or_update() and .update_or_create() were overridden on the RelatedManager created under create_reverse_many_to_one_manager(). The test I provided was for that case... But there is also ManyRelatedManager created under create_forward_many_to_many_manager(). Could you add a test to check whether that case is also fixed? (Actually, I'm not sure whether there is anything to do in the M2M case. Perhaps worth checking?)

I think, looking at RelatedManager, that this may also be needed for .create()?

Also, are there any issues with doing this as regards the database router stuff in the RelatedManager that won't be applied in this fix for calling these methods on the QuerySet?

[JaeHyuckSa]

@ngnpope Thank you for taking the time to review this despite your busy schedule.
Here are my answers to a few of your questions.

One thing I didn't mention on the ticket was that .get_or_update() and .update_or_create() were overridden on the RelatedManager created under create_reverse_many_to_one_manager(). The test I provided was for that case... But there is also ManyRelatedManager created under create_forward_many_to_many_manager(). Could you add a test to check whether that case is also fixed? _(Actually, I'm not sure whether there is anything to do in the M2M case. Perhaps worth checking?)

I've added test code to examine this part, and it still returns a QuerySet. While I was able to check using _known_related_objects in RelatedManager, I'm unsure which approach to take with ManyRelatedManager. Could you advise me on the right direction for this?

I think, looking at RelatedManager, that this may also be needed for .create()?

I agree with you on this point. I'm planning to add test code for the create() method—could you please advise me on the best location to place this test?

Also, are there any issues with doing this as regards the database router stuff in the RelatedManager that won't be applied in this fix for calling these methods on the QuerySet?

I'll need a bit more time to look into this part, so I'll follow up with you later.

[sarahboyce]

I agree with you on this point. I'm planning to add test code for the create() method—could you please advise me on the best location to place this test?

I would add something like:

--- a/tests/many_to_one/tests.py
+++ b/tests/many_to_one/tests.py
@@ -807,6 +807,11 @@ class ManyToOneTests(TestCase):
         d2 = city.districts.create(name="Goa")
         self.assertSequenceEqual(city.districts.all(), [d1, d2])
 
+    def test_create_on_related_queryset(self):
+        c = City.objects.create(name="Musical City")
+        c.districts.all().create(name="Ladida")
+        self.assertEqual(c.districts.count(), 1)
+
     def test_clear_after_prefetch(self):
         c = City.objects.create(name="Musical City")

[charettes]

Is there a reason we are picking the first field and the first instance for this field? It will work in the test case but _known_related_objects has a type of {field: {pk: Model}} which can contain multiple fields and multiple instances when Queryset combination is used (see usage of _merge_known_related_objects)

django/django/db/models/query.py

Lines 2008 to 2013 in bc1bfe1

	def _merge_known_related_objects(self, other):
	"""
	Keep track of all known related objects from either QuerySet instance.
	"""
	for field, objects in other._known_related_objects.items():
	self._known_related_objects.setdefault(field, {}).update(objects)

In other words, I don't think we can assume that _known_related_objects will only contain a single field and related object as assigned by create_reverse_many_to_one_manager.

[ngnpope]

So from the queryset we lose reference to self.field and self.instance on the RelatedManager. These are used by that manager to automatically pass in the related instance to .create(), .get_or_create(), and .update_or_create().

I'd merely seen that we already have access to these on the queryset via _known_related_objects and thought that could be a useful starting point for investigation. Also I'd seen:

django/django/db/models/fields/related_descriptors.py

Lines 730 to 732 in 8b241f8

	queryset._known_related_objects = {
	self.field: {rel_obj_id: self.instance}
	}

I guess that we'll just have to duplicate this stuff and explicitly set another attribute on QuerySet that we can use? Do you have any other ideas? 🤔