admin list_editable with unique_together raises Integrity Error

[Sławek Ehlert]

I'm getting an Integrity Error when trying to edit (on changelist) a model that has some unique_together fields and is
registered with list_editable containing some of that fields (not all of them).

Example

#models.py
class Test(models.Model):
    t1 = models.CharField(max_length=255, blank=True, null=True)
    t2 = models.CharField(max_length=255, blank=True, null=True)
    t3 = models.CharField(max_length=255, blank=True, null=True)

    class Meta:
        unique_together = ['t1','t2','t3']

#admin.py
class TestAdmin(admin.ModelAdmin):
    list_display = ['t3', 't1', 't2']
    list_editable = ['t1', 't2',]

admin.site.register(Test, TestAdmin)

the changelist form (?) doesn't check the uniqueness of an edited record.

Integrity Error is raised when for example having two rows -

first: t1 = 'a', t2 = 'b', t3 = 'z'

and second: t1 = 'a', t2 = 'a', t3 = 'z'

and trying to change the t2 in first row to 'a'.

In case when all fields from unique_together are in list_editable, the error message does show. However it isn't very informative (only "Please correct the errors below." but nothing below is indicated).

Done this on sqlite3 and PostgreSQL.

[Karen Tracey]

This is not a regression, so at this point not a candidate for 1.2.

Fields not on a model form are excluded from unique_together checking. This is noted here: ​http://docs.djangoproject.com/en/dev/ref/models/instances/#django.db.models.Model.validate_unique, though you kind of have to guess that this call with non-editable fields excluded is what's going to happen for the admin change list page.

Possibly the unique validation code could be changed to use existing instance values, if present, for excluded fields instead of simply dropping them from consideration. Not sure why it doesn't do that already.

A different ticket ought to be opened for the error message only saying "Please correct the errors below" and nothing about the actual problem. That's a different issue.

[Oroku Saki]

Replying to kmtracey:

Possibly the unique validation code could be changed to use existing instance values, if present, for excluded fields instead of simply dropping them from consideration. Not sure why it doesn't do that already.

I've mentioned this in my ticket with another example of where it'd be helpful: #13249 I might open a new ticket requesting this specific feature of excluding only if not present.

[Julien Phalip]

Replying to slafs:

In case when all fields from unique_together are in list_editable, the error message does show. However it isn't very informative (only "Please correct the errors below." but nothing below is indicated).

This particular issue should be fixed by the patch posted in #13126.

[Julien Phalip]

Like Karen, I'm surprised that any field should be excluded for validating uniqueness at this stage. That behaviour was apparently set in [12269] but it wasn't tested anyway.

The patch simply removes that exclusion.

[Ramiro Morales]

See #12521, #12901 and related commits.

[Russell Keith-Magee]

#15326 is another report of this problem, slightly closer to the root cause.

[Julien Phalip]

Just a note about the first patch above: the reason why I'm only testing the presence of the message "Please correct the errors below." in the response is because the actual specific error message currently isn't displayed at all. If/when the patch in #13126 gets committed (to allow the display of non-field error messages), then the test for this patch can be made slightly more robust by asserting the presence of the specific unique_together error message.

[Julien Phalip]

As per the note above, now that #13126 has been checked in I've just updated the patch so that the test asserts the presence of the relevant error message ("Unique together with this T1, T2 and T3 already exists.").

[Ramiro Morales]

#12028 seems to be a manifestation of this issue.

[Julien Phalip]

#15860 reports the same problem.

[legutierr]

Some relevant points about this are raised in #13249, and there is a discussion in the news group regarding this here:

As noted in the google-group thread, the current patch may break the fixes to #12521 and #12901, as discussed here: ​http://groups.google.com/group/django-developers/browse_thread/thread/cee43f109e0cf6b. It seems to me that a proper fix would focus only on how exclusions are used in deciding whether to ignore unique_together constraints in model validation, in the manner I propose in the thread.

[legutierr]

