Opened 3 years ago
Closed 3 years ago
#34370 closed Cleanup/optimization (fixed)
IntegerField validators don't work if the database is SQLite, leading to overflow errors
| Reported by: | Alex Urbanowicz | Owned by: | Mohamed Nabil Rady |
|---|---|---|---|
| Component: | Database layer (models, ORM) | Version: | 4.1 |
| Severity: | Normal | Keywords: | IntegerField SQLite MaxValueValidator |
| Cc: | Simon Charette, Sarah Boyce | 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
The documentation for IntegerField states that:
It uses MinValueValidator and MaxValueValidator to validate the input based on the values that the default database supports.
The PositiveIntegerField states that:
"Like an IntegerField, but must be either positive or zero (0). Values from 0 to 2147483647 are safe in all databases supported by Django. The value 0 is accepted for backward compatibility reasons."
So it is simple to assume that PositiveIntergerField uses MaxValueValidator that applies the limit of the default database.
Yet it is possible, while using SQLite to attempt to store the value of positive integer not supported by the DB, which leads to OverflowError:
Python int too large to convert to SQLite INTEGER
So it is either the IntegerField's MaxValueValidator is not applied, contrary to what the documentation suggests, or the documentation has an error for lack of specifying that you must use an explicit validator to validate to the safe value given (why not default then?).
In my opinion it should be limited to the safe value by default, or have the MaxValueValidator check it by default against the database limit.
Should I prepare the patch to limit it to the safe value?
This is also related to #27397.
Change History (9)
comment:1 by , 3 years ago
| Cc: | added |
|---|---|
| Easy pickings: | unset |
comment:2 by , 3 years ago
Not opposed to adding the value to integer_field_range for SQLite. It should be mentioned in the release notes though as it's slightly backward incompatible as users of SQLite were previously able to store overflowing values (e.g. 64 bit integers in 32 bit columns) without issues.
This issue is really just #27397 on save instead of reads because sqlite3 raises an OverflowError on when dealing with longs that overflow 64 bits.
In order to properly address #27397 by raising (Empty|Full)ResultSet when provided overflowing values we need integer_field_range to be defined anyway. Once it's the case we could adapt lookups such as Exact, GreaterThan(Eq)? to special case literal right-hand-sides that overflow their left hand side ranges to raise EmptyResultSet and the LowerThen(Eq)? to raise FullResultSet and not only address #27397 for SQLite but deal protect against a form of DDoS vector on PostgreSQL that is trivial to exploit (think of any view that accepts \d+ or <int:pk> to perform a Model.objects.get(pk=pk)).
comment:3 by , 3 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:4 by , 3 years ago
| Summary: | PositiveIntegerField validators don't work if the database is SQLite, leading to overflow errors → IntegerField validators don't work if the database is SQLite, leading to overflow errors |
|---|---|
| Triage Stage: | Unreviewed → Accepted |
| Type: | Bug → Cleanup/optimization |
Thanks Simon, let's treat this as the first step to fix #27397.
comment:5 by , 3 years ago
| Has patch: | set |
|---|
I removed the sqlite implementation of integer_field_range() so it uses the base implementation and I added a test, hope this PR is helpful.
comment:6 by , 3 years ago
| Needs documentation: | set |
|---|---|
| Needs tests: | set |
| Patch needs improvement: | set |
comment:7 by , 3 years ago
| Cc: | added |
|---|---|
| Needs tests: | unset |
| Patch needs improvement: | unset |
comment:8 by , 3 years ago
| Needs documentation: | unset |
|---|---|
| Triage Stage: | Accepted → Ready for checkin |
Thanks for the report.
SQLite support values up to
2 ** 63 - 1we could add limits to theDatabaseOperations.integer_field_range()on SQLite. What do you think Simon?