Opened 16 years ago
Closed 13 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)
Change History (23)
by , 16 years ago
Attachment: | db_model_fields.patch added |
---|
comment:1 by , 16 years ago
Has patch: | set |
---|
comment:2 by , 16 years ago
milestone: | post-1.0 |
---|
comment:3 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
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 , 16 years ago
Yes good point.
Adding a patch for the forms/models.py and an updated test case
comment:5 by , 16 years ago
db_model_fields.patch is no no longer valid and has been superseded by models.patch.py
comment:6 by , 16 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 , 16 years ago
comment:8 by , 15 years ago
Cc: | added |
---|
comment:9 by , 15 years ago
Summary: | ModelChoiceField fails when passed a non integer string → ModelChoiceField 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 , 15 years ago
Cc: | added |
---|
by , 14 years ago
Attachment: | django-modelchoicefields-invalid-value.patch added |
---|
comment:11 by , 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 , 14 years ago
Cc: | added |
---|
comment:13 by , 14 years ago
Cc: | added |
---|
comment:14 by , 14 years ago
Cc: | added |
---|
comment:15 by , 14 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:17 by , 13 years ago
Type: | Uncategorized → Bug |
---|
comment:18 by , 13 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
UI/UX: | unset |
This is obviously fixed now. If not, reopen with a specific test case.
Patch to catch errors when converting to an int