RawSQL ignores decimal_places property of DecimalField

[alakae]

When updating Django in our environment from 2.0.6 to 2.1.7 we have noticed that RawSQL does not take into account the decimal_places property of a DecimalField passed as output_field. Please consider the following example:

def currency_field(*, decimal_places=2, **kwargs):
    return models.DecimalField(max_digits=19, decimal_places=decimal_places, **kwargs)


class A(models.Model):
    x = currency_field()


def test_demo():
    A.objects.create(x=Decimal('1.3333333333333'))
    raw_sql = RawSQL(2.3333333333333, (), currency_field())

    qs = A.objects.all()
    res = qs.annotate(val=raw_sql).get()
    assert res.x == Decimal('1.33')
    assert res.val == Decimal('2.33')

On Django 2.0.6 the test passes. When using Django 2.1.7 the following error is returned:

>       assert res.val == Decimal('2.33')
E       AssertionError: assert Decimal('2.33333333333330') == Decimal('2.33')
E        +  where Decimal('2.33333333333330') = <A: A object (1)>.val
E        +  and   Decimal('2.33') = Decimal('2.33')

Since this change does not seem to be mentioned in ​https://docs.djangoproject.com/en/2.1/releases/2.1/ we suspect that this might be a bug. I am using Python 3.6.5 and sqlite 3.27.1.

Related to: https://code.djangoproject.com/ticket/23941

[Carlton Gibson]

Bisected to ebc4ee3369694e6dca5cf216d4176bdefd930fd6

Refs #23941 -- Prevented incorrect rounding of DecimalField annotations on SQLite.

[Carlton Gibson]

So there's an easy-enough adjustment for the test case, at the point where the behaviour changed, adjusting convert_decimalfield_value() like so:

    def convert_decimalfield_value(self, value, expression, connection):
        if value is not None:
            if not isinstance(expression, Col):
                # SQLite stores only 15 significant digits. Digits coming from
                # float inaccuracy must be removed.
                value = decimal.Context(prec=15).create_decimal_from_float(value)      # This `returns` in actual code.
            value = expression.output_field.format_number(value)
            return decimal.Decimal(value)
        return value

Here we continue to pass value through the output field's format_number(), as before the change.

Doing so, though, introduces two failures elsewhere in the test suite:

======================================================================
ERROR: test_decimal_max_digits_has_no_effect (aggregation.tests.AggregateTestCase)
----------------------------------------------------------------------
...
decimal.InvalidOperation: [<class 'decimal.InvalidOperation'>]

======================================================================
FAIL: test_decimal_annotation (annotations.tests.NonAggregateAnnotationTestCase)
----------------------------------------------------------------------
...
AssertionError: Decimal('0.00') != Decimal('0.001')

The first of these was introduced in the main patch for #23941 "Removed implicit decimal formatting from expressions."

Looking at that commit message and the discussion on #23941, it seems the behaviour here has explicitly unsupported since Django 1.8.

In particular, comment 5:

The trickier case is where an explicit output_field was given. I hear Anssi's argument that we should respect the max_digits and decimal_places in that case, otherwise we are "lying" to the user, but I'm not convinced. Why should DecimalField be special? If you specify a CharField as output_field, its max_length will not be respected in converting from db to Python. If you specify a PositiveIntegerField as output_field for an expression, you can get a negative result with no error, etc. Conversion is not the same thing as validation.

It seems to me that the idea of applying field validation to the output of an expression would be a major new feature addition that would need quite a lot of separate discussion to figure out how it would work, or if its even a good idea (there is currently no such thing as getting ValidationError from attempting to do a database query in Django). In the meantime, it's simply a bug (and a regression) that such validation is sort-of applied in the case of DecimalField, and the correct fix is to just stop doing that, as the current pull request does.

I think we have to say this is wontfix on that basis. Happy though if you want to go to the DevelopersMailingList about that.

(Note in master the relevant code has moved to ​`get_decimalfield_converter()`.)