Skip to content

Loading…

[PATCH] issue #13844 "Errors when using character fields for aggregation" #67

Closed
wants to merge 2 commits into from

2 participants

@valhallasw

https://code.djangoproject.com/ticket/13844

This patch solves the issue by using Field.to_python() to do conversion, instead of having dispatching logic in the database operations layer.

50edfe6 adds a regression test which specifically addresses #13844;
b0bdaf4 removes the conversion logic from BaseDatabaseOperations, dispatching it to Field.to_python() instead.

valhallasw added some commits
@valhallasw valhallasw Added regression test for #13844
This test acts as close to the problem as possible - by running
convert_values directly. Note that this test does not trigger under
sqlite, as the sqlite layer overrides convert_value behavior.
50edfe6
@valhallasw valhallasw Fix for #13844: convert using Field.to_python
Instead of adding another option in convert_value, I think it is
more sensible (and certainly more friendly to third party Fields)
to use functionality in Fields instead of in the database layer.

This version passes all current aggregation and aggregation_regress
tests under both sqlite and postgresql. Note that sqlite currently
overrides convert_value, which means it does *not* use to_python
as fields define them.
b0bdaf4
@akaariai
Django member

I am closing this pull request. I am proposing using akaariai@fe1e4f4 instead. The test is same, but no use of to_python, just remove the float(value) from the end of the method.

@akaariai akaariai closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 16, 2012
  1. @valhallasw

    Added regression test for #13844

    valhallasw committed
    This test acts as close to the problem as possible - by running
    convert_values directly. Note that this test does not trigger under
    sqlite, as the sqlite layer overrides convert_value behavior.
  2. @valhallasw

    Fix for #13844: convert using Field.to_python

    valhallasw committed
    Instead of adding another option in convert_value, I think it is
    more sensible (and certainly more friendly to third party Fields)
    to use functionality in Fields instead of in the database layer.
    
    This version passes all current aggregation and aggregation_regress
    tests under both sqlite and postgresql. Note that sqlite currently
    overrides convert_value, which means it does *not* use to_python
    as fields define them.
Showing with 14 additions and 10 deletions.
  1. +1 −10 django/db/backends/__init__.py
  2. +13 −0 tests/regressiontests/aggregation_regress/tests.py
View
11 django/db/backends/__init__.py
@@ -844,16 +844,7 @@ def convert_values(self, value, field):
"""Coerce the value returned by the database backend into a consistent type that
is compatible with the field type.
"""
- internal_type = field.get_internal_type()
- if internal_type == 'DecimalField':
- return value
- elif internal_type and internal_type.endswith('IntegerField') or internal_type == 'AutoField':
- return int(value)
- elif internal_type in ('DateField', 'DateTimeField', 'TimeField'):
- return value
- # No field, or the field isn't known to be a decimal or integer
- # Default to a float
- return float(value)
+ return field.to_python(value)
def check_aggregate_support(self, aggregate_func):
"""Check that the backend supports the provided aggregate
View
13 tests/regressiontests/aggregation_regress/tests.py
@@ -865,3 +865,16 @@ def test_filtering_by_annotation_name(self):
['Peter Norvig'],
lambda b: b.name
)
+
+ def test_type_conversion_for_CharField_aggregations(self):
+ # regression test for #13844
+ # the database backend convert_values function should not case
+ # CharFields to float.
+ from django.db.models import CharField
+ from django.db import connection
+ testData = u'not_a_float_value'
+ testField = CharField()
+ self.assertEqual(
+ connection.ops.convert_values(testData, testField),
+ testData
+ )
Something went wrong with that request. Please try again.