Opened 13 years ago

Closed 13 years ago

#15923 closed Bug (wontfix)

Validate that IntegerField is a valid Signed value for databases that don't do it themselves.

Reported by: Cal Leeming Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hey,

So I found that, if you attempt to store a number larger than the SIGNED INT maximum, for some reason, Django (or the db) auto converts it to the maximum allowed, rather than throwing an error. This really causes some strange issues, especially when your app logic *thinks* you've managed to save the correct value lol.

Suggested patch might be, enforcing a default maximum, which throws a ValueError whenever you try saving it into the database??

If this would be an appropriate way forward, I'd be more than happy to submit a patch.

Lmk

Cal

PS) To test for yourself, try using the following:

class TestModel(models.Model):
    test_field = models.IntegerField(db_index = True)

TestModel(
    test_field = 3517601254
)

TestModel.objects.get(test_field = 3517601254)


Attachments (1)

too-large-integers.diff (3.2 KB ) - added by Petr Marhoun <petr.marhoun@…> 13 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by Aymeric Augustin, 13 years ago

Since this could be database dependent, could you mention which database you are using (software, version, 32/64bits)? Thanks!

comment:2 by Cal Leeming, 13 years ago

Sorry, I should have thought to include that!

MySQL Server Version: MySQL 5.1.50-log
MySQL Protocol: 10
Django	1.2.4
Engine: InnoDB
Server: 64bit

Also, on a side note, I tried putting a larger value into the database, and it failed with an error, rather than reducing the number to the max allowed.

Last edited 13 years ago by Cal Leeming (previous) (diff)

comment:3 by Alex Gaynor, 13 years ago

Resolution: needsinfo
Status: newclosed

I'm almost positive this is MySQL being MySQL. If you can reproduce with another database please feel free to reopen.

comment:4 by Cal Leeming, 13 years ago

mysql> INSERT INTO ddcms_sessionip (`ip`) VALUES(35176012444
);
Query OK, 1 row affected
mysql>


Lol, damn it, you're right. Man, that's absolutely retarded, why on earth would they allow that :S

Do you not think we should protect against this by adding a default min/max for unsigned/signed integer fields anyway??

Cal

comment:5 by Cal Leeming, 13 years ago

Resolution: needsinfo
Status: closedreopened

comment:6 by Cal Leeming, 13 years ago

Changed to reopened, to find out whether we should be protecting against this anyway??

comment:7 by Alex Gaynor, 13 years ago

Summary: SIGNED IntegerField down converts to highest allowed number, instead of throwing an exception.. wtf?Validate that IntegerField is a valid Signed value for databases that don't do it themselves.
Triage Stage: UnreviewedDesign decision needed

Fair point, marking as DDN because I can't decide how I feel. (Edited the title of this issue to reflect the new status).

comment:8 by Aymeric Augustin, 13 years ago

Generally Django doesn't try to prevent people from shooting themselves into the foot with MySQL.

We should check what Django does when someone tries to insert a 20 char string in a 10 char field. This is the most common data-loss bug of MySQL. The int truncation should be handled in the same way.

comment:9 by Cal Leeming, 13 years ago

Usually, when the data you are inserting, isn't supported on the database (string truncation), it just issues a Warning exception.

Personally, I think when this event is encountered, a warning should be raised (like the char fields), to say the number has been truncated to the highest available.

comment:10 by Karen Tracey, 13 years ago

Resolution: wontfix
Status: reopenedclosed

I'm a little confused by your MySQL shell snippet not showing at least a warning for a case where data has to be adjusted. When I try what you show, I get a warning, though it does not prevent a row from being saved to the DB:

mysql> insert into testme (`val`) values (35176012444);
Query OK, 1 row affected, 1 warning (0.00 sec)

mysql> show warnings;
+---------+------+-------------------------------------------------------+
| Level   | Code | Message                                               |
+---------+------+-------------------------------------------------------+
| Warning | 1264 | Out of range value adjusted for column 'val' at row 1 |
+---------+------+-------------------------------------------------------+
1 row in set (0.00 sec)

mysql> select * from testme;
+------------+
| val        |
+------------+
| 2147483647 |
+------------+
1 row in set (0.00 sec)

Note though, that this behavior is dependent on the SQL mode in effect. MySQL can be set to reject out-of-range values:

