From 6ae7aaa7d6eb880043c4c80009b36e2287bfcb92 Mon Sep 17 00:00:00 2001 From: Viktor Danyliuk Date: Tue, 17 Jul 2018 05:08:43 +0300 Subject: [PATCH] Fixed #29413 -- Prevented evaluation of QuerySet.get_or_create()/update_or_create() defaults unless needed. Removed the logic added in 81e05a418dc6f347d9627373288e773c477a9fe0 which was obsolete since dbffffa7dc95fc62cbecfd00284bde62ee796f48. --- AUTHORS | 1 + django/db/models/query.py | 23 ++++++++------------- tests/get_or_create/tests.py | 40 ++++++++++++++++++++++++++++-------- 3 files changed, 42 insertions(+), 22 deletions(-) diff --git a/AUTHORS b/AUTHORS index e5a22185a9..89b6540f69 100644 --- a/AUTHORS +++ b/AUTHORS @@ -849,6 +849,7 @@ answer newbie questions, and generally made Django that much better: Vasil Vangelovski Victor Andrée viestards.lists@gmail.com + Viktor Danyliuk Ville Säävuori Vinay Sajip Vincent Foley diff --git a/django/db/models/query.py b/django/db/models/query.py index c0d2dee3c1..e023d7749d 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -478,14 +478,14 @@ class QuerySet: Return a tuple of (object, created), where created is a boolean specifying whether an object was created. """ - lookup, params = self._extract_model_params(defaults, **kwargs) # The get() needs to be targeted at the write database in order # to avoid potential transaction consistency problems. self._for_write = True try: - return self.get(**lookup), False + return self.get(**kwargs), False except self.model.DoesNotExist: - return self._create_object_from_params(lookup, params) + params = self._extract_model_params(defaults, **kwargs) + return self._create_object_from_params(kwargs, params) def update_or_create(self, defaults=None, **kwargs): """ @@ -495,13 +495,13 @@ class QuerySet: specifying whether an object was created. """ defaults = defaults or {} - lookup, params = self._extract_model_params(defaults, **kwargs) self._for_write = True with transaction.atomic(using=self.db): try: - obj = self.select_for_update().get(**lookup) + obj = self.select_for_update().get(**kwargs) except self.model.DoesNotExist: - obj, created = self._create_object_from_params(lookup, params) + params = self._extract_model_params(defaults, **kwargs) + obj, created = self._create_object_from_params(kwargs, params) if created: return obj, created for k, v in defaults.items(): @@ -528,15 +528,10 @@ class QuerySet: def _extract_model_params(self, defaults, **kwargs): """ - Prepare `lookup` (kwargs that are valid model attributes), `params` - (for creating a model instance) based on given kwargs; for use by - get_or_create() and update_or_create(). + Prepare `params` for creating a model instance based on the given + kwargs; for use by get_or_create() and update_or_create(). """ defaults = defaults or {} - lookup = kwargs.copy() - for f in self.model._meta.fields: - if f.attname in lookup: - lookup[f.name] = lookup.pop(f.attname) params = {k: v for k, v in kwargs.items() if LOOKUP_SEP not in k} params.update(defaults) property_names = self.model._meta._property_names @@ -554,7 +549,7 @@ class QuerySet: self.model._meta.object_name, "', '".join(sorted(invalid_params)), )) - return lookup, params + return params def _earliest_or_latest(self, *fields, field_name=None): """ diff --git a/tests/get_or_create/tests.py b/tests/get_or_create/tests.py index 1aed318608..e4647d2ab5 100644 --- a/tests/get_or_create/tests.py +++ b/tests/get_or_create/tests.py @@ -5,9 +5,8 @@ from threading import Thread from django.core.exceptions import FieldError from django.db import DatabaseError, IntegrityError, connection -from django.test import ( - SimpleTestCase, TestCase, TransactionTestCase, skipUnlessDBFeature, -) +from django.test import TestCase, TransactionTestCase, skipUnlessDBFeature +from django.utils.functional import lazy from .models import ( Author, Book, DefaultPerson, ManualPrimaryKeyTest, Person, Profile, @@ -178,6 +177,15 @@ class GetOrCreateTests(TestCase): defaults={"birthday": lambda: raise_exception()}, ) + def test_defaults_not_evaluated_unless_needed(self): + """`defaults` aren't evaluated if the instance isn't created.""" + def raise_exception(): + raise AssertionError + obj, created = Person.objects.get_or_create( + first_name='John', defaults=lazy(raise_exception, object)(), + ) + self.assertFalse(created) + class GetOrCreateTestsWithManualPKs(TestCase): @@ -443,6 +451,19 @@ class UpdateOrCreateTests(TestCase): self.assertIs(created, False) self.assertEqual(obj.last_name, 'NotHarrison') + def test_defaults_not_evaluated_unless_needed(self): + """`defaults` aren't evaluated if the instance isn't created.""" + Person.objects.create( + first_name='John', last_name='Lennon', birthday=date(1940, 10, 9) + ) + + def raise_exception(): + raise AssertionError + obj, created = Person.objects.get_or_create( + first_name='John', defaults=lazy(raise_exception, object)(), + ) + self.assertFalse(created) + class UpdateOrCreateTestsWithManualPKs(TestCase): @@ -515,15 +536,17 @@ class UpdateOrCreateTransactionTests(TransactionTestCase): self.assertEqual(updated_person.last_name, 'NotLennon') -class InvalidCreateArgumentsTests(SimpleTestCase): +class InvalidCreateArgumentsTests(TransactionTestCase): + available_apps = ['get_or_create'] msg = "Invalid field name(s) for model Thing: 'nonexistent'." + bad_field_msg = "Cannot resolve keyword 'nonexistent' into field. Choices are: id, name, tags" def test_get_or_create_with_invalid_defaults(self): with self.assertRaisesMessage(FieldError, self.msg): Thing.objects.get_or_create(name='a', defaults={'nonexistent': 'b'}) def test_get_or_create_with_invalid_kwargs(self): - with self.assertRaisesMessage(FieldError, self.msg): + with self.assertRaisesMessage(FieldError, self.bad_field_msg): Thing.objects.get_or_create(name='a', nonexistent='b') def test_update_or_create_with_invalid_defaults(self): @@ -531,11 +554,11 @@ class InvalidCreateArgumentsTests(SimpleTestCase): Thing.objects.update_or_create(name='a', defaults={'nonexistent': 'b'}) def test_update_or_create_with_invalid_kwargs(self): - with self.assertRaisesMessage(FieldError, self.msg): + with self.assertRaisesMessage(FieldError, self.bad_field_msg): Thing.objects.update_or_create(name='a', nonexistent='b') def test_multiple_invalid_fields(self): - with self.assertRaisesMessage(FieldError, "Invalid field name(s) for model Thing: 'invalid', 'nonexistent'"): + with self.assertRaisesMessage(FieldError, self.bad_field_msg): Thing.objects.update_or_create(name='a', nonexistent='b', defaults={'invalid': 'c'}) def test_property_attribute_without_setter_defaults(self): @@ -543,5 +566,6 @@ class InvalidCreateArgumentsTests(SimpleTestCase): Thing.objects.update_or_create(name='a', defaults={'name_in_all_caps': 'FRANK'}) def test_property_attribute_without_setter_kwargs(self): - with self.assertRaisesMessage(FieldError, "Invalid field name(s) for model Thing: 'name_in_all_caps'"): + msg = "Cannot resolve keyword 'name_in_all_caps' into field. Choices are: id, name, tags" + with self.assertRaisesMessage(FieldError, msg): Thing.objects.update_or_create(name_in_all_caps='FRANK', defaults={'name': 'Frank'})