Document that models don't cast field values to the same type that's retrieved from the database

[Oleg Belousov]

My experience is that when using the Constructor (ORM) class by references with Django 1.10.5
There might be some inconsistencies in the data (i.e. the attributes of the created object may get the type of the input data instead of the casted type of the ORM object property)

Example:

models

 class Payment(models.Model):
     amount_cash = models.DecimalField()

some_test.py - object.create

Class SomeTestCase: 
     def generate_orm_obj(self, _constructor, base_data=None, modifiers=None):
           objs = [] 
          if not base_data: 
               base_data = {'amount_case': 123.00} 
               for modifier in modifiers: 
                    actual_data = deepcopy(base_data) 
                    actual_data.update(modifier) 
                     _obj = _constructor.objects.create(**actual_data) 
                    print(type(_obj.amount_cash)) # Decimal                                                                       
               return objs

some_test.py - Constructor()

Class SomeTestCase: 
    def generate_orm_obj(self, _constructor, base_data=None, modifiers=None): 
                objs = [] 
                if not base_data:
                     base_data = {'amount_case': 123.00}
                     for modifier in modifiers: 
                        actual_data = deepcopy(base_data) 
                        actual_data.update(modifier) 
                         _obj = _constructor(**actual_data)
                        print(type(_obj.amount_cash)) # Float 
                    objs.append(_obj) return objs

[Tim Graham]

Can you simplify the description at all? I'm not sure all the deepcopy stuff is required to explain the issue. A failing test case would be more clear than an annotated function.

This might be related to, or a duplicate of, #24028.

[Oleg Belousov]

Update:
The problem is actually that the type is not being casted on 'save', and you have to call 'refresh_from_db' to accomplish that (is that a bug, or is that by design?)

[Oleg Belousov]

Failing TestCase:

class Payment(models.Model):
     amount_cash = models.DecimalField(max_digits=6, decimal_places=2)

from decimal import Decimal


class TestSave(unittest.TestCase):
    # I am failing
    def test_cast_on_save(self):
        data = {'amount_cash': 12.34}
        instance = Payment(**data)
        instance.save()
        self.assertIsInstance(instance, Decimal)
    
   # I am failing
   def test_truncate_on_save():
        data = {'amount_cash': Decimal(12.3456)}
        instance = Payment(**data)
        instance.save()
        self.assertEqual(5, len(str(instance.amount_cash)))
    
      # I am passing, but I am using `refresh_from_db` redundantly
     def test_cast_on_save(self):
        data = {'amount_cash': 12.34}
        instance = Payment(**data)
        instance.save()
        self.assertIsInstance(instance, Decimal)

   # I am passing, but I am using `refresh_from_db` redundantly
   def test_truncate_on_save():
        data = {'amount_cash': Decimal(12.3456)}
        instance = Payment(**data)
        instance.save()
        instance.refresh_from_db()
        self.assertEqual(5, len(str(instance.amount_cash)))

[Tim Graham]

Your proposal is a duplicate of the proposal in #24028: "a flag to Model.save which updates field attributes to the result of Field.to_python(value)". As I said in that ticket, we could document the current behavior if there's no consensus to change it. I think making this casting behavior mandatory would probably have unacceptable performance consequences for little benefit considering that it's a reasonable expectation for developers to provide the correct type.

[Oleg Belousov]

Hi @TimGraham,
You are wrong, this is not a duplicate.
And it is only a basic expectation that the DB layer and the Application layer will correspond to each-other after performing save, which is in other words, syncing your state with the DB.
Personally, this bug (one way binding between application and db on save) broke many of my tests and took a lot of my time.

[Tim Graham]

Could you please explain why my analysis is incorrect or offer a patch?

[Tim Graham]

You can add in a model.full_clean() if you want to coerce the values to the correct type.

I raised the topic on the ​django-developers mailing list. Perhaps you'd like to elaborate on your ideas there.

[Josh Smeaton]

The original wording of this post is confusing, because it *seems* to suggest that using Model.objects.create(decimal_field=123.12) will do the expected thing and type cast to a Decimal, but it does not.

class Costing(models.Model):
    cost = models.DecimalField(max_digits=20, decimal_places=2)
    rev = models.DecimalField(max_digits=20, decimal_places=2)
    ts = models.DateField(db_index=True)

In [31]: c = Costing.objects.create(cost=Decimal('123.12'), rev=123.12, ts=datetime.now().date())

In [32]: print(type(c.rev))
<type 'float'>

In [33]: c.rev
Out[33]: 123.12

While this might look different to #24028 I think it is the same. In the linked ticket, the related field has been cached and isn't reloaded and brought through the various adapters and converters. Here, it's simply the local field that hasn't been reloaded and gone through various transformations.

The only fix that will work for every field is to refetch it from the database, and rely on the adapters provided by the driver, plus the converters provided by the backend and fields, to cast the type correctly. That's a non-starter, because we'd need to do a SELECT for every INSERT or UPDATE, which harms performance for all the users handing the constructor correct data.

You could argue that save could accept a flag to do an automatic fetch, but this wouldn't help you in your situation. It's unlikely that users would pass in such a flag just in case bad data got in - they'd only use it when they *know* they need to refresh from the database, like when using expressions to update attributes. At that point, using refresh_from_db is nearly as good.

Personally, this bug (one way binding between application and db on save) broke many of my tests and took a lot of my time.

The first bug was in your own program. Handing a float to a decimal field is wrong. Let me show you a contrived example:

# Make the decimal field store a large decimal component

class Costing(models.Model):
    cost = models.DecimalField(max_digits=20, decimal_places=2)
    rev = models.DecimalField(max_digits=30, decimal_places=18)
    ts = models.DateField(db_index=True)

In [3]: c = Costing.objects.create(cost=Decimal('123.12'), rev=0.1 + 0.1 + 0.1, ts=datetime.now().date())

In [4]: c.refresh_from_db()

In [5]: c.rev
Out[5]: Decimal('0.300000000000000044')

That said, the behaviour of django models here is certainly surprising which is not a good thing. Decimal fields are among the worst culprits, because they'll silently accept and save wrong data. I'd be more inclined to accept a patch enforcing only decimals being assigned to decimal fields (or having the descriptor wrap any arguments in a Decimal(), which would be a breaking change for a lot of people, but would highlight a number of broken programs. Digressing a little, I'm also surprised the decimal type in python itself allows floats as arguments, because the same broken behaviour is possible there.

one way binding between application and db on save

We need to make this clearer in our docs I think. Without doing a full refresh of the object, it's impossible to convert input to the eventual output for every field type.

[Tim Graham]

Accepting as a documentation fix per the consensus on the ​mailing list thread.