From 4a66a69239c493c05b322815b18c605cd4c96e7c Mon Sep 17 00:00:00 2001 From: Greg Chapple Date: Thu, 4 Jun 2015 16:25:38 +0100 Subject: [PATCH] Fixed #24887 -- Removed one-arg limit from models.aggregate --- django/contrib/gis/db/models/aggregates.py | 5 +++-- django/db/backends/sqlite3/operations.py | 24 ++++++++++++---------- django/db/models/aggregates.py | 17 ++++++++------- tests/aggregation/tests.py | 24 +++++++++++++++++++++- 4 files changed, 49 insertions(+), 21 deletions(-) diff --git a/django/contrib/gis/db/models/aggregates.py b/django/contrib/gis/db/models/aggregates.py index 7458b2bdeb..bdcd05aa0e 100644 --- a/django/contrib/gis/db/models/aggregates.py +++ b/django/contrib/gis/db/models/aggregates.py @@ -25,8 +25,9 @@ class GeoAggregate(Aggregate): def resolve_expression(self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False): c = super(GeoAggregate, self).resolve_expression(query, allow_joins, reuse, summarize, for_save) - if not hasattr(c.input_field.field, 'geom_type'): - raise ValueError('Geospatial aggregates only allowed on geometry fields.') + for expr in c.get_source_expressions(): + if not hasattr(expr.field, 'geom_type'): + raise ValueError('Geospatial aggregates only allowed on geometry fields.') return c def convert_value(self, value, expression, connection, context): diff --git a/django/db/backends/sqlite3/operations.py b/django/db/backends/sqlite3/operations.py index f363556ced..895252ef9c 100644 --- a/django/db/backends/sqlite3/operations.py +++ b/django/db/backends/sqlite3/operations.py @@ -35,17 +35,19 @@ class DatabaseOperations(BaseDatabaseOperations): bad_fields = (fields.DateField, fields.DateTimeField, fields.TimeField) bad_aggregates = (aggregates.Sum, aggregates.Avg, aggregates.Variance, aggregates.StdDev) if isinstance(expression, bad_aggregates): - try: - output_field = expression.input_field.output_field - if isinstance(output_field, bad_fields): - raise NotImplementedError( - 'You cannot use Sum, Avg, StdDev and Variance aggregations ' - 'on date/time fields in sqlite3 ' - 'since date/time is saved as text.') - except FieldError: - # not every sub-expression has an output_field which is fine to - # ignore - pass + for expr in expression.get_source_expressions(): + try: + output_field = expr.output_field + if isinstance(output_field, bad_fields): + raise NotImplementedError( + 'You cannot use Sum, Avg, StdDev, and Variance ' + 'aggregations on date/time fields in sqlite3 ' + 'since date/time is saved as text.' + ) + except FieldError: + # Not every subexpression has an output_field which is fine + # to ignore. + pass def date_extract_sql(self, lookup_type, field_name): # sqlite doesn't support extract, so we fake it with the user-defined diff --git a/django/db/models/aggregates.py b/django/db/models/aggregates.py index 81e37e8dde..9e1a0b370b 100644 --- a/django/db/models/aggregates.py +++ b/django/db/models/aggregates.py @@ -15,13 +15,15 @@ class Aggregate(Func): name = None def resolve_expression(self, query=None, allow_joins=True, reuse=None, summarize=False, for_save=False): - assert len(self.source_expressions) == 1 # Aggregates are not allowed in UPDATE queries, so ignore for_save c = super(Aggregate, self).resolve_expression(query, allow_joins, reuse, summarize) - if c.source_expressions[0].contains_aggregate and not summarize: - name = self.source_expressions[0].name - raise FieldError("Cannot compute %s('%s'): '%s' is an aggregate" % ( - c.name, name, name)) + if not summarize: + expressions = c.get_source_expressions() + for index, expr in enumerate(expressions): + if expr.contains_aggregate: + before_resolved = self.get_source_expressions()[index] + name = before_resolved.name if hasattr(before_resolved, 'name') else repr(before_resolved) + raise FieldError("Cannot compute %s('%s'): '%s' is an aggregate" % (c.name, name, name)) c._patch_aggregate(query) # backward-compatibility support return c @@ -31,8 +33,9 @@ class Aggregate(Func): @property def default_alias(self): - if hasattr(self.source_expressions[0], 'name'): - return '%s__%s' % (self.source_expressions[0].name, self.name.lower()) + expressions = self.get_source_expressions() + if len(expressions) == 1 and hasattr(expressions[0], 'name'): + return '%s__%s' % (expressions[0].name, self.name.lower()) raise TypeError("Complex expressions require an alias") def get_group_by_cols(self): diff --git a/tests/aggregation/tests.py b/tests/aggregation/tests.py index 5b0bcb82bc..10f1275cf6 100644 --- a/tests/aggregation/tests.py +++ b/tests/aggregation/tests.py @@ -985,9 +985,31 @@ class AggregateTestCase(TestCase): self.assertEqual(author.sum_age, other_author.sum_age) def test_annotated_aggregate_over_annotated_aggregate(self): - with six.assertRaisesRegex(self, FieldError, "Cannot compute Sum\('id__max'\): 'id__max' is an aggregate"): + with self.assertRaisesMessage(FieldError, "Cannot compute Sum('id__max'): 'id__max' is an aggregate"): Book.objects.annotate(Max('id')).annotate(Sum('id__max')) + class MyMax(Max): + def as_sql(self, compiler, connection): + self.set_source_expressions(self.get_source_expressions()[0:1]) + return super(MyMax, self).as_sql(compiler, connection) + + with self.assertRaisesMessage(FieldError, "Cannot compute Max('id__max'): 'id__max' is an aggregate"): + Book.objects.annotate(Max('id')).annotate(my_max=MyMax('id__max', 'price')) + + def test_multi_arg_aggregate(self): + class MyMax(Max): + def as_sql(self, compiler, connection): + self.set_source_expressions(self.get_source_expressions()[0:1]) + return super(MyMax, self).as_sql(compiler, connection) + + with self.assertRaisesMessage(TypeError, 'Complex aggregates require an alias'): + Book.objects.aggregate(MyMax('pages', 'price')) + + with self.assertRaisesMessage(TypeError, 'Complex annotations require an alias'): + Book.objects.annotate(MyMax('pages', 'price')) + + Book.objects.aggregate(max_field=MyMax('pages', 'price')) + def test_add_implementation(self): class MySum(Sum): pass