Re: [Django] #13091: admin list_editable with unique_together raises Integrity Error
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Milestone: 1.3 | Component: contrib.admin
Version: SVN | Severity: Normal
Resolution: | Keywords: list_editable
Triage Stage: Accepted | unique_together IntegrityError
Needs documentation: 0 | Has patch: 1
Patch needs improvement: 1 | Needs tests: 0
UI/UX: 0 | Easy pickings: 0
-------------------------------------+-------------------------------------
Changes (by jezdez):
* ui_ux: => 0
* stage: Design decision needed => Accepted
Comment:
This looks good to me, is there anything that would hold this back?
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:16>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: sebastian (added)
Comment:
For the record, I just ran into this issue. In my case, it was the missing
check on uniqueness involving content types, as outlined in #12028.
That said, fixing this issue is highly appreciated here. Is there still a
reason why the proposed patch (comment:15) cannot be applied?
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* component: contrib.admin => Forms
Comment:
Hmm, there was extensive discussion of this on the mailing list around the
time that patch was worked on; apparently it never got summarized here; I
should have done that. I did want to get this resolved in some way for
Django 1.4.
When I dug into this issue in preparing to commit the patch here, I
realized that this bug is a case where `ModelForm` currently promises
something (validation of "incomplete" models) that isn't possible to
deliver in a way that is consistently correct. By applying this patch we
would just be trading one class of bugs for another; it could easily break
existing code that is correctly following documented patterns.
To give an example of the type of code that could be broken by this patch,
here's a common (and
[https://docs.djangoproject.com/en/dev/topics/forms/modelforms/#using-a
-subset-of-fields-on-the-form documented]) pattern for handling excluded
fields on a `ModelForm`:
{{{
if form.is_valid():
obj = form.save(commit=False)
obj.user = request.user
obj.save()
}}}
If this model has a unique-together constraint on "user" and some other
field that is part of the form, currently the `ModelForm` does not
validate that constraint. And it should not, because the real value of the
"user" field is not set until after form validation occurs. If we suddenly
start validating that constraint during form validation, as this patch
would do, it would be validated against whatever the default value for
"user" happens to be, not the actual to-be-saved value (which isn't set
until later). This could easily lead to false positives on the constraint
check, if there are any existing records in the database that actually
have that default value as their value for "user". And it still wouldn't
catch actual constraint violations, because the constraint would still
never be checked with the "real" value for the user field.
Options for moving forward, as I see them (none of these are mutually
exclusive):
1) Just fix this in the particular case of the admin. This could possibly
be done by adding a manual call to `full_clean` once the instance is fully
populated (might be a bit ugly getting any `ValidationError`s back into
the not-valid code path).
2) Add a warning to the above section of the docs (on partial
`ModelForm`s) about this issue and how to work around it in your own
forms.
3) Provide and document a better idiom for `ModelForm` validation, in
which tweaks to the model-to-be-saved always happen before validation, and
validation can then cover all fields on the model. Alex and I and others
started on some ideas for that (using context managers) in
[http://groups.google.com/group/django-
developers/browse_frm/thread/3d3d7b7200d23b0f/542c9991286c854b?lnk=gst&q=better+ModelForm+validati#542c9991286c854b
this thread], but haven't yet hit on anything that feels exactly right.
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: SVN
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: carsten.fuchs@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: SVN
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by david@…):
If you came here looking for a terrible work around till this gets
fixed...
{{{
def save_model(self, request, obj, form, change):
#HACK to work around bug 13091
try:
obj.full_clean()
obj.save()
except forms.ValidationError:
messages.warning(request, 'Could not save %s' % (obj,))
}}}
Use with caution as it's a bit strange to be saving some objects while
there is a validation issue. But it's also strange to save models then get
a 500 error.
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Fak3):
Regarding the proposed solution: Why not just add optional kwarg to
''is_valid'' method of ''ModelForm''? like this:
{{{
#!python
if form.is_valid(strict_uinique_checks=True):
obj = form.save()
}}}
The "''strict_uinique_checks''" kwarg will force ''ModelForm'' to perform
uinique_together checks using existing instance values if some.fields are
omitted in form.
Another option is to add new Meta option like this:
{{{
#!python
class MyForm(ModelForm):
class Meta:
strict_uinique_checks = True
}}}
Such form will then always fallback to using existing instance values for
uinique_together checks when performig validation.
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* cc: someuniquename@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by jdufresne):
I recently ran into this issue. Needless to say, I would very much
appreciate a proper solution to validating partial `unique_together`
fields.
I started a thread on Django developers before this ticket was brought to
my attention here: [https://groups.google.com/forum/#!msg/django-
developers/FmllO4t53bE/79GMGTKDwooJ]
In the thread I proposed an alternative solution. I'm not saying it is any
better or worse than the other proposed solutions. Carl Meyer, pointed out
some early issues with this idea, but I'll add it here for documentation
purposes.
----
One solution would be for model formset instances to have a "static"
fields argument. This could be a dictionary with a subset of field names
as keys with field values. The assumption being that all instances in the
formset share these static values. These fields would not be a part of the
rendered HTML, but could be used for:
* Default queryset (could always be overridden)
* Creating new instances
* Validating form data (mostly `unique_together`)
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: slafs | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by carljm):
Basically there are two problems that need to be solved in order to fix
this:
1) How to tell the form what values to use on the model instance for
excluded fields when performing validation involving those fields.
2) How to tell the form to go ahead and do validation on excluded fields,
because we have provided meaningful values for them.
(1) can already be done by passing in an unsaved new `instance` at form
instantiation with some attributes set on it, but that leaves (2) to be
solved. Fak3 proposes a `strict_unique_checks` Meta setting or `is_valid`
kwarg for (2).
jdufresne's proposal of a new "static field values" argument handles both
of these; if you provide these values, you are indicating you want full
validation.
I don't love adding a second way to supply pre-filled model attribute
values, but I also don't like the cruft of a new Meta option that's
basically only there for backwards compatibility.
The proposal of a context-manager-based approach in the linked thread
solves both the problems, at the cost of introducing an entirely new idiom
for validating model forms (different from that used for validating
regular forms). This may be OK (model forms already have API that regular
forms don't), but it's a bigger change.
I don't like adding a kwarg to `is_valid`, because that introduces
multiple possible validity states for a single bound form, depending how
`is_valid` was called. What would be the correct behavior if you called
`is_valid(full=False)` and then `is_valid(full=True)`? A given bound form
instance should either be consistently valid or not, not "it depends how
you ask" or (even worse) "it depends how you asked last time" (keeping in
mind that `form.errors`, once populated, can be accessed directly).
I think I'd prefer an `__init__` kwarg to a `Meta` setting, because the
feature is likely used along with passing in an unsaved `instance` at init
time, so it's naturally instance-specific. If you want every instance of a
given `ModelForm` class to do full validation, you can override
`__init__`.
So I think my leaning is to add a kwarg to `__init__` and then document
the use of that kwarg in conjunction with passing in a pre-populated
unsaved `instance`. Rather than giving the kwarg a unique-checks-specific
name, I would give it a name that more generally indicates "I want full
validation of the model, not just validation of the fields included in the
form"; maybe something like `validate_full_model=True`?
The docs would need to note that if you supply an invalid value for an
excluded field and then ask for full validation, you're making it
impossible for users to complete the form; that's on you as the developer
to avoid.
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:25>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Rafael Ferreira):
* cc: Rafael Ferreira (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlton Gibson (added)
* needs_better_patch: 1 => 0
Comment:
As well as the patches here, there's a
[https://github.com/django/django/pull/8470 PR] for this now. Unchecking
Patch needs improvement to pull this back into the review queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Carlton Gibson):
* has_patch: 1 => 0
Comment:
The PR adopted (essentially) the same approach as the patch attached here,
which isn't viable since it breaks the `commit=False` idiom.
(I've [https://github.com/django/django/pull/10216 added a test for this
usage in a PR].)
Reviewing the history, Carl Meyer's comments still capture the situation.
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"1c05fe65f280cedaddf5e5f308e5e45449b02e93" 1c05fe65]:
{{{
#!CommitTicketReference repository=""
revision="1c05fe65f280cedaddf5e5f308e5e45449b02e93"
Refs #13091 -- Added test for commit=False idiom with partial
unique_together validation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Same issue pops up in #29649, where a `unique_together` field is excluded
from the form via `read_only_fields`
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Carlos Palol (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Ryan Jarvis (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Ryan Jarvis):
In Django 2.2 it appears this affects CheckConstraint and UniqueConstraint
as well.
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Sławek Ehlert | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: list_editable | Triage Stage: Accepted
unique_together IntegrityError |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Dan Madere (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/13091#comment:34>