Opened 8 years ago

Closed 5 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 8 years ago.
Patch to catch errors when converting to an int
models.patch.py (990 bytes) - added by Rozza 8 years ago.
forms/models.py patch
models.py (1.1 KB) - added by Rozza 8 years ago.
Test to display the bug
9209.diff (3.9 KB) - added by Karen Tracey 8 years ago.
django-modelchoicefields-invalid-value.patch (2.5 KB) - added by hgeerts@… 6 years ago.

Download all attachments as: .zip

Change History (23)

Changed 8 years ago by Rozza

Attachment: db_model_fields.patch added

Patch to catch errors when converting to an int

comment:1 Changed 8 years ago by Rozza

Has patch: set

comment:2 Changed 8 years ago by anonymous

milestone: post-1.0

comment:3 Changed 8 years ago by Malcolm Tredinnick

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 Changed 8 years ago by Rozza

Yes good point.

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

Changed 8 years ago by Rozza

Attachment: models.patch.py added

forms/models.py patch

Changed 8 years ago by Rozza

Attachment: models.py added

Test to display the bug

comment:5 Changed 8 years ago by Rozza

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

comment:6 Changed 8 years ago by Karen Tracey

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?

Changed 8 years ago by Karen Tracey

Attachment: 9209.diff added

comment:7 Changed 8 years ago by Karen Tracey

#8974 and #9938 closed as dupes of this.

comment:8 Changed 7 years ago by Gábor Farkas

Cc: gabor@… added

comment:9 Changed 7 years ago by mauve

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 Changed 7 years ago by Ben Firshman

Cc: ben@… added

Changed 6 years ago by hgeerts@…

comment:11 Changed 6 years ago by hgeerts@…

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 Changed 6 years ago by Vlada Macek

Cc: t.django@… added

comment:13 Changed 6 years ago by Andrey Golovizin

Cc: Andrey Golovizin added

comment:14 Changed 6 years ago by Roman Barczyński

Cc: Roman Barczyński added

comment:15 Changed 6 years ago by patchhammer

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 Changed 6 years ago by Harm Geerts <hgeerts@…>

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

comment:17 Changed 6 years ago by Julien Phalip

Type: UncategorizedBug

comment:18 Changed 5 years ago by Claude Paroz

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