Opened 14 months ago

Last modified 4 weeks ago

#36235 assigned Bug

RelatedManager.all().get_or_create() does not work

Reported by: Nick Pope Owned by: Johanan Oppong Amoateng
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: get_or_create, related, manager
Cc: Johanan Oppong Amoateng Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Nick Pope)

When accessing the queryset for a related manager, .get_or_create() and .update_or_create() lose context of the related instance.

Calling on the related manager works as expected:

publisher.books.get_or_create(name="The Very Hungry Caterpillar")

This is the case that was fixed by #3121 and #23611.

But calling on the queryset causes an IntegrityError to be raised:

publisher.books.all().get_or_create(name="The Very Hungry Caterpillar")

This can be a subtle failure that is hard to understand, especially if publisher.books.all() is assigned to a variable earlier.

The challenge here is that the overridden methods on the manager set kwargs[self.field.name] = self.instance.

We'd need to be able to pass this information down to the queryset. It looks like this might already be available in _known_related_objects which is set by RelatedManager._apply_rel_filters(), so that would be a good starting point for investigation.

We would also need to check whether something needs to be done to select the correct database as RelatedManager.get_or_create() has handling for this:

            db = router.db_for_write(self.model, instance=self.instance)

If this is something we can't fix reliably, then we should update the admonition under .get_or_create() in the docs and probably update .update_or_create() to make this and other issues related to use through RelatedManager more clear.

Change History (13)

comment:1 by Nick Pope, 14 months ago

The following test can be added to GetOrCreateTests in tests/get_or_create/tests.py below test_get_or_create_on_related_manager:

    def test_get_or_create_on_related_queryset(self):
        p = Publisher.objects.create(name="Acme Publishing")
        # Create a book through the publisher.
        book, created = p.books.all().get_or_create(name="The Book of Ed & Fred")
        self.assertTrue(created)
        # The publisher should have one book.
        self.assertEqual(p.books.count(), 1)

comment:2 by Nick Pope, 14 months ago

Description: modified (diff)

comment:3 by Sarah Boyce, 14 months ago

Triage Stage: UnreviewedAccepted

Thank you for the report and test!

comment:4 by JaeHyuckSa, 14 months ago

Has patch: set
Owner: set to JaeHyuckSa
Status: newassigned

comment:5 by JaeHyuckSa, 14 months ago

Has patch: unset

comment:6 by JaeHyuckSa, 14 months ago

Has patch: set

comment:7 by Sarah Boyce, 11 months ago

Patch needs improvement: set

in reply to:  6 comment:8 by Johanan Oppong Amoateng, 5 months ago

Replying to JaeHyuckSa:

Are you still working on this?

Last edited 5 months ago by Johanan Oppong Amoateng (previous) (diff)

comment:9 by Johanan Oppong Amoateng, 5 months ago

Cc: Johanan Oppong Amoateng added

comment:10 by JaeHyuckSa, 5 months ago

Hello! I forgot that Johanan had a ticket. At the moment, I don’t have time to work on this, so please feel free to take it on if you’re able to.

comment:11 by Johanan Oppong Amoateng, 5 months ago

Owner: changed from JaeHyuckSa to Johanan Oppong Amoateng

comment:12 by Vasilis Vagenas, 4 weeks ago

Hello.
The problem can still be reproduced:

  • Git commit hash: ed79c5959add54b6e8ea589ec601e0d2e801517e
  • test method: the method test_get_or_create_on_related_queryset, mentioned above by Nick Pope
  • test output:
