Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#12030 closed Bug (fixed)

PositiveIntegerField in model form does not validate max value

Reported by: paul@… Owned by: Simon Charette <charette.s@…>
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords:
Cc: aliaselbarto@…, Simon Charette 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

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.

Attachments (2)

positive_integer.diff (3.7 KB) - added by elbarto 6 years ago.
new_tests.diff (928 bytes) - added by elbarto 6 years ago.
Added new tests.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 7 years ago by Russell Keith-Magee

Component: FormsDatabase layer (models, ORM)
Triage Stage: UnreviewedAccepted

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

comment:2 Changed 6 years ago by elbarto

Owner: changed from nobody to elbarto

comment:3 Changed 6 years ago by elbarto

Has patch: set

Changed 6 years ago by elbarto

Attachment: positive_integer.diff added

Changed 6 years ago by elbarto

Attachment: new_tests.diff added

Added new tests.

comment:4 Changed 6 years ago by Julien Phalip

Easy pickings: unset
Patch needs improvement: set
Severity: Normal
Type: Bug

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

comment:5 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 Changed 3 years ago by 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.

comment:7 Changed 3 years ago by elbarto

Cc: aliaselbarto@… added

comment:8 Changed 3 years ago by elbarto

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

Please, reassign it.

comment:9 Changed 3 years ago by 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.

comment:10 Changed 3 years ago by elbarto

Status: newassigned

Thanks for the info aagustin!

comment:11 Changed 3 years ago by elbarto

Owner: elbarto deleted
Status: assignednew

comment:12 Changed 3 years ago by Tim Graham

#22143 was a duplicate and has a pull request.

comment:13 Changed 3 years ago by Claude Paroz

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

comment:14 Changed 3 years ago by 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.

comment:15 Changed 3 years ago by 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.

comment:16 Changed 3 years ago by 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.

comment:17 Changed 3 years ago by 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.

comment:18 in reply to:  15 Changed 3 years ago by Simon Charette

Cc: Simon Charette added
Version: 1.1master

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.

comment:19 Changed 3 years ago by Shai Berger

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

comment:20 Changed 3 years ago by Simon Charette

Patch needs improvement: unset

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?

comment:21 Changed 3 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:22 Changed 3 years ago by Simon Charette <charette.s@…>

Owner: set to Simon Charette <charette.s@…>
Resolution: fixed
Status: newclosed

In 1506c71a95cd7f58fbc6363edf2ef742c58d2487:

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

Thanks to @timgraham for the review.

comment:23 Changed 3 years ago by 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

comment:24 Changed 3 years ago by Tim Graham <timograham@…>

In ff874f363ce6556577f03909b15c71c051d5b617:

Fixed field deconstruction tests failures introduced by 1506c71a95.

refs #12030.

comment:25 Changed 3 years ago by Simon Charette <charette.s@…>

In 81d3d48b5ed841414c1d91743aea23dab36c8b4b:

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

refs #12030.

Backport of ff874f363c from master

comment:26 Changed 3 years ago by Tim Graham

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

comment:27 Changed 3 years ago by Simon Charette

Opened another PR addressing the circular import.

comment:28 Changed 3 years ago by 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.

comment:29 Changed 3 years ago by 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

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