Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#12881 closed (fixed)

Unique constraint error with model inheritance while ModelForm should not validate

Reported by: fgaudin Owned by: nobody
Component: Database layer (models, ORM) Version: 1.1
Severity: Keywords: unique constraint, ModelForm, inheritance
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

The use case is :

  • ParentClass has an attribute "name" with unique=True
  • FirstSubClass inherits ParentClass
  • SecondSubClass inherits ParentClass

With a ModelForm for each subclass, I can't create two FirstSubClasses with the same name, neither two SecondSubClasses, the form doesn't validate, what is expected.
But if I create a FirstSubClass instance and try to create a SecondSubClass instance, the form validate and unique constraint error is raised by postgres.

A project showing the use case with unit tests is in attachment.

Attachments (2)

django_bug.tar.gz (3.4 KB) - added by fgaudin 5 years ago.
project which shows the validation behaviour of ModelForm in case of inheritance
12881.diff (22.9 KB) - added by kmtracey 5 years ago.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by fgaudin

project which shows the validation behaviour of ModelForm in case of inheritance

comment:1 Changed 5 years ago by fgaudin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Same bug in django 1.2 with sqlite

comment:2 Changed 5 years ago by kmtracey

  • Component changed from Forms to Database layer (models, ORM)
  • milestone set to 1.2
  • Triage Stage changed from Unreviewed to Accepted

Ugh. The model class used for the unique checks is the model class associated with the form. In the case where this class is derived from another, the resulting query involves a join of the tables for the derived and parent models. Thus the unique check is improperly constraining the unique test in the case where the unique constraint is declared in the parent class. I think the unique check needs to be made using the model class where the unique constraint actually came from. I'll attach a patch that works for the posted test, but I'm not too familiar with all the model class internals so there well be a more straightforward way to determine the right class to use for the check.

comment:3 Changed 5 years ago by Alex

  • Has patch set
  • Needs tests set

comment:4 Changed 5 years ago by kmtracey

#13079 reports an additional problem with unique_together that persists even after a fix for this is applied. Apparently the unique_together constraint on the parent is not even seen as something that needs checking for a ModelForm for the child model.

comment:5 Changed 5 years ago by kmtracey

  • Patch needs improvement set

Given the report in #13079 I think the approach in the patch here is wrong. Unique constraints on a field are only being seen because all the fields are pushed down to the child model. #13079 shows unique_together specified on a parent is invisible to the child model, thus the logic that gathers the unique checks in the first place needs to be changed. I think it would better for that logic to keep track of the model that is the source of the each unique constraint, rather than later on attempting to figure out which model should be used for performing the check.

comment:6 Changed 5 years ago by kmtracey

  • Needs tests unset
  • Patch needs improvement unset

I've attached a new patch, with tests, that attempts to fix the issues for inherited unique, unique_together, and unique for dates. Review from someone with a clue in the ORM area would be helpful.

comment:7 Changed 5 years ago by kmtracey

  • Patch needs improvement set

Full test suite fails with various formset unique validation errors so there's clearly something not quite right with it yet.

Changed 5 years ago by kmtracey

comment:8 Changed 5 years ago by kmtracey

  • Patch needs improvement unset

New patch, passes full test suite. Both model formsets and some validation tests depend on what is returned by _get_unique_checks(), so they were unexpectedly (to me) broken by changing what that returned.

comment:9 Changed 5 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from new to closed

(In [12797]) Fixed #12881: Corrected handling of inherited unique constraints. Thanks for report fgaudin.

comment:10 Changed 5 years ago by kmtracey

Though this problem afflicts 1.1.X as well, backporting the fix is not trivial. Subsequent to 1.1 all the unique validation logic has been moved from the ModelForm code into the base Model code. Moving the logic of this fix (which relies on internals of model data structures) back out to the ModelForm class is decidedly ugly, so at present I'm inclined to leave this as a 1.2-only fix.

comment:11 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

Note: See TracTickets for help on using tickets.
Back to Top