[Django] #27979: Bug: Using F() with mysql can leak an unwrapped OperationalError
Django
-------------------------------------+-------------------------------------
Reporter: Chris | Owner: nobody
Dary |
Type: | Status: new
Uncategorized |
Component: | Version: 1.10
Uncategorized | Keywords: db, mysql,
Severity: Normal | exceptions, OperationalError
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Hey folks - I'm using MySQL 5.7, python 3.6, Django 1.10.
I just ran into this one: It looks like with a PositiveIntegerField it's
possible to get django to leak a raw OperationalError from mysql rather
than a wrapped one as it's intended if you try to set it below zero. I
assume this is a specific case of a more general issue.
Here's a sample model:
{{{#!python
from django.db import models
class SampleModel(models.Model):
num = models.PositiveIntegerField()
}}}
And here's some example code showing it leaking a mysql exception which is
not trappable by the traditional django.db exceptions (in shell_plus to
show the queries).
{{{#!python
In [1]: import sys
...: from django.db import models
...:
In [2]: sample = SampleModel.objects.create(num=0)
...:
INSERT INTO `users_samplemodel` (`num`)
VALUES (0)
Execution time: 0.000492s [Database: default]
In [3]: try:
...: sample.num = models.F('num') - 1
...: sample.save()
...: except:
...: err_type, error, traceback = sys.exc_info()
...: print("Caught Exception of type: %s" % err_type)
...: print("Error: %s" % error)
...:
UPDATE `users_samplemodel`
SET `num` = (`users_samplemodel`.`num` - 1)
WHERE `users_samplemodel`.`id` = 2
Execution time: 0.000409s [Database: default]
Caught Exception of type: <class '_mysql_exceptions.OperationalError'>
Error: (1690, "BIGINT UNSIGNED value is out of range in
'(`limbo`.`users_samplemodel`.`num` - 1)'")
}}}
Looks like this just needs to be trapped and wrapped somewhere?
I was able to work around it just by using a transaction and checking
exists() first, but thought it'd be helpful to report.
Thanks for all your work on Django!
--
Ticket URL: <https://code.djangoproject.com/ticket/27979>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.10
Severity: Normal | Resolution:
Keywords: db, mysql, | Triage Stage:
exceptions, OperationalError | Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
'(`sample`.`users_samplemodel`.`num` - 1)'")
}}}
Looks like this just needs to be trapped and wrapped somewhere?
I was able to work around it just by using a transaction and checking
exists() first, but thought it'd be helpful to report.
Thanks for all your work on Django!
--
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db, mysql, | Triage Stage:
exceptions, OperationalError | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Uncategorized => Bug
* component: Uncategorized => Database layer (models, ORM)
Comment:
What do you mean by "a wrapped exception"? Are you thinking it would be
reraised as `IntegrityError` similar to
90279aeaf07222dd7388956bfd465d30365087a5?
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: Bug | Status: new
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db, mysql, | Triage Stage:
exceptions, OperationalError | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Chris Dary):
Yep - as mentioned in the docs here:
https://docs.djangoproject.com/en/1.10/ref/exceptions/#database-exceptions
It's nice to have Django wrap DB specific exceptions so that you have an
agnostic interface to them and don't have to catch engine specific
exceptions. Based on the phrasing there ("guaranteed common
implementation"), I presume it's a bug when one of these engine specific
exceptions leaks.
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:3>
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
exceptions, OperationalError |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
I can't reproduce the unwrapped exception (I'm seeing
`django.db.utils.OperationalError: (1690, "BIGINT UNSIGNED value is out of
range in '(`test_django`.`model_fields_positiveintegermodel`.`value` -
1)'")` for the attached test before the fix is applied), however, to be
consistent with the behavior of other databases, I think this should raise
`IntegrityError` rather than `OperationalError`. The attached patch needs
to skip the test on databases that don't enforce the positive constraint
(SQLite) as well as on MySQL, if strict mode is disabled.
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:4>
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db, mysql, | Triage Stage: Accepted
exceptions, OperationalError |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* Attachment "27979.diff" added.
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db, mysql, | Triage Stage: Accepted
exceptions, OperationalError |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Cool, that works for me - I can confirm that this patch fixes my issue! I
also appreciate the comment denoting what the error message related is.
{{{#!python
In [6]: import sys
In [7]: from django.db import models
In [8]: sample = SampleModel.objects.create(num=0)
INSERT INTO `payments_samplemodel` (`num`)
VALUES (0)
Execution time: 0.006077s [Database: default]
In [9]: try:
...: sample.num = models.F('num') - 1
...: sample.save()
...: except:
...: err_type, error, traceback = sys.exc_info()
...: print("Caught Exception of type: %s" % err_type)
...: print("Error: %s" % error)
...:
UPDATE `payments_samplemodel`
SET `num` = (`payments_samplemodel`.`num` - 1)
WHERE `payments_samplemodel`.`id` = 1
Execution time: 0.007049s [Database: default]
Caught Exception of type: <class 'django.db.utils.IntegrityError'>
Error: (1690, "BIGINT UNSIGNED value is out of range in
'(`sample`.`payments_samplemodel`.`num` - 1)'")
}}}
Thanks for that, Tim. Looks good to me.
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:5>
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db, mysql, | Triage Stage: Accepted
exceptions, OperationalError |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:6>
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
Keywords: db, mysql, | Triage Stage: Accepted
exceptions, OperationalError |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
Comment:
[https://github.com/django/django/pull/9172 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:7>
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution:
exceptions, OperationalError | checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:8>
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Keywords: db, mysql, | Triage Stage: Ready for
exceptions, OperationalError | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"dd82f3327124fd2762cf6df2ac8c6380772bf127" dd82f332]:
{{{
#!CommitTicketReference repository=""
revision="dd82f3327124fd2762cf6df2ac8c6380772bf127"
Fixed #27979 -- Made MySQL raise IntegrityError rather than
OperationalError when saving negative numbers in PositiveInteger fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:9>
Django
raise IntegrityError rather than OperationalError
-------------------------------------+-------------------------------------
Reporter: Chris Dary | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: 1.10
(models, ORM) |
Severity: Normal | Resolution: fixed
Keywords: db, mysql, | Triage Stage: Ready for
exceptions, OperationalError | checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"d6cec5f6ffd1b5dc906a85ecbb0ca8d11e6d494d" d6cec5f]:
{{{
#!CommitTicketReference repository=""
revision="d6cec5f6ffd1b5dc906a85ecbb0ca8d11e6d494d"
[2.0.x] Fixed #27979 -- Made MySQL raise IntegrityError rather than
OperationalError when saving negative numbers in PositiveInteger fields.
Backport of dd82f3327124fd2762cf6df2ac8c6380772bf127 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27979#comment:10>