Opened 14 years ago

Closed 10 years ago

Last modified 10 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: dev
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 13 years ago.
new_tests.diff (928 bytes ) - added by elbarto 13 years ago.
Added new tests.

Download all attachments as: .zip

Change History (31)

comment:1 by Russell Keith-Magee, 14 years ago

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 by elbarto, 13 years ago

Owner: changed from nobody to elbarto

comment:3 by elbarto, 13 years ago

Has patch: set

by elbarto, 13 years ago

Attachment: positive_integer.diff added

by elbarto, 13 years ago

Attachment: new_tests.diff added

Added new tests.

comment:4 by Julien Phalip, 13 years ago

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 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:6 by amcharg@…, 11 years ago

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 by elbarto, 11 years ago

Cc: aliaselbarto@… added

comment:8 by elbarto, 11 years ago

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

Please, reassign it.

comment:9 by Aymeric Augustin, 11 years ago

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 by elbarto, 11 years ago

Status: newassigned

Thanks for the info aagustin!

comment:11 by elbarto, 11 years ago

Owner: elbarto removed
Status: assignednew

comment:12 by Tim Graham, 10 years ago

#22143 was a duplicate and has a pull request.

comment:13 by Claude Paroz, 10 years ago

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

comment:14 by Simon Charette, 10 years ago

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 by Claude Paroz, 10 years ago

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 by dimyur27@…, 10 years ago

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 by Anssi Kääriäinen, 10 years ago

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.

in reply to:  15 comment:18 by Simon Charette, 10 years ago

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 by Shai Berger, 10 years ago

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

comment:20 by Simon Charette, 10 years ago

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 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady for checkin

comment:22 by Simon Charette <charette.s@…>, 10 years ago

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 by Simon Charette <charette.s@…>, 10 years ago

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 by Tim Graham <timograham@…>, 10 years ago

In ff874f363ce6556577f03909b15c71c051d5b617:

Fixed field deconstruction tests failures introduced by 1506c71a95.

refs #12030.

comment:25 by Simon Charette <charette.s@…>, 10 years ago

In 81d3d48b5ed841414c1d91743aea23dab36c8b4b:

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

refs #12030.

Backport of ff874f363c from master

comment:26 by Tim Graham, 10 years ago

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

comment:27 by Simon Charette, 10 years ago

Opened another PR addressing the circular import.

comment:28 by Simon Charette <charette.s@…>, 10 years ago

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 by Simon Charette <charette.s@…>, 10 years ago

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