Opened 9 years ago

Closed 5 years ago

#1021 closed defect (fixed)

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

Reported by: Ian@… Owned by: nobody
Component: Validators Version: master
Severity: major Keywords: model-validation
Cc: mpjung@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

something like this should be in the default framework whenever someone has a 'unique_together' meta option in their model

    def _manipulator_validate_name(self, field_data, all_data):
          from django.core import validators
          if str(self.__class__) == "django.models.tags.TagManipulatorAdd":          
            cursor= db.cursor()                                                      
            cursor.execute("""                                                       
                SELECT 1 FROM conf_tags WHERE name=%s and type_id=%s LIMIT 1         
                """                                                                  
                ,[ all_data['name'], all_data['type'] ])                             
            if cursor.fetchone():                                                    
              raise validators.ValidationError, "A tag with this name and type already exists."

Attachments (2)

django-manipulators-fix-ticket1021.patch (619 bytes) - added by wam@… 8 years ago.
fix for unique_together
scm-4828-unique-together-might-be-null.diff (761 bytes) - added by David Danier <goliath.mailinglist@…> 8 years ago.
Patch for unique_together containing fields that might be NULL, (, None) could perhaps be replaced with EMPTY_VALUES from newforms

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by wam@…

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.

comment:2 Changed 9 years ago by wam@…

  • priority changed from normal to high

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.

comment:3 Changed 8 years ago by wam@…

  • Severity changed from normal to major
  • Summary changed from unique_together should check the uniqueness in the validation phase to [PATCH] unique_together should check the uniqueness in the validation phase
  • Version set to 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).

Changed 8 years ago by wam@…

fix for unique_together

comment:4 Changed 8 years ago by jacob

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

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

comment:5 Changed 8 years ago by mpjung@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:6 Changed 8 years ago by anonymous

  • Cc mpjung@… added

comment:7 Changed 8 years ago by SmileyChris

  • Has patch set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

Changed 8 years ago by David Danier <goliath.mailinglist@…>

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

comment:8 Changed 7 years ago by mrts

  • Component changed from Core framework to Validators
  • Keywords model-validation added
  • milestone set to 1.0 beta

Tagging as model-validation.

comment:9 Changed 7 years ago by jacob

  • milestone changed from 1.0 beta to post-1.0

comment:10 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:11 Changed 5 years ago by Honza_Kral

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

With model-validation merged, this is fixed in [12098]

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