Opened 14 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)
Change History (17)
comment:1 by , 14 years ago
comment:2 by , 14 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
comment:3 by , 14 years ago
Resolution: | → needsinfo |
---|---|
Status: | new → closed |
I'm almost positive this is MySQL being MySQL. If you can reproduce with another database please feel free to reopen.
comment:4 by , 14 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 , 14 years ago
Resolution: | needsinfo |
---|---|
Status: | closed → reopened |
comment:6 by , 14 years ago
Changed to reopened, to find out whether we should be protecting against this anyway??
comment:7 by , 14 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: | Unreviewed → 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 by , 14 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 , 14 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 , 14 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → 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 by , 14 years ago
Resolution: | wontfix |
---|---|
Status: | closed → 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 by , 14 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → 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 by , 14 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 , 14 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
comment:15 by , 13 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
UI/UX: | unset |
Version: | 1.3 → 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.)
by , 13 years ago
Attachment: | too-large-integers.diff added |
---|
comment:16 by , 13 years ago
Resolution: | → wontfix |
---|---|
Status: | reopened → 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.
Since this could be database dependent, could you mention which database you are using (software, version, 32/64bits)? Thanks!