======================================================================
ERROR: test_get_or_create_on_related_queryset (get_or_create.tests.GetOrCreateTests.test_get_or_create_on_related_queryset)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vasilis/oss_projects/django/django/django/db/models/query.py", line 1029, in get_or_create
    return self.get(**kwargs), False
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/query.py", line 681, in get
    raise self.model.DoesNotExist(
    ^^^^^^^^^^^^^^^^^
get_or_create.models.Book.DoesNotExist: Book matching query does not exist.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/vasilis/oss_projects/django/django/django/db/backends/utils.py", line 105, in _execute
    return self.cursor.execute(sql, params)
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/backends/sqlite3/base.py", line 359, in execute
    return super().execute(query, params)
    ^^^^^^^^^^^^^^^^^
sqlite3.IntegrityError: NOT NULL constraint failed: get_or_create_book.publisher_id_column

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/lib/python3.12/unittest/case.py", line 58, in testPartExecutor
    yield
  File "/usr/lib/python3.12/unittest/case.py", line 634, in run
    self._callTestMethod(testMethod)
    ^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/unittest/case.py", line 589, in _callTestMethod
    if method() is not None:
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/tests/get_or_create/tests.py", line 159, in test_get_or_create_on_related_queryset
    book, created = p.books.all().get_or_create(name="The Book of Ed & Fred")
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/query.py", line 1036, in get_or_create
    return self.create(**params), True
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/query.py", line 711, in create
    obj.save(force_insert=True, using=self.db)
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/base.py", line 896, in save
    self.save_base(
  File "/home/vasilis/oss_projects/django/django/django/db/models/base.py", line 988, in save_base
    updated = self._save_table(
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/base.py", line 1191, in _save_table
    results = self._do_insert(
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/base.py", line 1243, in _do_insert
    return manager._insert(
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/manager.py", line 87, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/query.py", line 2108, in _insert
    return query.get_compiler(using=using).execute_sql(returning_fields)
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/models/sql/compiler.py", line 1935, in execute_sql
    cursor.execute(sql, params)
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/backends/utils.py", line 79, in execute
    return self._execute_with_wrappers(
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/backends/utils.py", line 92, in _execute_with_wrappers
    return executor(sql, params, many, context)
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/backends/utils.py", line 100, in _execute
    with self.db.wrap_database_errors:
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/utils.py", line 94, in __exit__
    raise dj_exc_value.with_traceback(traceback) from exc_value
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/backends/utils.py", line 105, in _execute
    return self.cursor.execute(sql, params)
    ^^^^^^^^^^^^^^^^^
  File "/home/vasilis/oss_projects/django/django/django/db/backends/sqlite3/base.py", line 359, in execute
    return super().execute(query, params)
    ^^^^^^^^^^^^^^^^^
django.db.utils.IntegrityError: NOT NULL constraint failed: get_or_create_book.publisher_id_column

----------------------------------------------------------------------
Version 1, edited 4 weeks ago by Vasilis Vagenas (previous) (next) (diff)

comment:13 by Vasilis Vagenas, 4 weeks ago

Some more notes on the issue:

  • As also specified in the pull request, the create method of the queryset is the root of the issue; e.g. the following test shows the issue by throwing an IntegrityError:
        def test_create_on_related_queryset(self):
            p = Publisher.objects.create(name="Acme Publishing")
            p.books.all().create(name="The Book of Ed & Fred")
            self.assertEqual(p.books.count(), 1)
    
  • A question raised is what the behavior should be in the case of queryset operations which create ambiguity for the implicit related object (e.g. for publisher in this case). A typical example is the union of querysets, when the querysets have different implicit related objects. For example, should the test below continue throwing an IntegrityError, i.e. leaving the publisher equal to None?
        def test_create_on_union_of_querysets_when_publisher_not_set(self):
            p1 = Publisher.objects.create(name="Acme Publishing")
            p2 = Publisher.objects.create(name="Packt Publishing")
            books_union = p1.books.all().union(p2.books.all())
            books_union.create(name="The Book of Ed & Fred")
            self.assertEqual(Book.objects.count(), 1)
    
  • Implementation notes:
    • in the test above, books_union._known_related_objects has just 1 publisher, so this attribute currently does not support the detection of the ambiguous case.
    • any future fix should not override a foreign key relation specified explicitly, e.g. the following should succeed:
          def test_create_on_union_of_querysets_when_publisher_set(self):
              p1 = Publisher.objects.create(name="Acme Publishing")
              p2 = Publisher.objects.create(name="Packt Publishing")
              books_union = p1.books.all().union(p2.books.all())
              books_union.create(name="The Book of Ed & Fred", publisher=p2)
              self.assertEqual(p2.books.count(), 1)
      
    • regarding all 3 test methods above and the test_get_or_create_on_related_queryset mentioned above, a draft implementation that makes them succeed could be to add to the beginning of django.db.models.query.QuerySet.create the following snippet:
              for field in self._known_related_objects:
                  field_dict = self._known_related_objects[field]
                  if len(field_dict) == 1:
                      field_id_name = f'{field.name}_id'
                      field_explicitly_specified = field.name in kwargs or field_id_name in kwargs
                      if not field_explicitly_specified:
                          model_instance = list(field_dict.values())[0]
                          kwargs[field.name] = model_instance
      
Note: See TracTickets for help on using tickets.
Back to Top