PositiveIntegerField in model form does not validate max value

[paul@…]

Entering in a value of more than 4294967295 for a PositiveIntegerField in a model form causes an error to be thrown trying to insert the value into the database. Ideally this would get validated by the model form.

[Russell Keith-Magee]

I don't think it's as simple as checking against 2^{32-1; it will depend on how the backend defines PositiveIntegerField. It's also something that we can now integrate into the model validators.}

[elbarto]

Added new tests.

[Julien Phalip]

As pointed out by Russell, the maximum number would need to be determined by each backend -- not hardcoded in the PositiveInteger class.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[amcharg@…]

How would the following be handled?

if form.is_valid():
   instance = form.save(commit=False)
   instance.save(using='nowforsomethingcompletelydifferent')

I can't think of a generic way solve this. However I am working on a patch that consults the backend's features to get the correct min and max value.

[elbarto]

I tried to change the owner but I don't know how to do that.

Please, reassign it.

[Aymeric Augustin]

Yes, there's a small glitch in Trac since we simplified the workflow. You'll have to use the "assign to myself" option, save changes, and then that option will turn into "unassign". Sorry for the inconvenience.

[elbarto]

Thanks for the info aagustin!

[Tim Graham]

#22143 was a duplicate and has a ​pull request.

[Claude Paroz]

The pull request completely ignores the backend-specific limits, so I think it doesn't resolve anything, unfortunately.

[Simon Charette]

I went ahead and gathered data about backend-specific limits based on datatypes used.

Field	Documented range	PostgreSQL	SQLite3*	MySQL	Oracle†
SmallIntegerField	[-32768; 32767]	As documented	No limits	As documented	[-99999999999; 99999999999]
IntegerField	[-2147483648; 2147483647]	As documented	No limits	As documented	[-99999999999; 99999999999]
BigIntegerField	[-9223372036854775808; 9223372036854775807]	As documented	No limits	As documented	[-9999999999999999999; 9999999999999999999]
PositiveSmallIntegerField	[0; 32767]	As documented	No limits	[0; 4294967295]	[0; 99999999999]
PositiveIntegerField	[0; 2147483647]	As documented	No limits	[0; 18446744073709551615]	[0; 99999999999]

* Based on local testing SQLite3 doesn't enforce any limits.
† I also assumed Oracle limits based on ​documented values of NUMBER(11) and NUMBER(19).

What if we added a BaseDatabaseOperations.field_integer_range(self, internal_type) that returns a tuple of the (min_value, max_value) form?

This method could then be called from django.db.fields.IntegerField.__init__ (or stored in a cached_property to avoid accessing django.db.connection at import time) in order to pass along default values when formfield is called. Database field level validation could also be added at this point.

[Claude Paroz]

Thanks Simon for this research. Very nice.

However, as shown in comment:6, the database is not necessarily known at the time of the form instanciation. Considering that probably 98% of Django sites use only one database (and 99.5% only one database engine simultaneously), we could just skip the check if more than one database engine is in use.

[dimyur27@…]

I'm no expert, but what if it is not a Field itself should bother about the limits?
I mean the formfields returned by modelfields are not aware of the limits.
But ModelForm maybe can find out with which backend it has to interact and set the limits of its integer fields.
It is simple when 'instance' kwarg is provided. Anyway ModelForm knows which backend will be used when save() is called, except something interesting happens between form.is_valid() and form.save().
So is_valid() may update the limits before validating.
In that case first time rendered html will lack HTML5 attributes.
And it is intuitive that is_valid just do checks and does not alter the state.
So my scenario is rather weird than useful, but maybe it makes a little sense.

[Anssi Kääriäinen]

Skipping the check if more than one engine is in use seems good to me. Especially so if we go with only one database engine in use rule. Alternatively we could just collapse the limits of all DB engines in use by doing intersection of all the limit ranges. But even that seems a bit complicated for this problem.

[Simon Charette]

Replying to claudep:

Thanks Simon for this research. Very nice.

However, as shown in comment:6, the database is not necessarily known at the time of the form instanciation. Considering that probably 98% of Django sites use only one database (and 99.5% only one database engine simultaneously), we could just skip the check if more than one database engine is in use.

I'd add that of this 0.5% using many database engine simultaneously not many of them are retrieving an object from an engine and saving to another. IMHO comment:6 is really an edge case and I assume many other things might also break since constraints checks are all ran against the wrong database here (foreign keys, uniques, ...).

Just to be safe we could add a database aware check in django.db.models.fields.IntegerField.validate based on model_instance._state.db or router.db_for_write(model_instance.__class__, instance=model_instance) by referring directly to connections[db].ops.field_integer_range(self.get_internal_type()).

Both limits intersection and skips of formfield default kwargs are suitable since the validation should be enforced at the model level anyway.

[Shai Berger]

I agree with @charettes' last statement. This should be in the model validation, not form.

[Simon Charette]

Created a PR that assumes only one database engine: ​https://github.com/django/django/pull/2464

It was locally tested against (Py2, Py3) X (SQLite3, PostgreSQL, MySQL) (only missing Oracle).

It's missing a release note since we might want to merge this bugfix into the 1.7.x instead?

[Simon Charette <charette.s@…>]

In 1506c71a95cd7f58fbc6363edf2ef742c58d2487:

Fixed #12030 -- Validate integer field range at the model level.

Thanks to @timgraham for the review.

[Simon Charette <charette.s@…>]

In 78211b13a51f2ada1c7beb81ff97ef11f9e239eb:

[1.7.x] Fixed #12030 -- Validate integer field range at the model level.

Thanks to @timgraham for the review.

Backport of 1506c71a95 from master

[Tim Graham <timograham@…>]

In ff874f363ce6556577f03909b15c71c051d5b617:

Fixed field deconstruction tests failures introduced by 1506c71a95.

refs #12030.

[Simon Charette <charette.s@…>]

In 81d3d48b5ed841414c1d91743aea23dab36c8b4b:

[1.7.x] Fixed field deconstruction tests failures introduced by 1506c71a95.

refs #12030.

Backport of ff874f363c from master

[Tim Graham]

I guess a circular import may be causing ​the PostGIS test failure.

[Simon Charette]

Opened ​another PR addressing the circular import.

[Simon Charette <charette.s@…>]

In b9e50e47742f67dcf3c37aad989b350e8255154e:

Fixed the PostGIS circular imports caused by 1506c71a95.

Thanks to @loic for the help and @timgraham for the review.

refs #12030.

[Simon Charette <charette.s@…>]

In 4678efd3f1e2f764583a59b9c1f4302d955acebb:

[1.7.x] Fixed the PostGIS circular imports caused by 1506c71a95.

Thanks to @loic for the help and @timgraham for the review.

refs #12030.

Backport of b9e50e4774 from master