Opened 6 years ago

Closed 14 months ago

Last modified 14 months 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@…, charettes 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 4 years ago.
new_tests.diff (928 bytes) - added by elbarto 4 years ago.
Added new tests.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 5 years ago by russellm

  • Component changed from Forms to Database layer (models, ORM)
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

  • Owner changed from nobody to elbarto

comment:3 Changed 4 years ago by elbarto

  • Has patch set

Changed 4 years ago by elbarto

Changed 4 years ago by elbarto

Added new tests.

comment:4 Changed 4 years ago by julien

  • Easy pickings unset
  • Patch needs improvement set
  • Severity set to Normal
  • Type set to 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 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:6 Changed 21 months 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 21 months ago by elbarto

  • Cc aliaselbarto@… added

comment:8 Changed 21 months ago by elbarto

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

Please, reassign it.

comment:9 Changed 21 months ago by aaugustin

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 21 months ago by elbarto

  • Status changed from new to assigned

Thanks for the info aagustin!

comment:11 Changed 21 months ago by elbarto

  • Owner elbarto deleted
  • Status changed from assigned to new

comment:12 Changed 15 months ago by timo

#22143 was a duplicate and has a pull request.

comment:13 Changed 15 months ago by claudep

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

comment:14 Changed 15 months ago by charettes

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 follow-up: Changed 15 months ago by 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.

comment:16 Changed 15 months 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 15 months ago by akaariai

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 15 months ago by charettes

  • Cc charettes added
  • Version changed from 1.1 to master

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 15 months ago by shai

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

comment:20 Changed 14 months ago by charettes

  • 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 14 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:22 Changed 14 months ago by Simon Charette <charette.s@…>

  • Owner set to Simon Charette <charette.s@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 1506c71a95cd7f58fbc6363edf2ef742c58d2487:

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

Thanks to @timgraham for the review.

comment:23 Changed 14 months 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 14 months ago by Tim Graham <timograham@…>

In ff874f363ce6556577f03909b15c71c051d5b617:

Fixed field deconstruction tests failures introduced by 1506c71a95.

refs #12030.

comment:25 Changed 14 months 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 14 months ago by timo

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

comment:27 Changed 14 months ago by charettes

Opened another PR addressing the circular import.

comment:28 Changed 14 months 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 14 months 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