ModelChoiceField's clean() uses default manager instead of query set
Nick
ModelMultipleChoiceField use a block similar to the following in order
to validate the selected choice:
try:
value = self.queryset.model._default_manager.get(pk=value)
except self.queryset.model.DoesNotExist:
raise ValidationError(ugettext(u'Select a valid choice. That
choice is not one of the available choices.'))
Is there any reason that shouldn't simply be
self.queryset.get(pk=value) ?
I came across this in a project when I was trying to work out why it
was allowing choices that I had explicitly filtered out of the
queryset - and of course this explains it. Is there a reason that the
default manager should be used instead of just the original queryset?
oggie rob
you have previously-valid information and wish to save it again, it
may become invalid.
For example, I have a list of "active" employees for selection in the
filtered choice field. If I open up some object and hit "Save" after
the employee is no longer active, this would incorrectly raise an
error. I would imagine this avoids it.
-rob
Nick Lane
> I'm not sure of the intent, but something to keep in mind is that if
> you have previously-valid information and wish to save it again, it
> may become invalid.
> For example, I have a list of "active" employees for selection in the
> filtered choice field. If I open up some object and hit "Save" after
> the employee is no longer active, this would incorrectly raise an
> error. I would imagine this avoids it.
Hmm, I see what you're saying but (using your example) if the employee
is no longer active, wouldn't that mean the field *should* raise a
validation error? If the choices are meant to be limited to only
employees which are active shouldn't the validation prevent the user
from choosing an option which has been invalidated since the form was
rendered?
At least that's the reason I came across this in the first place. For
some background: The user needs to choose from a list of objects, and
these objects expire over time. I wanted validation to catch the case
of a selected choice expiring and no longer being valid. I have
created a custom ModelChoiceField which works using the query set
instead of the default manager and that suits my purposes fine, I just
thought I would raise it here to see if this should apply in general
or not.
Cheers,
Nick
oggie rob
> is no longer active, wouldn't that mean the field *should* raise a
> validation error? If the choices are meant to be limited to only
> employees which are active shouldn't the validation prevent the user
> from choosing an option which has been invalidated since the form was
> rendered?
Not in my case. Among other things, I'm tracking employee's time. So
just because they're no longer active doesn't mean the work they did
previously is invalid. Although making changes to these records is not
common, sometimes some details need to be updated and I don't want to
be prevented. I see the choices interface as a convenience /
performance tool, but not as a validation tool.
> At least that's the reason I came across this in the first place. For
> some background: The user needs to choose from a list of objects, and
> these objects expire over time. I wanted validation to catch the case
> of a selected choice expiring and no longer being valid. I have
> created a custom ModelChoiceField which works using the query set
> instead of the default manager and that suits my purposes fine, I just
> thought I would raise it here to see if this should apply in general
> or not.
So, this would be a problem if you were using the form for updating
records, but reading between the lines it looks like you are using the
form only for adding records. However, I'm a little confused now. I
thought the problem was only validation, and not the actual data that
was added to the choice list. Perhaps its too late for me to be
commenting (like usual)... I'll let others speak now :)
-rob
David Danier
> thought the problem was only validation, and not the actual data that
> was added to the choice list.
The problem is, that validation does not use the actual data of the
choice list.
I think doing so may even become a security issue, as people may guess
valid IDs that they should not use (for example if you use
user_profile.some_relation as the queryset).
Perhaps it could be changed to only allow choices in the queryset, but
an option is added to ModelChoiceField to use the default manager?
Greetings, David Danier
Nick Lane
> > However, I'm a little confused now. I
> > thought the problem was only validation, and not the actual data that
> > was added to the choice list.
>
> The problem is, that validation does not use the actual data of the
> choice list.
>
> I think doing so may even become a security issue, as people may guess
> valid IDs that they should not use (for example if you use
> user_profile.some_relation as the queryset).
Well that was the point I was trying to make, but I probably didn't
word myself as clearly as I should have :-)
Validation is done using a (possibly) different set of data than the
choices displayed to the user.
> Perhaps it could be changed to only allow choices in the queryset, but
> an option is added to ModelChoiceField to use the default manager?
That could work. The main point of this thread was to see if there are
use cases for using the default manager for validation instead of the
query set because I couldn't really think of any - but I'm sure there
could well be some.
Cheers,
Nick
oggie rob
> use cases for using the default manager for validation instead of the
> query set because I couldn't really think of any - but I'm sure there
> could well be some.
Like the one I explained? :)
I think the strongest argument against using adding an extra
"validation queryset" argument is that it doesn't give the flexibility
of programmatically validating data. A function works much better in
this respect. And as for the default behaviour, I've put forth my
thoughts on it and I think it is more commonly a problem than the
security issue that was suggested (since you probably won't usually
construct your forms to allow a user to have any say in their own
access).
Not incidentally, this is what the "validator_list" has been used for.
However I'm not sure if this will remain in place since oldforms is
going away... but you should be able to transition it easily if
necessary.
-rob
Nick Lane
> > That could work. The main point of this thread was to see if there are
> > use cases for using the default manager for validation instead of the
> > query set because I couldn't really think of any - but I'm sure there
> > could well be some.
>
> Like the one I explained? :)
Sorry, perhaps I don't quite understand how this is different...
unless you are rendering the field widget manually and somehow adding
extra choices that aren't in the query set passed to the
Model*ChoiceField?
> I think the strongest argument against using adding an extra
> "validation queryset" argument is that it doesn't give the flexibility
> of programmatically validating data. A function works much better in
> this respect. And as for the default behaviour, I've put forth my
> thoughts on it and I think it is more commonly a problem than the
> security issue that was suggested (since you probably won't usually
> construct your forms to allow a user to have any say in their own
> access).
I guess my take on it is that if you wanted the user to be able to
choose from any object you would just pass the default manager as the
query set instead of passing in a filtered query set in the first
place.
If you are passing in a query set that has some objects excluded, the
user won't be able to choose them anyway since they won't be presented
in the list of choices for the field[1]. I figured the validation
should work in the same way and restrict the submitted choices to the
choices displayed in the form field.
> Not incidentally, this is what the "validator_list" has been used for.
> However I'm not sure if this will remain in place since oldforms is
> going away... but you should be able to transition it easily if
> necessary.
I could add a clean_FIELDNAME method to the form to validate that the
choice is actually one contained in the query set and not just the
default manager, but this means an extra lookup since it will be done
after the field's clean() method is called (which does the original
lookup using the default manager).
As I said before I'm not too fussed if it's decided that this should
not apply to the standard Model*ChoiceField classes, but I felt that
it made sense. I'll shut up now :-)
Cheers,
Nick
[1] Unless a clever user hacked up a POST request which included the
excluded items. Validation would pass, which could be considered a
security issue.
oggie rob
> unless you are rendering the field widget manually and somehow adding
> extra choices that aren't in the query set passed to the
> Model*ChoiceField?
There would be one extra choice - the previously selected, and no
longer valid, related field!
Perhaps I can be clearer with the following example. Please ignore the
details of getting/displaying the form - this is only to show you the
issue with related fields and the choice validation:
class Employee(models.Model):
first_name ...
last_name ...
active ...
class Activity(models.Model):
employee = models.ForeingnKey(Employee,
limit_choices_to={'active__exact':True})
type = models.ChoiceField(...)
duration = models.PositiveSmallIntegerField()
notes = models.TextField()
# again, ignore the details of getting/displaying the form. Just using
the admin will show the potential problem
1) Create new active employee, employee1
2) Create new activity object, activity1 linked to employee1
3) Find employee in intimate relationship with office equipment, fire
employee
4) Edit employee1 so that active=False
5) Edit activity1 (the original employee1 item is still displayed &
selected) to add some notes ("Remember to buy new furniture") & save.
Blam! Can't save because employee is not active, if using the
"active=False" manager. (Currently however, admin would be using the
default manager so it would allow you to save.)
So hopefully you can see why I think it is a good idea to restrict the
choices, for convenience, but not the validation part of the
ModelChoiceField. This doesn't solve all problems - like adding
"activity2" after employee1 is fired - but at least it keeps the
original objects usable.
Now if you really wanted to prevent somebody from cheating the
selection, add the extra validation - but again, this would be easier
as a method (e.g. if superuser, use default manager for validation:
else if editing & the same, return: else use custom manager for
validation).
> I guess my take on it is that if you wanted the user to be able to
> choose from any object you would just pass the default manager as the
> query set instead of passing in a filtered query set in the first
> place.
Because the number of unfiltered items might be unreasonable large, or
at least annoyingly large, in a select field.
> As I said before I'm not too fussed if it's decided that this should
> not apply to the standard Model*ChoiceField classes, but I felt that
> it made sense. I'll shut up now :-)
Well it is a judgement call, not black and white. I've just been in
the situation I've mentioned (except maybe for the office furniture
part!) and I know it would be a major pain to deal with validation
that prevents legitimate use.
-rob