ModelChoiceField raises ValueError when passed a non integer string

[Rozza]

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

[Rozza]

Patch to catch errors when converting to an int

[Malcolm Tredinnick]

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.

[Rozza]

Yes good point.

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

[Rozza]

forms/models.py patch

[Rozza]

Test to display the bug

[Rozza]

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

[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?

[Karen Tracey]

#8974 and #9938 closed as dupes of this.

[mauve]

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.

[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.

[patchhammer]

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

[Harm Geerts <hgeerts@…>]

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

[Claude Paroz]

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