Code

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#12404 closed (fixed)

Improve max_length validation

Reported by: tiliv Owned by: josh
Component: Database layer (models, ORM) Version: master
Severity: Keywords: max_length, validation
Cc: Leo Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Absentmindedly I wrote a line like the following in a model, where I accidentally contracted the definition of max_length and help_text into a single keyword arg:

    field = models.CharField(max_length="This field is really cool. Not to mention this message.")

My accidental value for max_length is not being validated to check and see if it is indeed an integer for use in a MySQL varchar field definition. The SQL created during ./manage.py syncdb is as follows, which causes the error also following:

    `field` varchar(This field is really cool. Not to mention this message.) NOT NULL
    _mysql_exceptions.ProgrammingError: (1064, "You have an error in your SQL syntax;
    check the manual that corresponds to your MySQL server version for the right
    syntax to use near 'This field is really cool. Not to mention this message.) NOT NULL\n)'
    at line 4")

Improved validation might simply try casting the value of max_length to an integer before proceeding.

Attachments (2)

12404_patch.diff (1.5 KB) - added by josh 4 years ago.
Add CharField max_length validation to mange.py 'validate' command.
12404_new.diff (2.9 KB) - added by Leo 4 years ago.
updated patch with tests

Download all attachments as: .zip

Change History (16)

comment:1 follow-up: Changed 4 years ago by orokusaki

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

If I have a max_length=5 in a Field, and I use firebug to disable the HTML maxlength='5' attribute on the page, and enter 10 characters, then hit save, it'll give me a "KeyError" of some sort, instead of using application level validation to see if the text is indeed 5 characters long. You shouldn't have a server error just because some nerd with Firebug hacked your page, right?

comment:2 in reply to: ↑ 1 Changed 4 years ago by lukeplant

Replying to orokusaki:

If I have a max_length=5 in a Field, and I use firebug to disable the HTML maxlength='5' attribute on the page, and enter 10 characters, then hit save, it'll give me a "KeyError" of some sort, instead of using application level validation to see if the text is indeed 5 characters long.

FieldTests.test_charfield_2 in tests/regressiontests/forms/fields.py indicates that you are incorrect about that issue, or you are not using the application level validation that is available. This bug is about another issue anyway.

Changed 4 years ago by josh

Add CharField max_length validation to mange.py 'validate' command.

comment:3 Changed 4 years ago by josh

  • Has patch set
  • Owner changed from nobody to josh
  • Status changed from new to assigned

comment:4 Changed 4 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

comment:5 follow-up: Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

1.2 is feature-frozen, moving this feature request off the milestone.

comment:6 in reply to: ↑ 5 Changed 4 years ago by tiliv

Replying to ubernostrum:

1.2 is feature-frozen, moving this feature request off the milestone.

Funny, I see this as a bug, not a feature addition. But okay. I submitted it a while back for this very reason :)

comment:7 follow-up: Changed 4 years ago by russellm

  • milestone set to 1.2

I agree - this is a bug that should be fixed. Chalk this one up to friendly fire.

Patch still requires tests. See the invalid_models regression test for how to do it.

comment:8 in reply to: ↑ 7 Changed 4 years ago by tiliv

Replying to russellm:

Patch still requires tests. See the invalid_models regression test for how to do it.

For my own future reference, does the fact that there's an assigned user suggest who the author of the test should be? I've kept my distance tentatively since it was picked up by someone the day after I submitted it. I haven't seen direct reference to such a question in the documentation on contributing, unless I missed that somehow.

comment:9 Changed 4 years ago by russellm

Ultimately, we'll accept a patch from whoever submits it, regardless of whether they have officially claimed the ticket.

The ticket assignment just exists as an attempt to minimize duplicated effort. During a sprint, assigning a bug helps ensure that multiple people aren't working on the same issue; on larger features, it announces that someone is working on the problem.

@josh claimed this ticket over a month ago, and hasn't updated the patch since. Given that this is a small bug with a simple fix, it's probably safe to assume he isn't actively working on it.

The documentation (such as it is) for this process is here.

Changed 4 years ago by Leo

updated patch with tests

comment:10 Changed 4 years ago by Leo

  • Cc Leo added
  • Needs tests unset

Updated patch with tests. I added a check for wrong type argument as well as invalid int.

comment:11 Changed 4 years ago by russellm

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 4 years ago by russellm

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [12768]) Fixed #12404 -- Improved model validation for CharField and DecimalField. Thanks to tiliv for the report, josh for the patch, and Leo for the tests.

comment:13 Changed 4 years ago by russellm

(In [12769]) [1.1.X] Fixed #12404 -- Improved model validation for CharField and DecimalField. Thanks to tiliv for the report, josh for the patch, and Leo for the tests.

Backport of r12768 from trunk.

comment:14 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.