#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 Mariusz Felisiak, 14 months ago

Cc: Simon Charette added
Easy pickings: unset

Thanks for the report.

SQLite support values up to 2 ** 63 - 1 we could add limits to the DatabaseOperations.integer_field_range() on SQLite. What do you think Simon?

comment:2 by Simon Charette, 14 months 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)) since we don't use server bound variables.

Last edited 14 months ago by Simon Charette (previous) (diff)

comment:3 by Mohamed Nabil Rady, 14 months ago

Owner: changed from nobody to Mohamed Nabil Rady
Status: newassigned

comment:4 by Mariusz Felisiak, 14 months ago

Summary: PositiveIntegerField validators don't work if the database is SQLite, leading to overflow errorsIntegerField validators don't work if the database is SQLite, leading to overflow errors
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Thanks Simon, let's treat this as the first step to fix #27397.

comment:5 by Mohamed Nabil Rady, 14 months 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 Mariusz Felisiak, 14 months ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:7 by Mariusz Felisiak, 14 months ago

Cc: Sarah Boyce added
Needs tests: unset
Patch needs improvement: unset

comment:8 by Mariusz Felisiak, 14 months ago

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 14 months ago

Resolution: fixed
Status: assignedclosed

In 32d4b61c:

Fixed #34370 -- Added integer fields validation as 64-bit on SQLite.

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