[Django] #29549: ModelForms doesn't validate CHOICES
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny | Owner: nobody
Arshinov |
Type: | Status: new
Uncategorized |
Component: Database | Version: 1.11
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
This is basically a sequel of #6967.
For some reason, only form validation was considered in the original
ticket, but data validation is not limited to that, but also includes data
integrity checks running when one calls `model_instance.save()`.
As a developer, I would expect `model_instance.save()` to validate the
`choices` field against the list of possible choices, not least because
the list is defined at the model level, participates in db migrations
etc., so it should take effect on modal instances.
I haven't been able to find any documentation or existing bug reports that
clearly state that [choices
https://docs.djangoproject.com/en/2.0/ref/models/fields/#choices] argument
only affects the corresponding model form field presentation and does not
ensure data integrity.
In summary:
1. I would like to know the rationale for ignoring `choices` during data
integrity checks, in case I am missing something.
2. Please consider providing an option to turn data integrity check on.
3. If the current behavior is left intact, it should be explicitly
documented in the official documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/29549>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Uncategorized | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Did you mean to title the ticket "Model.save() doesn't validate CHOICES"?
This is
[https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.Model.clean
documented]:
> Note, however, that like Model.full_clean(), a model’s clean() method is
not invoked when you call your model’s save() method.
This is because of backwards compatibility (and probably performance
considerations).
There are ways to [https://stackoverflow.com/questions/4441539/why-doesnt-
djangos-model-save-call-full-cleanchange change the behavior in your
project].
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: nobody
Type: Uncategorized | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> This is basically a sequel of #6967.
>
> For some reason, only form validation was considered in the original
> ticket, but data validation is not limited to that, but also includes
> data integrity checks running when one calls `model_instance.save()`.
>
> As a developer, I would expect `model_instance.save()` to validate the
> `choices` field against the list of possible choices, not least because
> the list is defined at the model level, participates in db migrations
> etc., so it should take effect on modal instances.
>
> I haven't been able to find any documentation or existing bug reports
> that clearly state that [choices
> https://docs.djangoproject.com/en/2.0/ref/models/fields/#choices]
> argument only affects the corresponding model form field presentation and
> does not ensure data integrity.
>
> In summary:
> 1. I would like to know the rationale for ignoring `choices` during data
> integrity checks, in case I am missing something.
> 2. Please consider providing an option to turn data integrity check on.
> 3. If the current behavior is left intact, it should be explicitly
> documented in the official documentation.
New description:
This is basically a sequel of #6967.
For some reason, only form validation was considered in the original
ticket, but data validation is not limited to that, but also includes data
integrity checks running when one calls `model_instance.save()`.
As a developer, I would expect `model_instance.save()` to validate the
`choices` field against the list of possible choices, not least because
the list is defined at the model level, participates in db migrations
etc., so it should take effect on modal instances.
I haven't been able to find any documentation or existing bug reports that
clearly state that
[https://docs.djangoproject.com/en/2.0/ref/models/fields/#choices choices]
argument only affects the corresponding model form field presentation and
does not ensure data integrity.
In summary:
1. I would like to know the rationale for ignoring `choices` during data
integrity checks, in case I am missing something.
2. Please consider providing an option to turn data integrity check on.
3. If the current behavior is left intact, it should be explicitly
documented in the official documentation.
--
Comment (by Evgeny Arshinov):
Replying to [comment:1 Tim Graham]:
> Did you mean to title the ticket "Model.save() doesn't validate
CHOICES"?
>
Yes. Sorry for the incorrect title, I fixed that.
> This is
[https://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.Model.clean
documented]:
>
> > Note, however, that like Model.full_clean(), a model’s clean() method
is not invoked when you call your model’s save() method.
OK, I think I did not pay enough attention to the point that saving and
validation are two different steps, as documented in the
[https://docs.djangoproject.com/en/dev/ref/models/instances/#validating-
objects model instance reference], and fields are generally validated
during `clean()` except for explicitly documented cases like
[https://docs.djangoproject.com/en/2.0/ref/models/fields/#unique unique]
(which is validated in `save()`).
However, it seems that many people stumble upon this issue. Maybe an
explicit note could be added to the documentation for the choices
parameter?
>
> This is because of backwards compatibility (and probably performance
considerations).
>
> There are ways to [https://stackoverflow.com/questions/4441539/why-
doesnt-djangos-model-save-call-full-cleanchange change the behavior in
your project].
OK, thank you for the direct link. We will consider using this approach in
our project.
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: assigned
Component: Documentation | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => assigned
* component: Database layer (models, ORM) => Documentation
* owner: nobody => Tim Graham
* has_patch: 0 => 1
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
[https://github.com/django/django/pull/10161 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Tim
Type: | Graham
Component: Documentation | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8b1d361f28c80cb0fa84a3714d711174bd25cdfa" 8b1d361f]:
{{{
#!CommitTicketReference repository=""
revision="8b1d361f28c80cb0fa84a3714d711174bd25cdfa"
Fixed #29549 -- Doc'd that Field.choices are enforced by model validation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"50dc9fad74b4992602584252207f38c5faecc2ba" 50dc9fad]:
{{{
#!CommitTicketReference repository=""
revision="50dc9fad74b4992602584252207f38c5faecc2ba"
[2.1.x] Fixed #29549 -- Doc'd that Field.choices are enforced by model
validation.
Backport of 8b1d361f28c80cb0fa84a3714d711174bd25cdfa from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> This is basically a sequel of #6967.
>
> For some reason, only form validation was considered in the original
> ticket, but data validation is not limited to that, but also includes
> data integrity checks running when one calls `model_instance.save()`.
>
> As a developer, I would expect `model_instance.save()` to validate the
> `choices` field against the list of possible choices, not least because
> the list is defined at the model level, participates in db migrations
> etc., so it should take effect on modal instances.
>
> I haven't been able to find any documentation or existing bug reports
> that clearly state that
> [https://docs.djangoproject.com/en/2.0/ref/models/fields/#choices
> choices] argument only affects the corresponding model form field
> presentation and does not ensure data integrity.
>
> In summary:
> 1. I would like to know the rationale for ignoring `choices` during data
> integrity checks, in case I am missing something.
> 2. Please consider providing an option to turn data integrity check on.
> 3. If the current behavior is left intact, it should be explicitly
> documented in the official documentation.
New description:
This is basically a sequel of #6967.
For some reason, only form validation was considered in the original
ticket, but data validation is not limited to that, but also includes data
integrity checks running when one calls `model_instance.save()`.
As a developer, I would expect `model_instance.save()` to validate the
`choices` field against the list of possible choices, not least because
the list is defined at the model level, participates in db migrations
etc., so it should take effect on model instances.
I haven't been able to find any documentation or existing bug reports that
clearly state that
[https://docs.djangoproject.com/en/2.0/ref/models/fields/#choices choices]
argument only affects the corresponding model form field presentation and
does not ensure data integrity.
In summary:
1. I would like to know the rationale for ignoring `choices` during data
integrity checks, in case I am missing something.
2. Please consider providing an option to turn data integrity check on.
3. If the current behavior is left intact, it should be explicitly
documented in the official documentation.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Tim
| Graham
Type: Uncategorized | Status: new
Component: Database layer | Version: 1.11
(models, ORM) |
Keywords: | Triage Stage:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
* component: Documentation => Database layer (models, ORM)
* has_patch: 1 => 0
* type: Cleanup/optimization => Uncategorized
* stage: Accepted => Unreviewed
Comment:
Guys, I am sorry but I have to reopen the ticket because it turned out
that the proposed solution with calling `full_clean` on `pre_save` does
not work, which means that the original problem is still unsolved. In
#29655 I have received a [comment:3:ticket:29655 response] that performing
validation during model save is not a supported scenario.
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:7>
Django
Reporter: Evgeny Arshinov | Owner: Tim
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
* component: Database layer (models, ORM) => Documentation
* type: Uncategorized => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
Absent some proposal of the changes Django should make (not clear to me),
opening a Trac ticket isn't the way to proceed. This ticket has been
repurposed for further documentation. If we decide to make some other
change, we'll open a new ticket. Feel free to write to the
DevelopersMailingList if you have some ideas.
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Evgeny Arshinov):
Replying to [comment:8 Tim Graham]:
> Absent some proposal of the changes Django should make (not clear to
me), opening a Trac ticket isn't the way to proceed. This ticket has been
repurposed for further documentation. If we decide to make some other
change, we'll open a new ticket. Feel free to write to the
DevelopersMailingList if you have some ideas.
Hello Tim,
I don't have a proposal except starting to support model validation on
save. I am not familiar with Django's inner workings to suggest any
concrete steps.
However, I am inclined to think that presence of a problem should be
enough for a ticket to be opened, whether or not there is a concrete
change proposal. The question about incomplete validation comes up fairly
regularly i.e. on StackOverflow, and a ticket in the official bug tracker
could be a good reference source and an indicator of the fact that Django
developers are aware of the problem and maybe fix it someday. Also, a
ticket can be subscribed to. I guess I am not the only one who prefers a
ticket to numerous disorganized discussions on StackOverflow and other
forums.
I understand that what I have written might contradict the code of
practice established in Django. If it is so, I am fine with closing the
discussion since I have nothing more to suggest.
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Evgeny Arshinov | Owner: Tim
Type: | Graham
Cleanup/optimization | Status: closed
Component: Documentation | Version: 1.11
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
As Simon said in #29655, "Enabling model validation on save is likely to
break a few things and degrade performance significantly. I don't think
that's a pattern we should encourage or support at this point. Django and
third-party applications simply don't expect ValidationError to be thrown
on save(), this effectively change the signature of the function."
Thus, a ticket suggesting that would be closed as wontfix unless a
discussion on the mailing list reaches a different conclusion.
--
Ticket URL: <https://code.djangoproject.com/ticket/29549#comment:10>