mysql> SELECT @@SESSION.sql_mode;
+--------------------+
| @@SESSION.sql_mode |
+--------------------+
|                    |
+--------------------+
1 row in set (0.00 sec)

mysql> set session sql_mode='traditional';
Query OK, 0 rows affected (0.00 sec)

mysql> SELECT @@SESSION.sql_mode;
+-------------------------------------------------------------------------------------------------------------------------------+
| @@SESSION.sql_mode                                                                                                            |
+-------------------------------------------------------------------------------------------------------------------------------+
| STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,TRADITIONAL,NO_AUTO_CREATE_USER |
+-------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)

mysql> delete from testme;
Query OK, 1 rows affected (0.00 sec)

mysql> insert into testme (`val`) values (35176012444);
ERROR 1264 (22003): Out of range value adjusted for column 'val' at row 1
mysql> select * from testme;
Empty set (0.00 sec)

Marking this wontfix -- if you want errors for this kind of situation then I think the right way to do that is to set the DB to report the error. (And you can set sql mode to be strict via server config, you do not need to do it per-session as shown above.)

comment:11 by Cal Leeming, 13 years ago

Resolution: wontfix
Status: closedreopened

If this is the case, then we need to make it easier for users to know how to set the DB to report the error. Also, we should make it default to reporting these problems, via settings.py. But, by setting SQL mode to 'traditional', what other can of worms do we potentially open? If MySQL wanted to tell us these things, why doesn't it do it by default?? I really don't think it is appropriate to just mark this as 'wontfix', without at least defaulting to the correct behaviour.

comment:12 by Aymeric Augustin, 13 years ago

Resolution: wontfix
Status: reopenedclosed

Please do not reopen tickets marked as wontfix by core developers. See the guidelines here: http://docs.djangoproject.com/en/dev/internals/contributing/#reporting-bugs (7th bullet point)

More generally, MySQL is broken in many ways, and documenting them belongs to MySQL's docs, not Django's. By the way, it's fairly clear in Django's docs that PostgreSQL is the recommended database engine.

comment:13 by Cal Leeming, 13 years ago

Sorry, I didn't realise I had to follow up 'wontfix' on django-developers. I will send a mail to that now.

comment:14 by Cal Leeming, 13 years ago

Further discussion has taken place in these two threads:

http://groups.google.com/group/django-developers/browse_thread/thread/f0b8ddbda03a2d8e
http://groups.google.com/group/django-developers/browse_thread/thread/22dd636f421823fd/a058fd6c31ee09e3

I've submitted two new tickets for this:

[Django] #15939: SignedIntegerField / UnsignedIntegerField as part of the core fields.py

[Django] #15940: Patch database documentation to explain why using sql_mode=strict is important

Last edited 13 years ago by Cal Leeming (previous) (diff)

comment:15 by Petr Marhoun <petr.marhoun@…>, 13 years ago

Resolution: wontfix
Status: closedreopened
UI/UX: unset
Version: 1.3SVN

I am not sure if I should reopen ticket closed by a core developer, especially after direct note. But I think that the resolution was based on invalid imformation - that only MySQL is broken, SQLite and recommended PostgreSQL are OK. But I think it is not true. Documentation of PostgreSQL say: "integer ---- 4 bytes ---- typical choice for integer ---- -2147483648 to +2147483647":

http://www.postgresql.org/docs/9.0/interactive/datatype-numeric.html

Moreover, BigIntegerField in Django checks interval [2 63, 2 63 - 1] so for consistency IntegerField could check interval [2 31, 2 31 -1].

I prepare patch with two tests - one (for BigIntegerField) passes on all tested databases (PostgreSQL 8.4.8, MySQL 5.1.54 with InnoDB, SQlite from Python 2.7.1 - everything on Ubuntu 11.04 64bit). The almost same second one (for IntegerField) failes on PostgreSQL with "DatabaseError: integer out of range" and on MySQL with "AssertionError" (saved number is truncated). (But I do not know if these two databases are so important that they should define interval common for all databases.)

by Petr Marhoun <petr.marhoun@…>, 13 years ago

Attachment: too-large-integers.diff added

comment:16 by Petr Marhoun <petr.marhoun@…>, 13 years ago

Resolution: wontfix
Status: reopenedclosed

Sorry, I can't read - now I see that this ticket is about hiding of an database error, not about this database error. So there is another ticket: #16747 - this one is closed again.

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