Opened 7 years ago

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

Download all attachments as: .zip

Change History (23)

Changed 7 years ago by Rozza

Patch to catch errors when converting to an int

comment:1 Changed 7 years ago by Rozza

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

comment:3 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to 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 Changed 7 years ago by Rozza

Yes good point.

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

Changed 7 years ago by Rozza

forms/models.py patch

Changed 7 years ago by Rozza

Test to display the bug

comment:5 Changed 7 years ago by Rozza

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

comment:6 Changed 7 years ago by kmtracey

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

comment:7 Changed 7 years ago by kmtracey

#8974 and #9938 closed as dupes of this.

comment:8 Changed 6 years ago by gabor

  • Cc gabor@… added

comment:9 Changed 6 years ago by mauve

  • Summary changed from ModelChoiceField fails when passed a non integer string to 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 Changed 6 years ago by bfirsh

  • Cc ben@… added

Changed 5 years ago by hgeerts@…

comment:11 Changed 5 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 5 years ago by Tuttle

  • Cc t.django@… added

comment:13 Changed 5 years ago by EroSennin

  • Cc EroSennin added

comment:14 Changed 5 years ago by romke

  • Cc romke added

comment:15 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Uncategorized

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

comment:16 Changed 4 years ago by Harm Geerts <hgeerts@…>

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

comment:17 Changed 4 years ago by julien

  • Type changed from Uncategorized to Bug

comment:18 Changed 4 years ago by claudep

  • Resolution set to fixed
  • Status changed from new to closed
  • 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