Opened 15 years ago

Closed 12 years ago

#9209 closed Bug (fixed)

ModelChoiceField raises ValueError when passed a non integer string

Reported by: Rozza Owned by: nobody
Component: Forms Version: 1.0
Severity: Normal Keywords:
Cc: gabor@…, ben@…, t.django@…, Andrey Golovizin, Roman Barczyński Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The ModelChoiceField fails when invalid data is passed to a form i.e. something that cant be converted to an integer

Patch incoming and so is a test :D

Rozza

Attachments (5)

db_model_fields.patch (471 bytes ) - added by Rozza 15 years ago.
Patch to catch errors when converting to an int
models.patch.py (990 bytes ) - added by Rozza 15 years ago.
forms/models.py patch
models.py (1.1 KB ) - added by Rozza 15 years ago.
Test to display the bug
9209.diff (3.9 KB ) - added by Karen Tracey 15 years ago.
django-modelchoicefields-invalid-value.patch (2.5 KB ) - added by hgeerts@… 14 years ago.

Download all attachments as: .zip

Change History (23)

by Rozza, 15 years ago

Attachment: db_model_fields.patch added

Patch to catch errors when converting to an int

comment:1 by Rozza, 15 years ago

Has patch: set

comment:2 by anonymous, 15 years ago

milestone: post-1.0

comment:3 by Malcolm Tredinnick, 15 years ago

Triage Stage: UnreviewedAccepted

The first patch isn't really the right place to be addressing this problem. It's a form validation issue, so changing what happens when saving to the database is only disguising the real problem.

comment:4 by Rozza, 15 years ago

Yes good point.

Adding a patch for the forms/models.py and an updated test case

by Rozza, 15 years ago

Attachment: models.patch.py added

forms/models.py patch

by Rozza, 15 years ago

Attachment: models.py added

Test to display the bug

comment:5 by Rozza, 15 years ago

db_model_fields.patch is no no longer valid and has been superseded by models.patch.py

comment:6 by Karen Tracey, 15 years ago

First, when creating a diff for patches please do so from the the top-level trunk directory, not from within the django subdirectory, so it's crystal clear where things go. Also, please don't name diff files with a .py extension since trac then won't display them as diffs -- .diff is the recommended suffix so trac can do its syntax highlighting thing. And finally, tests that integrate with the existing test suite are more helpful than standalone ones that have to then be fitted in somewhere.

That all said, unfortunately the 2nd approach doesn't work either, since it requires that pk values be integers, which is not always true. One can use explicit primary keys that are character fields, or floats, or ... etc. so the field clean method can't require that they be integers. A way to fix this (and deal with the fact that primary keys may be of varied types, and the form input may fail to conform to that type), is to change the except: self.queryset.model.DoesNotExist after the self.queryset.get to a bare except. Not sure how acceptable that is as a solution so I'll upload a patch with that and some integrated tests for this case and see what others think.

I will note that I think the situation here is similar to the one the admin code has to deal with in processing lookup parameters (see #9252). I did think about narrowing the bare except used in that code in r9246, but ultimately did not. The prospect of adding tests for the cross-product of field types and possible inputs that could be supplied was more than I wanted to take on...I did test enough random combinations to come to the conclusion that there was not a single exception raised in the case of input that failed to conform to type. For this ticket, we'd have to add at least ValueError and ValidationError to the except to cover the cases I tested ad-hoc for #9252...but there easily could be others I didn't run across, so I'm tending to think the bare except is safer. Unless there's some other approach I'm missing?

by Karen Tracey, 15 years ago

Attachment: 9209.diff added

comment:7 by Karen Tracey, 15 years ago

#8974 and #9938 closed as dupes of this.

comment:8 by Gábor Farkas, 15 years ago

Cc: gabor@… added

comment:9 by mauve, 14 years ago

Summary: ModelChoiceField fails when passed a non integer stringModelChoiceField raises ValueError when passed a non integer string

Marked #12154 as dupe of this.

This occurs in a user-facing situation when using raw_id_fields in the admin; whereas the normal SelectWidget generally won't supply an invalid ID unless a user is doing something funny, ForeignKeyRawIdWidget happily passes whatever is typed in.

Trunk now seems to have a fix in place for ModelMultipleChoiceField.

comment:10 by Ben Firshman, 14 years ago

Cc: ben@… added

comment:11 by hgeerts@…, 14 years ago

A bare except will mask programming errors with an invalid to_field on a ModelChoiceField.
A different approach would be to use the model field to validate the value before passing it to the queryset.
Note that I do not use field.validate on purpose since it performs database lookups by itself.

comment:12 by Vlada Macek, 13 years ago

Cc: t.django@… added

comment:13 by Andrey Golovizin, 13 years ago

Cc: Andrey Golovizin added

comment:14 by Roman Barczyński, 13 years ago

Cc: Roman Barczyński added

comment:15 by patchhammer, 13 years ago

Easy pickings: unset
Patch needs improvement: set
Severity: Normal
Type: Uncategorized

django-modelchoicefields-invalid-value.patch fails to apply cleanly on to trunk

comment:16 by Harm Geerts <hgeerts@…>, 13 years ago

Can you confirm this issues has been fixed in #13149 ?

comment:17 by Julien Phalip, 13 years ago

Type: UncategorizedBug

comment:18 by Claude Paroz, 12 years ago

Resolution: fixed
Status: newclosed
UI/UX: unset

This is obviously fixed now. If not, reopen with a specific test case.

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