Code

Opened 3 years ago

Closed 3 years ago

#15923 closed Bug (wontfix)

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

Reported by: foxwhisper Owned by: nobody
Component: Database layer (models, ORM) Version: master
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@…> 3 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:2 Changed 3 years ago by foxwhisper

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 3 years ago by foxwhisper (previous) (diff)

comment:3 Changed 3 years ago by Alex

  • Resolution set to needsinfo
  • Status changed from new to closed

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

comment:4 Changed 3 years ago by foxwhisper

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 Changed 3 years ago by foxwhisper

  • Resolution needsinfo deleted
  • Status changed from closed to reopened

comment:6 Changed 3 years ago by foxwhisper

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

comment:7 Changed 3 years ago by Alex

  • Summary changed from SIGNED IntegerField down converts to highest allowed number, instead of throwing an exception.. wtf? to Validate that IntegerField is a valid Signed value for databases that don't do it themselves.
  • Triage Stage changed from Unreviewed to Design 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 Changed 3 years ago by aaugustin

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 Changed 3 years ago by foxwhisper

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 Changed 3 years ago by kmtracey

  • Resolution set to wontfix
  • Status changed from reopened to closed

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 Changed 3 years ago by foxwhisper

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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 Changed 3 years ago by aaugustin

  • Resolution set to wontfix
  • Status changed from reopened to closed

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 Changed 3 years ago by foxwhisper

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

comment:14 Changed 3 years ago by foxwhisper

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'm going to submit two feature requests (one for SignedIntegerField/UnsignedIntegerField) and one for documentation on MySQL strict mode.

Version 0, edited 3 years ago by foxwhisper (next)

comment:15 Changed 3 years ago by Petr Marhoun <petr.marhoun@…>

  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • UI/UX unset
  • Version changed from 1.3 to SVN

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

Changed 3 years ago by Petr Marhoun <petr.marhoun@…>

comment:16 Changed 3 years ago by Petr Marhoun <petr.marhoun@…>

  • Resolution set to wontfix
  • Status changed from reopened to closed

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.