Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#12404 closed (fixed)

Improve max_length validation

Reported by: Tim Valenta Owned by: josh
Component: Database layer (models, ORM) Version: master
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: 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 7 years ago.
Add CharField max_length validation to mange.py 'validate' command.
12404_new.diff (2.9 KB) - added by Leo Shklovskii 7 years ago.
updated patch with tests

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by Oroku Saki

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 7 years ago by Luke Plant

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 7 years ago by josh

Attachment: 12404_patch.diff added

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

comment:3 Changed 7 years ago by josh

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

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

Needs tests: set
Triage Stage: UnreviewedAccepted

comment:5 Changed 7 years ago by James Bennett

milestone: 1.2

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

comment:6 in reply to:  5 Changed 7 years ago by Tim Valenta

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

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.

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

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

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 7 years ago by Leo Shklovskii

Attachment: 12404_new.diff added

updated patch with tests

comment:10 Changed 7 years ago by Leo Shklovskii

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

Triage Stage: AcceptedReady for checkin

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

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

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

milestone: 1.2

Milestone 1.2 deleted

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