[PATCH] unique_together should check the uniqueness in the validation phase

I just encountered this bug/deficency in some code I wrote that uses unique_together and is populated in a generic view. Very disconcerting to see

OperationalError at [URL_DELETED]
columns COL1, COL2 are not unique

when I would have expected the transaction to have simply generated a uniqueness error which could have been handled by the view and thus able to be gracefully handled in the template (such as what occurs if unique=True is added onto the individual model attributes).

Personally, I would have rated this as a 'major' severity bug, as it breaks the advertised behavior of generic views. I'll leave it as it was originally entered though and allow whoever picks the bug up (hopefully soon) to re-prioritize as they see fit.

I looked into this a bit more. First, the problem occurs even in the latest subversion checkout (revision 3884). In my case, the model attributes that need to be unique is a combination of two ForeignKeys?. Looking into db/models/manipulators.py I saw manipulator_validator_unique_together. I traced through the execution of the validation of my generic view create template which was generating the OperationalError? and I see saw that the validator never made it to the manager query of kwargs. It took the early return in the following block of code:

        field_val = all_data.get(f.attname, None)
        if field_val is None:
            # This will be caught by another validator, assuming the field
            # doesn't have blank=True.
            print >>debug, "5 early return"
            return

At this point in the code, all_data contains:

<MultiValueDict: {'COL1': ['2'], 'COL2': ['5']}>

(sorry, I have to scrub the column names of my app, you should get the idea though). The problem seems to be that the f.attname value that is attempted ended up being 'COL2_id', which isn't in all_data. Thus the lookup in the all_data dictionary fails, thus the validator returns prematurely, thus the insert attempt is performed, and consequently the operational error is generated at the database interface level and not at the validation level.

I don't know if this will totally break other things, but I changed the statement:

field_val = all_data.get(f.attname, None)

to

field_val = all_data.get(f.name, None)

and my OperationalError? traceback turned into a nicely formatted validation error in my generic view. I'm now bumping thec priority of this to 'High' since the solution has been solved (hopefully) and just needs to be applied into SVN.

This has been working well for us for the past 3 weeks. I'm updating the summary to include the fact that it's a patch, bumping priority up to high (as the status quo is broken) and changing the component to database wrapper (although it might legitimately belong as a Validator issue).

fix for unique_together

(In [4028]) Fixed #1021: unique_together should now get validated correctly. Thanks, wam@cisco.com

I got model with three fields that are in a unique_together constraint. The company can be null as not every user is associated with a company.

class User(models.Model):
    username = models.CharField(maxlength=50)
    role = models.CharField(maxlength=10)
    company = models.ForeignKey(Company, blank=True, null=True)
    class Meta:
        unique_together = (('username', 'company', 'role'),)

When using a ChangeManipulator? to change a User with a blank company field the manipulator_validator_unique_together fails:

Traceback (most recent call last):

  File "/usr/local/lib/python2.5/site-packages/django/core/handlers/base.py", line 74, in get_response
    response = callback(request, *callback_args, **callback_kwargs)

  File "/srv/mfp/django/mfp/../mfp/sysop/views.py", line 33, in wrapped
    return func(request, *args, **kwargs)

  File "/srv/mfp/django/mfp/../mfp/sysop/views.py", line 87, in user_edit
    errors = manipulator.get_validation_errors(new_data)

  File "/usr/local/lib/python2.5/site-packages/django/forms/__init__.py", line 59, in get_validation_errors
    errors.update(field.get_validation_errors(new_data))

  File "/usr/local/lib/python2.5/site-packages/django/forms/__init__.py", line 357, in get_validation_errors
    self.run_validator(new_data, validator)

  File "/usr/local/lib/python2.5/site-packages/django/forms/__init__.py", line 347, in run_validator
    validator(new_data.get(self.field_name, ''), new_data)

  File "/usr/local/lib/python2.5/site-packages/django/utils/functional.py", line 3, in _curried
    return _curried_func(*(args+moreargs), **dict(kwargs, **morekwargs))

  File "/usr/local/lib/python2.5/site-packages/django/db/models/manipulators.py", line 299, in manipulator_validator_unique_together
    old_obj = self.manager.get(**kwargs)

  File "/usr/local/lib/python2.5/site-packages/django/db/models/manager.py", line 67, in get
    return self.get_query_set().get(*args, **kwargs)

  File "/usr/local/lib/python2.5/site-packages/django/db/models/query.py", line 211, in get
    obj_list = list(clone)

  File "/usr/local/lib/python2.5/site-packages/django/db/models/query.py", line 103, in __iter__
    return iter(self._get_data())

  File "/usr/local/lib/python2.5/site-packages/django/db/models/query.py", line 430, in _get_data
    self._result_cache = list(self.iterator())

  File "/usr/local/lib/python2.5/site-packages/django/db/models/query.py", line 172, in iterator
    cursor.execute("SELECT " + (self._distinct and "DISTINCT " or "") + ",".join(select) + sql, params)

Prior Changeset 4028 this problem did not exist. Latest SVN (4152) with django/db/models/manipulators.py changed back to pre-4028 works, too.

I know that a null company disables the constraint in the DB. That's why I use a custom validator for the case that the company field was left empty and also added a unique index to my database. Just in case you were wondering how that index looks like:

CREATE UNIQUE INDEX shop_user_username_and_role_unique on shop_user(username,"role") where company_id is null;

It would be nice if the manipulator_validator_unique_together would support such constraint out of the box, even though it's a bit out of scope for this ticket.

Patch for unique_together containing fields that might be NULL, (, None) could perhaps be replaced with EMPTY_VALUES from newforms