Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#12404 closed (fixed)

Improve max_length validation

Reported by: Tim Valenta Owned by: josh
Component: Database layer (models, ORM) Version: dev
Severity: Keywords: max_length, validation
Cc: Leo Shklovskii 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

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 14 years ago.
Add CharField max_length validation to mange.py 'validate' command.
12404_new.diff (2.9 KB ) - added by Leo Shklovskii 14 years ago.
updated patch with tests

Download all attachments as: .zip

Change History (16)

comment:1 by Oroku Saki, 14 years ago

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?

in reply to:  1 comment:2 by Luke Plant, 14 years ago

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.

by josh, 14 years ago

Attachment: 12404_patch.diff added

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

comment:3 by josh, 14 years ago

Has patch: set
Owner: changed from nobody to josh
Status: newassigned

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

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:5 by James Bennett, 14 years ago

milestone: 1.2

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

in reply to:  5 comment:6 by Tim Valenta, 14 years ago

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 by Russell Keith-Magee, 14 years ago

milestone: 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.

in reply to:  7 comment:8 by Tim Valenta, 14 years ago

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 by Russell Keith-Magee, 14 years ago

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.

by Leo Shklovskii, 14 years ago

Attachment: 12404_new.diff added

updated patch with tests

comment:10 by Leo Shklovskii, 14 years ago

Cc: Leo Shklovskii added
Needs tests: unset

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

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

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

(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 by Russell Keith-Magee, 14 years ago

(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 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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