Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#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 Chris Dary)

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)

27979.diff (1.5 KB ) - added by Tim Graham 8 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by Chris Dary, 8 years ago

Description: modified (diff)

comment:2 by Tim Graham, 8 years ago

Component: UncategorizedDatabase layer (models, ORM)
Summary: Bug: Using F() with mysql can leak an unwrapped OperationalErrorUsing F() with mysql can leak an unwrapped OperationalError
Type: UncategorizedBug

What do you mean by "a wrapped exception"? Are you thinking it would be reraised as IntegrityError similar to 90279aeaf07222dd7388956bfd465d30365087a5?

comment:3 by Chris Dary, 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.

Version 0, edited 8 years ago by Chris Dary (next)

comment:4 by Tim Graham, 8 years ago

Summary: Using F() with mysql can leak an unwrapped OperationalErrorUsing F() to save negative integers in unsigned columns on MySQL should raise IntegrityError rather than OperationalError
Triage Stage: UnreviewedAccepted
Type: BugCleanup/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 Tim Graham, 8 years ago

Attachment: 27979.diff added

comment:5 by Chris Dary, 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 Tim Graham, 8 years ago

Patch needs improvement: set

comment:7 by Tim Graham, 7 years ago

Patch needs improvement: unset

comment:8 by Simon Charette, 7 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: newclosed

In dd82f332:

Fixed #27979 -- Made MySQL raise IntegrityError rather than OperationalError when saving negative numbers in PositiveInteger fields.

comment:10 by Tim Graham <timograham@…>, 7 years ago

In d6cec5f:

[2.0.x] Fixed #27979 -- Made MySQL raise IntegrityError rather than OperationalError when saving negative numbers in PositiveInteger fields.

Backport of dd82f3327124fd2762cf6df2ac8c6380772bf127 from master

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