From f479df7f8d03ab767bb5e5d655243191087d6432 Mon Sep 17 00:00:00 2001 From: Daniyal Date: Wed, 24 Mar 2021 11:15:08 +0530 Subject: [PATCH] Refs #32508 -- Raised Type/ValueError instead of using "assert" in django.db.models. Co-authored-by: Mariusz Felisiak --- django/db/models/base.py | 10 +++--- django/db/models/fields/__init__.py | 9 ++--- django/db/models/fields/related.py | 30 ++++++++-------- django/db/models/functions/datetime.py | 7 ++-- django/db/models/indexes.py | 9 ++--- django/db/models/query.py | 34 ++++++++++++------- django/db/models/sql/query.py | 12 +++---- tests/bulk_create/tests.py | 5 +++ tests/dates/tests.py | 5 +-- tests/datetimes/tests.py | 13 +++++++ .../datetime/test_extract_trunc.py | 3 +- tests/delete/tests.py | 6 ++++ tests/distinct_on_fields/tests.py | 9 +++-- tests/invalid_models_tests/test_models.py | 12 +++++++ tests/model_fields/test_foreignkey.py | 9 +++++ tests/model_fields/test_manytomanyfield.py | 28 +++++++++++++++ tests/model_indexes/tests.py | 7 ++-- tests/queries/tests.py | 18 ++++++---- tests/schema/fields.py | 6 ++-- tests/validation/models.py | 15 -------- 20 files changed, 168 insertions(+), 79 deletions(-) diff --git a/django/db/models/base.py b/django/db/models/base.py index b1202f51c75..f208067aae1 100644 --- a/django/db/models/base.py +++ b/django/db/models/base.py @@ -947,12 +947,12 @@ class Model(metaclass=ModelBase): field.delete_cached_value(self) def delete(self, using=None, keep_parents=False): + if self.pk is None: + raise ValueError( + "%s object can't be deleted because its %s attribute is set " + "to None." % (self._meta.object_name, self._meta.pk.attname) + ) using = using or router.db_for_write(self.__class__, instance=self) - assert self.pk is not None, ( - "%s object can't be deleted because its %s attribute is set to None." % - (self._meta.object_name, self._meta.pk.attname) - ) - collector = Collector(using=using) collector.collect([self], keep_parents=keep_parents) return collector.delete() diff --git a/django/db/models/fields/__init__.py b/django/db/models/fields/__init__.py index 84c81f9e218..7198d430cea 100644 --- a/django/db/models/fields/__init__.py +++ b/django/db/models/fields/__init__.py @@ -2484,10 +2484,11 @@ class AutoFieldMixin: return value def contribute_to_class(self, cls, name, **kwargs): - assert not cls._meta.auto_field, ( - "Model %s can't have more than one auto-generated field." - % cls._meta.label - ) + if cls._meta.auto_field: + raise ValueError( + "Model %s can't have more than one auto-generated field." + % cls._meta.label + ) super().contribute_to_class(cls, name, **kwargs) cls._meta.auto_field = self diff --git a/django/db/models/fields/related.py b/django/db/models/fields/related.py index a0e8da10fd8..8f2a367d655 100644 --- a/django/db/models/fields/related.py +++ b/django/db/models/fields/related.py @@ -815,13 +815,13 @@ class ForeignKey(ForeignObject): try: to._meta.model_name except AttributeError: - assert isinstance(to, str), ( - "%s(%r) is invalid. First parameter to ForeignKey must be " - "either a model, a model name, or the string %r" % ( - self.__class__.__name__, to, - RECURSIVE_RELATIONSHIP_CONSTANT, + if not isinstance(to, str): + raise TypeError( + '%s(%r) is invalid. First parameter to ForeignKey must be ' + 'either a model, a model name, or the string %r' % ( + self.__class__.__name__, to, RECURSIVE_RELATIONSHIP_CONSTANT, + ) ) - ) else: # For backwards compatibility purposes, we need to *try* and set # the to_field during FK construction. It won't be guaranteed to @@ -1169,18 +1169,20 @@ class ManyToManyField(RelatedField): try: to._meta except AttributeError: - assert isinstance(to, str), ( - "%s(%r) is invalid. First parameter to ManyToManyField must be " - "either a model, a model name, or the string %r" % - (self.__class__.__name__, to, RECURSIVE_RELATIONSHIP_CONSTANT) - ) + if not isinstance(to, str): + raise TypeError( + '%s(%r) is invalid. First parameter to ManyToManyField ' + 'must be either a model, a model name, or the string %r' % ( + self.__class__.__name__, to, RECURSIVE_RELATIONSHIP_CONSTANT, + ) + ) if symmetrical is None: symmetrical = (to == RECURSIVE_RELATIONSHIP_CONSTANT) - if through is not None: - assert db_table is None, ( - "Cannot specify a db_table if an intermediary model is used." + if through is not None and db_table is not None: + raise ValueError( + 'Cannot specify a db_table if an intermediary model is used.' ) kwargs['rel'] = self.rel_class( diff --git a/django/db/models/functions/datetime.py b/django/db/models/functions/datetime.py index 7fc49897b51..20161bef382 100644 --- a/django/db/models/functions/datetime.py +++ b/django/db/models/functions/datetime.py @@ -214,9 +214,10 @@ class TruncBase(TimezoneMixin, Transform): copy = super().resolve_expression(query, allow_joins, reuse, summarize, for_save) field = copy.lhs.output_field # DateTimeField is a subclass of DateField so this works for both. - assert isinstance(field, (DateField, TimeField)), ( - "%r isn't a DateField, TimeField, or DateTimeField." % field.name - ) + if not isinstance(field, (DateField, TimeField)): + raise TypeError( + "%r isn't a DateField, TimeField, or DateTimeField." % field.name + ) # If self.output_field was None, then accessing the field will trigger # the resolver to assign it to self.lhs.output_field. if not isinstance(copy.output_field, (DateField, DateTimeField, TimeField)): diff --git a/django/db/models/indexes.py b/django/db/models/indexes.py index cc91c704790..9c393ca2c0c 100644 --- a/django/db/models/indexes.py +++ b/django/db/models/indexes.py @@ -161,10 +161,11 @@ class Index: column_names[0][:7], '%s_%s' % (names_digest(*hash_data, length=6), self.suffix), ) - assert len(self.name) <= self.max_name_length, ( - 'Index too long for multiple database support. Is self.suffix ' - 'longer than 3 characters?' - ) + if len(self.name) > self.max_name_length: + raise ValueError( + 'Index too long for multiple database support. Is self.suffix ' + 'longer than 3 characters?' + ) if self.name[0] == '_' or self.name[0].isdigit(): self.name = 'D%s' % self.name[1:] diff --git a/django/db/models/query.py b/django/db/models/query.py index 7856cf1a9f7..71a52fb7546 100644 --- a/django/db/models/query.py +++ b/django/db/models/query.py @@ -291,10 +291,14 @@ class QuerySet: 'QuerySet indices must be integers or slices, not %s.' % type(k).__name__ ) - assert ((not isinstance(k, slice) and (k >= 0)) or - (isinstance(k, slice) and (k.start is None or k.start >= 0) and - (k.stop is None or k.stop >= 0))), \ - "Negative indexing is not supported." + if ( + (isinstance(k, int) and k < 0) or + (isinstance(k, slice) and ( + (k.start is not None and k.start < 0) or + (k.stop is not None and k.stop < 0) + )) + ): + raise ValueError('Negative indexing is not supported.') if self._result_cache is not None: return self._result_cache[k] @@ -480,7 +484,8 @@ class QuerySet: # PostgreSQL via the RETURNING ID clause. It should be possible for # Oracle as well, but the semantics for extracting the primary keys is # trickier so it's not done yet. - assert batch_size is None or batch_size > 0 + if batch_size is not None and batch_size <= 0: + raise ValueError('Batch size must be a positive integer.') # Check that the parents share the same concrete model with the our # model to detect the inheritance pattern ConcreteGrandParent -> # MultiTableParent -> ProxyChild. Simply checking self.model._meta.proxy @@ -900,10 +905,10 @@ class QuerySet: Return a list of date objects representing all available dates for the given field_name, scoped to 'kind'. """ - assert kind in ('year', 'month', 'week', 'day'), \ - "'kind' must be one of 'year', 'month', 'week', or 'day'." - assert order in ('ASC', 'DESC'), \ - "'order' must be either 'ASC' or 'DESC'." + if kind not in ('year', 'month', 'week', 'day'): + raise ValueError("'kind' must be one of 'year', 'month', 'week', or 'day'.") + if order not in ('ASC', 'DESC'): + raise ValueError("'order' must be either 'ASC' or 'DESC'.") return self.annotate( datefield=Trunc(field_name, kind, output_field=DateField()), plain_field=F(field_name) @@ -916,10 +921,13 @@ class QuerySet: Return a list of datetime objects representing all available datetimes for the given field_name, scoped to 'kind'. """ - assert kind in ('year', 'month', 'week', 'day', 'hour', 'minute', 'second'), \ - "'kind' must be one of 'year', 'month', 'week', 'day', 'hour', 'minute', or 'second'." - assert order in ('ASC', 'DESC'), \ - "'order' must be either 'ASC' or 'DESC'." + if kind not in ('year', 'month', 'week', 'day', 'hour', 'minute', 'second'): + raise ValueError( + "'kind' must be one of 'year', 'month', 'week', 'day', " + "'hour', 'minute', or 'second'." + ) + if order not in ('ASC', 'DESC'): + raise ValueError("'order' must be either 'ASC' or 'DESC'.") if settings.USE_TZ: if tzinfo is None: tzinfo = timezone.get_current_timezone() diff --git a/django/db/models/sql/query.py b/django/db/models/sql/query.py index 2412e6ad4ea..b3d92d786ce 100644 --- a/django/db/models/sql/query.py +++ b/django/db/models/sql/query.py @@ -567,14 +567,14 @@ class Query(BaseExpression): The 'connector' parameter describes how to connect filters from the 'rhs' query. """ - assert self.model == rhs.model, \ - "Cannot combine queries on two different base models." + if self.model != rhs.model: + raise TypeError('Cannot combine queries on two different base models.') if self.is_sliced: raise TypeError('Cannot combine queries once a slice has been taken.') - assert self.distinct == rhs.distinct, \ - "Cannot combine a unique query with a non-unique query." - assert self.distinct_fields == rhs.distinct_fields, \ - "Cannot combine queries with different distinct fields." + if self.distinct != rhs.distinct: + raise TypeError('Cannot combine a unique query with a non-unique query.') + if self.distinct_fields != rhs.distinct_fields: + raise TypeError('Cannot combine queries with different distinct fields.') # Work out how to relabel the rhs aliases, if necessary. change_map = {} diff --git a/tests/bulk_create/tests.py b/tests/bulk_create/tests.py index df764f945fc..2ee54c382fc 100644 --- a/tests/bulk_create/tests.py +++ b/tests/bulk_create/tests.py @@ -347,3 +347,8 @@ class BulkCreateTests(TestCase): ) with self.assertRaisesMessage(ValueError, msg): NullableFields.objects.bulk_create([NullableFields(auto_field=parent)]) + + def test_invalid_batch_size_exception(self): + msg = 'Batch size must be a positive integer.' + with self.assertRaisesMessage(ValueError, msg): + Country.objects.bulk_create([], batch_size=-1) diff --git a/tests/dates/tests.py b/tests/dates/tests.py index 82f21a26348..9f12d997e03 100644 --- a/tests/dates/tests.py +++ b/tests/dates/tests.py @@ -98,11 +98,12 @@ class DatesTests(TestCase): def test_dates_fails_when_given_invalid_kind_argument(self): msg = "'kind' must be one of 'year', 'month', 'week', or 'day'." - with self.assertRaisesMessage(AssertionError, msg): + with self.assertRaisesMessage(ValueError, msg): Article.objects.dates("pub_date", "bad_kind") def test_dates_fails_when_given_invalid_order_argument(self): - with self.assertRaisesMessage(AssertionError, "'order' must be either 'ASC' or 'DESC'."): + msg = "'order' must be either 'ASC' or 'DESC'." + with self.assertRaisesMessage(ValueError, msg): Article.objects.dates("pub_date", "year", order="bad order") @override_settings(USE_TZ=False) diff --git a/tests/datetimes/tests.py b/tests/datetimes/tests.py index f42936b5576..7f98032e0af 100644 --- a/tests/datetimes/tests.py +++ b/tests/datetimes/tests.py @@ -199,3 +199,16 @@ class DateTimesTests(TestCase): Article.objects.create(pub_date=dt, published_on=dt.date(), title="Don't put dates into datetime functions!") with self.assertRaisesMessage(ValueError, "Cannot truncate DateField 'published_on' to DateTimeField"): list(Article.objects.datetimes('published_on', 'second')) + + def test_datetimes_fails_when_given_invalid_kind_argument(self): + msg = ( + "'kind' must be one of 'year', 'month', 'week', 'day', 'hour', " + "'minute', or 'second'." + ) + with self.assertRaisesMessage(ValueError, msg): + Article.objects.datetimes('pub_date', 'bad_kind') + + def test_datetimes_fails_when_given_invalid_order_argument(self): + msg = "'order' must be either 'ASC' or 'DESC'." + with self.assertRaisesMessage(ValueError, msg): + Article.objects.datetimes('pub_date', 'year', order='bad order') diff --git a/tests/db_functions/datetime/test_extract_trunc.py b/tests/db_functions/datetime/test_extract_trunc.py index e9e069c1cf6..ced9da0c784 100644 --- a/tests/db_functions/datetime/test_extract_trunc.py +++ b/tests/db_functions/datetime/test_extract_trunc.py @@ -655,7 +655,8 @@ class DateFunctionTests(TestCase): with self.assertRaisesMessage(ValueError, msg): list(DTModel.objects.annotate(truncated=Trunc('start_datetime', 'year', output_field=IntegerField()))) - with self.assertRaisesMessage(AssertionError, "'name' isn't a DateField, TimeField, or DateTimeField."): + msg = "'name' isn't a DateField, TimeField, or DateTimeField." + with self.assertRaisesMessage(TypeError, msg): list(DTModel.objects.annotate(truncated=Trunc('name', 'year', output_field=DateTimeField()))) with self.assertRaisesMessage(ValueError, "Cannot truncate DateField 'start_date' to DateTimeField"): diff --git a/tests/delete/tests.py b/tests/delete/tests.py index bf6543d7353..d269a08b9d7 100644 --- a/tests/delete/tests.py +++ b/tests/delete/tests.py @@ -280,6 +280,12 @@ class DeletionTests(TestCase): with self.assertRaisesMessage(TypeError, msg): M.objects.all()[0:5].delete() + def test_pk_none(self): + m = M() + msg = "M object can't be deleted because its id attribute is set to None." + with self.assertRaisesMessage(ValueError, msg): + m.delete() + def test_m2m(self): m = M.objects.create() r = R.objects.create() diff --git a/tests/distinct_on_fields/tests.py b/tests/distinct_on_fields/tests.py index f87a3affb3c..5404c3eb411 100644 --- a/tests/distinct_on_fields/tests.py +++ b/tests/distinct_on_fields/tests.py @@ -87,9 +87,14 @@ class DistinctOnTests(TestCase): self.assertSequenceEqual(qset, expected) self.assertEqual(qset.count(), len(expected)) - # Combining queries with different distinct_fields is not allowed. + # Combining queries with non-unique query is not allowed. base_qs = Celebrity.objects.all() - with self.assertRaisesMessage(AssertionError, "Cannot combine queries with different distinct fields."): + msg = 'Cannot combine a unique query with a non-unique query.' + with self.assertRaisesMessage(TypeError, msg): + base_qs.distinct('id') & base_qs + # Combining queries with different distinct_fields is not allowed. + msg = 'Cannot combine queries with different distinct fields.' + with self.assertRaisesMessage(TypeError, msg): base_qs.distinct('id') & base_qs.distinct('name') # Test join unreffing diff --git a/tests/invalid_models_tests/test_models.py b/tests/invalid_models_tests/test_models.py index 66657211b28..f0984fa7592 100644 --- a/tests/invalid_models_tests/test_models.py +++ b/tests/invalid_models_tests/test_models.py @@ -1591,6 +1591,18 @@ class OtherModelTests(SimpleTestCase): ]) +class MultipleAutoFieldsTests(TestCase): + def test_multiple_autofields(self): + msg = ( + "Model invalid_models_tests.MultipleAutoFields can't have more " + "than one auto-generated field." + ) + with self.assertRaisesMessage(ValueError, msg): + class MultipleAutoFields(models.Model): + auto1 = models.AutoField(primary_key=True) + auto2 = models.AutoField(primary_key=True) + + @isolate_apps('invalid_models_tests') class JSONFieldTests(TestCase): @skipUnlessDBFeature('supports_json_field') diff --git a/tests/model_fields/test_foreignkey.py b/tests/model_fields/test_foreignkey.py index d30cca9b5cd..0cd6d62a55a 100644 --- a/tests/model_fields/test_foreignkey.py +++ b/tests/model_fields/test_foreignkey.py @@ -147,3 +147,12 @@ class ForeignKeyTests(TestCase): ) with self.assertRaisesMessage(FieldError, msg): Related._meta.get_field('child').related_fields + + def test_invalid_to_parameter(self): + msg = ( + "ForeignKey(1) is invalid. First parameter to ForeignKey must be " + "either a model, a model name, or the string 'self'" + ) + with self.assertRaisesMessage(TypeError, msg): + class MyModel(models.Model): + child = models.ForeignKey(1, models.CASCADE) diff --git a/tests/model_fields/test_manytomanyfield.py b/tests/model_fields/test_manytomanyfield.py index 5724fe9384b..0ddf57ceaf6 100644 --- a/tests/model_fields/test_manytomanyfield.py +++ b/tests/model_fields/test_manytomanyfield.py @@ -59,6 +59,34 @@ class ManyToManyFieldTests(SimpleTestCase): assert_app_model_resolved('model_fields') assert_app_model_resolved('tests') + def test_invalid_to_parameter(self): + msg = ( + "ManyToManyField(1) is invalid. First parameter to " + "ManyToManyField must be either a model, a model name, or the " + "string 'self'" + ) + with self.assertRaisesMessage(TypeError, msg): + class MyModel(models.Model): + m2m = models.ManyToManyField(1) + + @isolate_apps('model_fields') + def test_through_db_table_mutually_exclusive(self): + class Child(models.Model): + pass + + class Through(models.Model): + referred = models.ForeignKey(Child, on_delete=models.CASCADE) + referent = models.ForeignKey(Child, on_delete=models.CASCADE) + + msg = 'Cannot specify a db_table if an intermediary model is used.' + with self.assertRaisesMessage(ValueError, msg): + class MyModel(models.Model): + m2m = models.ManyToManyField( + Child, + through='Through', + db_table='custom_name', + ) + class ManyToManyFieldDBTests(TestCase): diff --git a/tests/model_indexes/tests.py b/tests/model_indexes/tests.py index 46f769cddc6..95dec7ef480 100644 --- a/tests/model_indexes/tests.py +++ b/tests/model_indexes/tests.py @@ -173,8 +173,11 @@ class SimpleIndexesTests(SimpleTestCase): # suffix can't be longer than 3 characters. long_field_index.suffix = 'suff' - msg = 'Index too long for multiple database support. Is self.suffix longer than 3 characters?' - with self.assertRaisesMessage(AssertionError, msg): + msg = ( + 'Index too long for multiple database support. Is self.suffix ' + 'longer than 3 characters?' + ) + with self.assertRaisesMessage(ValueError, msg): long_field_index.set_name_with_model(Book) @isolate_apps('model_indexes') diff --git a/tests/queries/tests.py b/tests/queries/tests.py index ac4e8849c4b..fa87e7859c1 100644 --- a/tests/queries/tests.py +++ b/tests/queries/tests.py @@ -417,9 +417,10 @@ class Queries1Tests(TestCase): def test_heterogeneous_qs_combination(self): # Combining querysets built on different models should behave in a well-defined # fashion. We raise an error. - with self.assertRaisesMessage(AssertionError, 'Cannot combine queries on two different base models.'): + msg = 'Cannot combine queries on two different base models.' + with self.assertRaisesMessage(TypeError, msg): Author.objects.all() & Tag.objects.all() - with self.assertRaisesMessage(AssertionError, 'Cannot combine queries on two different base models.'): + with self.assertRaisesMessage(TypeError, msg): Author.objects.all() | Tag.objects.all() def test_ticket3141(self): @@ -1226,10 +1227,11 @@ class Queries3Tests(TestCase): # This shouldn't create an infinite loop. self.assertQuerysetEqual(Valid.objects.all(), []) - def test_ticket8683(self): + def test_datetimes_invalid_field(self): # An error should be raised when QuerySet.datetimes() is passed the # wrong type of field. - with self.assertRaisesMessage(AssertionError, "'name' isn't a DateField, TimeField, or DateTimeField."): + msg = "'name' isn't a DateField, TimeField, or DateTimeField." + with self.assertRaisesMessage(TypeError, msg): Item.objects.datetimes('name', 'month') def test_ticket22023(self): @@ -2405,13 +2407,17 @@ class QuerySetSupportsPythonIdioms(TestCase): def test_slicing_negative_indexing_not_supported_for_single_element(self): """hint: inverting your ordering might do what you need""" - with self.assertRaisesMessage(AssertionError, "Negative indexing is not supported."): + msg = 'Negative indexing is not supported.' + with self.assertRaisesMessage(ValueError, msg): Article.objects.all()[-1] def test_slicing_negative_indexing_not_supported_for_range(self): """hint: inverting your ordering might do what you need""" - with self.assertRaisesMessage(AssertionError, "Negative indexing is not supported."): + msg = 'Negative indexing is not supported.' + with self.assertRaisesMessage(ValueError, msg): Article.objects.all()[0:-5] + with self.assertRaisesMessage(ValueError, msg): + Article.objects.all()[-1:] def test_invalid_index(self): msg = 'QuerySet indices must be integers or slices, not str.' diff --git a/tests/schema/fields.py b/tests/schema/fields.py index aaba202364b..0e567e2d193 100644 --- a/tests/schema/fields.py +++ b/tests/schema/fields.py @@ -32,8 +32,10 @@ class CustomManyToManyField(RelatedField): ) self.swappable = swappable self.db_table = db_table - if kwargs['rel'].through is not None: - assert self.db_table is None, "Cannot specify a db_table if an intermediary model is used." + if kwargs['rel'].through is not None and self.db_table is not None: + raise ValueError( + 'Cannot specify a db_table if an intermediary model is used.' + ) super().__init__( related_name=related_name, related_query_name=related_query_name, diff --git a/tests/validation/models.py b/tests/validation/models.py index ff9aad11f05..3a5a9cd3549 100644 --- a/tests/validation/models.py +++ b/tests/validation/models.py @@ -125,18 +125,3 @@ class GenericIPAddressTestModel(models.Model): class GenericIPAddrUnpackUniqueTest(models.Model): generic_v4unpack_ip = models.GenericIPAddressField(null=True, blank=True, unique=True, unpack_ipv4=True) - - -# A model can't have multiple AutoFields -# Refs #12467. -assertion_error = None -try: - class MultipleAutoFields(models.Model): - auto1 = models.AutoField(primary_key=True) - auto2 = models.AutoField(primary_key=True) -except AssertionError as exc: - assertion_error = exc -assert str(assertion_error) == ( - "Model validation.MultipleAutoFields can't have more than one " - "auto-generated field." -)