This patch keeps the tests added to admin_views by the prior patch, but it makes a more precise change to the behavior (plus more tests and documantaion), as discussed here: ​http://groups.google.com/group/django-developers/browse_thread/thread/deb2645bf5157211

[Carl Meyer]

Latest patch is much closer. Thanks for the patch! Some comments:

• The versionadded note in the validate_unique docs should probably go right below the modified paragraph, not at the top of the section.
• The "as of Django 1.2" note in the unique_together docs should use the versionadded marker.
• This patch doesn't require adding another method to model _meta; it's already bloated. Use the existing .get_field_by_name() method and discard the values you don't need in the returned tuple.
• The use of check_count is a bit convoluted and hard to understand. Given the new logic, I think it would be clearer to drop the use of for...else and just have a boolean perform_check that gets flipped True if any field in the check is found to not be excluded and not have a default (and when its flipped to True you can break from the loop) and then after the loop use the boolean to decide whether to add the check to the unique_checks list.
• Why does the admin list_editable test post values for t3 in the POST data when that field is not in list_editable? Does the admin actually post values from hidden inputs for all fields not included in list_editable? This test should be checked to make sure it matches what the admin actually does.
• I'm not sure checking if default is NOT_PROVIDED is actually sufficient. In particular, I think a CharField blank=True but with no default specified could still break if there's a record in the database with that field blank, because empty string is the implicit default. We need a test for this case.
• The test where values are set on form.instance prior to validation isn't really relevant, because that's not something Django does internally (e.g. in the admin), nor is it a documented pattern we need to ensure works. It would be better to replace that test with a case like the one in the #15326 report, a real-world case that is currently broken.

It's late so I won't guarantee there isn't more that I'm missing, but that's what I see on first review.

[legutierr]

The patch I just attached should address all of your issues, Carl, except that I don't quite understand what you are referring to in your last bullet, regarding setting form.instance prior to validation. I do not see anything comparable in #15326. Did you mean #13249?

I realize that setting values on form.instance prior to calling form.is_valid() is not the normal approach, but I am not sure if there is any other approach that makes as much sense. The validate_unique() call needs to be made only after the final values are set on the ORM instance, otherwise the check will not catch the kinds of errors that we are trying to catch prior to the database being hit. Because is_valid only calculates its the form errors once (after which they are cached), all of this has to be done before is_valid is called, and updating form.instance seems to be the easiest way.

That being said, I did change things so that they would look a little bit more like Russel's example in #13249. Is that all you were thinking?

[Jannis Leidel]

This looks good to me, is there anything that would hold this back?

[Jacob]

Milestone 1.3 deleted

[Sebastian Goll]

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?

[Carl Meyer]

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 ​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 ValidationErrors back into the not-valid code path).

2) Add a warning to the above section of the docs (on partial ModelForms) 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 ​this thread, but haven't yet hit on anything that feels exactly right.

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

[Evstifeev Roman]

Regarding the proposed solution: Why not just add optional kwarg to is_valid method of ModelForm? like this:

if form.is_valid(strict_unique_checks=True):
    obj = form.save()

The "strict_unique_checks" kwarg will force ModelForm to perform unique_together checks using existing instance values if some.fields are omitted in form.

Another option is to add new Meta option like this:

class MyForm(ModelForm):
    class Meta:
        strict_unique_checks = True

Such form will then always fallback to using existing instance values for unique_together checks when performig validation.

[Jon Dufresne]

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)

[Carl Meyer]

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.

[Carlton Gibson]

As well as the patches here, there's a ​PR for this now. Unchecking Patch needs improvement to pull this back into the review queue.

[Carlton Gibson]

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 ​added a test for this usage in a PR.)

Reviewing the history, Carl Meyer's comments still capture the situation.

[Tim Graham <timograham@…>]

In 1c05fe65:

Refs #13091 -- Added test for commit=False idiom with partial unique_together validation.

[Carlton Gibson]

Same issue pops up in #29649, where a unique_together field is excluded from the form via read_only_fields

[Ryan Jarvis]

In Django 2.2 it appears this affects CheckConstraint and UniqueConstraint as well.