Opened 16 months ago

Last modified 16 months ago

#27825 new Cleanup/optimization

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

Reported by: Oleg Belousov Owned by: nobody
Component: Documentation Version: 1.10
Severity: Normal Keywords: ORM
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (10)

comment:1 Changed 16 months ago by Tim Graham

Component: Testing frameworkDatabase layer (models, ORM)

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. I'm not sure if this behavior should be considered a bug though.

Last edited 16 months ago by Tim Graham (previous) (diff)

comment:2 Changed 16 months ago by Oleg Belousov

Component: Database layer (models, ORM)Testing framework

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?)

comment:3 Changed 16 months ago by 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)))
Last edited 16 months ago by Oleg Belousov (previous) (diff)

comment:4 Changed 16 months ago by Tim Graham

Resolution: duplicate
Status: newclosed

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.

comment:5 Changed 16 months ago by 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.

comment:6 Changed 16 months ago by Oleg Belousov

Resolution: duplicate
Status: closednew

comment:7 Changed 16 months ago by Tim Graham

Component: Testing frameworkDatabase layer (models, ORM)

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

comment:8 Changed 16 months ago by Tim Graham

Triage Stage: UnreviewedSomeday/Maybe

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.

comment:9 Changed 16 months ago by 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.

comment:10 Changed 16 months ago by Tim Graham

Component: Database layer (models, ORM)Documentation
Summary: ORM Object constructor does not cast types of it's attributes when called by referenceDocument that models don't cast field values to the same type that's retrieved from the database
Triage Stage: Someday/MaybeAccepted
Type: BugCleanup/optimization

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

Note: See TracTickets for help on using tickets.
Back to Top