Code

Opened 2 years ago

Closed 16 months ago

#17615 closed Bug (fixed)

Unique field validation with multi-table inheritance

Reported by: ungenio Owned by: nobody
Component: Database layer (models, ORM) Version: 1.5
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

I've encountered an issue where if you have a Parent class with a unique field and a Child class that specifies its own primary_key, the unique field will fail validation when using a Child ModelForm.

I'm attaching a patch that includes a test and fix.

Attachments (2)

unique_field_with_mti.diff (3.6 KB) - added by ungenio 2 years ago.
Test and fix.
17615.diff (3.1 KB) - added by akaariai 2 years ago.
A minor cleanup to unique_field_with_mti.diff

Download all attachments as: .zip

Change History (10)

Changed 2 years ago by ungenio

Test and fix.

comment:1 Changed 2 years ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Confirmed and attached is a cleaned up version of the patch. I think this is ready for commit, but I am not going to tick that box as I have modified the patch.

Changed 2 years ago by akaariai

A minor cleanup to unique_field_with_mti.diff

comment:2 Changed 2 years ago by claudep

  • Triage Stage changed from Accepted to Ready for checkin

Looks fine, thanks.

comment:3 Changed 2 years ago by akaariai

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

In [17920]:

Fixed #17615 -- Corrected unique field validation when using multitable inheritance. The validation used wrong pk value if the parent and child model had different pk fields. Thanks ungenio for the report and patch.

comment:4 Changed 16 months ago by anonymous

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to new
  • Version changed from 1.3 to 1.5

Got an error:
AttributeError: 'NoneType' object has no attribute 'attname'

Because a model can be 'abstract = True' doesn't have a pk, but a form (also 'abstract = True') can be used for validation 'is_valid()'. I use it for Ajax validation of a single item in a form with multiple items.

row 823-825:
# model_class_pk = self._get_pk_val(model_class._meta)
# if not self._state.adding and model_class_pk is not None:
# qs = qs.exclude(pk=model_class_pk)

comment:5 Changed 16 months ago by ramiro

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

Please open a new ticket with a full and understandable description of what you are reporting. Thanks.

Last edited 16 months ago by ramiro (previous) (diff)

comment:6 follow-up: Changed 16 months ago by johan.holman@…

  • Resolution fixed deleted
  • Status changed from closed to new

I'm sorry didn't find the time yesterday to create a complete example, but here it is:

a table with some values:

CREATE TABLE test_contact (
  `id` int(11) NOT NULL auto_increment,
  `subject` varchar(64) NOT NULL,
  `email` varchar(64) NOT NULL,
  PRIMARY KEY  (`id`),
  UNIQUE KEY `subject` (`subject`)
) ENGINE=InnoDB;
INSERT INTO test_contact VALUES (1,'aaa','aaa@test.com');
INSERT INTO test_contact VALUES (2,'bbb','bbb@test.com');
INSERT INTO test_contact VALUES (3,'ccc','ccc@test.com');

python manage.py shell

import django
django.get_version()
'1.5'

from django.db import models

class ContactSubjectManager(models.Manager):
   def get_query_set(self):
       return Contact.objects.values('id','subject').all()

class ContactSubject(models.Model):
    subject = models.CharField(max_length=64, unique=True)
    objects = ContactSubjectManager()
    class Meta:
        abstract = True
        app_label = 'test'

class ContactEmailManager(models.Manager):
   def get_query_set(self):
       return Contact.objects.values('id','email').all()

class ContactEmail(models.Model):
    email = models.CharField(max_length=64)
    objects = ContactEmailManager()
    class Meta:
        abstract = True
        app_label = 'test'

class Contact(ContactSubject,ContactEmail):
    objects = models.Manager()
    class Meta:
        app_label = 'test'
    def __unicode__(self):
        return u'%s - %s' % (self.subject, self.email)

from django import forms

class ContactSubjectForm(forms.ModelForm):
    subject = forms.RegexField(
        regex          = r'^[ 0-9a-zA-Z()-]+$',
        max_length     = 30,
        min_length     = 3,
        error_messages = {'invalid':  u'Please enter a valid subject.'})
    class Meta:
        abstract = True
        model = ContactSubject

class ContactEmailForm(forms.ModelForm):
    email = forms.EmailField(
        error_messages = {'invalid':  u'Please enter a valid email address.'})
    class Meta:
        abstract = True
        model = ContactEmail

class ContactForm(ContactSubjectForm,ContactEmailForm):
    class Meta:
        model = Contact

f = Contact.objects.all()
f
[<Contact: aaa - aaa@test.com>, <Contact: bbb - bbb@test.com>, <Contact: ccc - ccc@test.com>]

data = { 'subject': 'aaa' , 'email' : 'aaa@test.com' }
f = ContactForm(data)
f.is_valid()
False
f.errors
{'subject': [u'Contact with this Subject already exists.']}

data = { 'subject': 'aaa'}
f = ContactSubjectForm(data)
f.is_valid()
Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "…/lib/python2.7/site-packages/django/forms/forms.py", line 126, in is_valid
    return self.is_bound and not bool(self.errors)
  File "…/lib/python2.7/site-packages/django/forms/forms.py", line 117, in _get_errors
    self.full_clean()
  File "…l/lib/python2.7/site-packages/django/forms/forms.py", line 274, in full_clean
    self._post_clean()
  File "…/lib/python2.7/site-packages/django/forms/models.py", line 344, in _post_clean
    self.validate_unique()
  File "…/lib/python2.7/site-packages/django/forms/models.py", line 353, in validate_unique
    self.instance.validate_unique(exclude=exclude)
  File "…/lib/python2.7/site-packages/django/db/models/base.py", line 731, in validate_unique
    errors = self._perform_unique_checks(unique_checks)
  File "…/lib/python2.7/site-packages/django/db/models/base.py", line 823, in _perform_unique_checks
    model_class_pk = self._get_pk_val(model_class._meta)
  File "…/lib/python2.7/site-packages/django/db/models/base.py", line 466, in _get_pk_val
    return getattr(self, meta.pk.attname)
AttributeError: 'NoneType' object has no attribute 'attname'

The output should be:

f.is_valid()
False
f.errors
{'subject': [u'Contact subject with this Subject already exists.']}

comment:7 Changed 16 months ago by johan.holman@…

Solution:

In function _perform_unique_checks "lib/python2.7/site-packages/django/db/models/base.py" remove comment on lines 817-822

            # Exclude the current object from the query if we are editing an
            # instance (as opposed to creating a new one)
            # Note that we need to use the pk as defined by model_class, not
            # self.pk. These can be different fields because model inheritance
            # allows single model to have effectively multiple primary keys.
            # Refs #17615.

remove code lines:

823            model_class_pk = self._get_pk_val(model_class._meta)
824            if not self._state.adding and model_class_pk is not None:
825                qs = qs.exclude(pk=model_class_pk)

add these 2 lines (same as in Django 1.4)

            if not self._state.adding and self.pk is not None:
                qs = qs.exclude(pk=self.pk)

comment:8 in reply to: ↑ 6 Changed 16 months ago by ptone

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

Replying to johan.holman@…:

I'm sorry didn't find the time yesterday to create a complete example, but here it is:

For our workflow - please open this in a new ticket with your two comments summarized in the description. You can then cross link to it being related to this ticket by putting the ticket number #17615 in the description. The original ticket was closed, and we generally don't revise committed fixes by reopening tickets. Thanks.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.