[Django] #27825: ORM Object constructor does not cast types of it's attributes when called by reference
Django
by reference
---------------------------------------------+------------------------
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.10
Severity: Normal | Keywords: ORM
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27825>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Testing framework => Database layer (models, ORM)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:1>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.10
Keywords: ORM | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Oleg Belousov):
* component: Database layer (models, ORM) => Testing framework
Comment:
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?)
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:2>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.10
Keywords: ORM | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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):
def test_cast_on_save(self):
data = {'amount_cash': 12.34}
instance = Payment(**data)
instance.save()
self.assertIsInstance(instance, Decimal)
def test_truncate_on_save():
data = {'amount_cash': Decimal(12.3456)}
instance = Payment(**data)
instance.save()
self.assertEqual(5, len(str(instance.amount_cash)))
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:3>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Component: Testing framework | Version: 1.10
Keywords: ORM | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => duplicate
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:4>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Component: Testing framework | Version: 1.10
Keywords: ORM | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:5>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 1.10
Keywords: ORM | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Oleg Belousov):
* status: closed => new
* resolution: duplicate =>
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:6>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Testing framework => Database layer (models, ORM)
Comment:
Could you please explain why my analysis is incorrect or offer a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:7>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Someday/Maybe
Comment:
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 [https://groups.google.com/d/msg/django-
developers/3IX1zH09Idg/s7mSmWZhBgAJ django-developers mailing list].
Perhaps you'd like to elaborate on your ideas there.
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:8>
Django
by reference
Reporter: Oleg Belousov | Owner: nobody
Type: Bug | Status: new
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage:
| Someday/Maybe
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:9>
Django
retrieved from the database
--------------------------------------+------------------------------------
Reporter: Oleg Belousov | Owner: nobody
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Database layer (models, ORM) => Documentation
* type: Bug => Cleanup/optimization
* stage: Someday/Maybe => Accepted
Comment:
Accepting as a documentation fix per the consensus on the
[https://groups.google.com/d/topic/django-
developers/3IX1zH09Idg/discussion mailing list thread].
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:10>
Django
retrieved from the database
--------------------------------------+------------------------------------
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aarav Sharma):
Submitted PR: https://github.com/django/django/pull/#20874
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:11>
Django
retrieved from the database
--------------------------------------+------------------------------------
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by Aarav Sharma):
* cc: Aarav Sharma (added)
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:12>
Django
retrieved from the database
--------------------------------------+------------------------------------
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by AKSHAT MAKWANA):
Hi, I'd like to work on this issue. Is it okay if I submit a patch for it?
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:13>
Django
retrieved from the database
--------------------------------------+------------------------------------
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by AKSHAT MAKWANA):
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:14>
Django
retrieved from the database
--------------------------------------+------------------------------------
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Jacob Walls):
* needs_better_patch: 0 => 1
Comment:
Changes are squeezed into the middle of the section on "what happens on
save?", which doesn't seem right to me. Also, I think a single sentence
addressing why this would matter would help -- perhaps drawing from the
discussion in #35434.
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:15>
Django
retrieved from the database
--------------------------------------+------------------------------------
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* has_patch: 1 => 0
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:16>
Django
retrieved from the database
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by somi):
* owner: nobody => somi
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:17>
Django
retrieved from the database
--------------------------------------+------------------------------------
Reporter: Oleg Belousov | Owner: somi
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by somi):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:18>
Django
retrieved from the database
--------------------------------------+------------------------------------
Reporter: Oleg Belousov | Owner: somi
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by somi):
[https://github.com/django/django/pull/21174 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:19>
Django
retrieved from the database
--------------------------------------+------------------------------------
Reporter: Oleg Belousov | Owner: somi
Type: Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.10
Severity: Normal | Resolution:
Keywords: ORM | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Changes (by Jacob Walls):
--
Ticket URL: <https://code.djangoproject.com/ticket/27825#comment:20>