Opened 8 months ago

Closed 7 weeks ago

Last modified 7 weeks 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 months ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 months ago by Chris Dary

Description: modified (diff)

comment:2 Changed 8 months ago by Tim Graham

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 Changed 8 months ago 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.

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

Last edited 8 months ago by Chris Dary (previous) (diff)

comment:4 Changed 8 months ago by Tim Graham

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.

Changed 8 months ago by Tim Graham

Attachment: 27979.diff added

comment:5 Changed 8 months ago by Chris Dary

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 Changed 8 months ago by Tim Graham

Patch needs improvement: set

comment:7 Changed 7 weeks ago by Tim Graham

Patch needs improvement: unset

comment:8 Changed 7 weeks ago by Simon Charette

Triage Stage: AcceptedReady for checkin

comment:9 Changed 7 weeks ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In dd82f332:

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

comment:10 Changed 7 weeks ago by Tim Graham <timograham@…>

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