#27979 closed Cleanup/optimization (fixed)
Using F() to save negative integers in unsigned columns on MySQL should raise IntegrityError rather than OperationalError
Reported by: | Chris Dary | Owned by: | nobody |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 1.10 |
Severity: | Normal | Keywords: | db, mysql, exceptions, OperationalError |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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:
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).
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 '(`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!
Attachments (1)
Change History (11)
comment:1 by , 8 years ago
Description: | modified (diff) |
---|
comment:2 by , 8 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Summary: | Bug: Using F() with mysql can leak an unwrapped OperationalError → Using F() with mysql can leak an unwrapped OperationalError |
Type: | Uncategorized → Bug |
comment:3 by , 8 years ago
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.
(Just to be perfectly explicit, I'd guess this would be wrapped as an agnostic OperationalError)
Last edit: In case it's helpful, I'm using mysqlclient 1.3.10 as my MySQL client library for python 3 (which is recommended here: https://docs.djangoproject.com/en/1.10/ref/databases/#id9)
comment:4 by , 8 years ago
Summary: | Using F() with mysql can leak an unwrapped OperationalError → Using F() to save negative integers in unsigned columns on MySQL should raise IntegrityError rather than OperationalError |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
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.
by , 8 years ago
Attachment: | 27979.diff added |
---|
comment:5 by , 8 years ago
Has patch: | set |
---|
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.
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.
comment:6 by , 8 years ago
Patch needs improvement: | set |
---|
comment:8 by , 7 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
What do you mean by "a wrapped exception"? Are you thinking it would be reraised as
IntegrityError
similar to 90279aeaf07222dd7388956bfd465d30365087a5?