unique_together bug when it's not a tuple. Validation missing

[MarioGonzalez <gonzalemario @…>]

if unique_together is a tuple with strings, Django will manage the content as characters. So I wrote a patch to validate that the unique_together content should be a tuple of tuples.

[Chris Beaven]

The current patch uses type(x) == ... when it should be using isinstance(x, (list, tuple)). New error wording could be polished a bit too.

Pushing to design decision to see if the idea in general is deemed worthy.

[James Bennett]

Personally, if we're going to type-check the value I think we should take the opposite approach -- use if isinstance(val, basestring) to only catch things that are strings. Limiting to a tuple or a list, while it fits with common practice, does limit the types of iterable things people could use in unique_together, and I wouldn't want to do that needlessly.

[MarioGonzalez <gonzalemario @…>]

Hi ubernostrum, I think you've got a good point of view. However almost every setting in Django that needs more than one value, use tuples or lists; if all should be a string, how do you transform unique_together = (('field1', 'field2'),)? ex: unique_together="field1, field2" or unique_together="'field1, field2' 'field1, field3'"

I mean, I think it's hard (not impossible) not use lists or tuples in those situations and also IMHO that's "Design decision".

[Malcolm Tredinnick]

I agree with James, the error checking is backwards here (and the above comment misunderstand the point James is making I suspect). Do a check like

if isinstance(val, basestring):
    raise TypeError, '....'

and then no further indenting is needed for the common case. It wasn't immediately clear from the bug description, but I gather you are just trying to work around the case of when people use a string when a tuple is required. So test for that one case, instead of ruling out iterators that are permissible.

If it's a simple check like this, catching a common error case, we can include it.

[MarioGonzalez <gonzalemario @…>]

It just needed a very little change :-)

[MarioGonzalez <gonzalemario @…>]

Also, please change the error message. I'm not a English native speaker but I think you have noticed this :-)

[MarioGonzalez <gonzalemario @…>]

Is this ticket going to be added to the trunk?

[deryck]

Closing as invalid since existing validation will error on the case that the field name doesn't exist. In others words, the following already fails validation...

unique_together = 'foo, bar'

or

unique_together = ('foo', 'bar')

so why should we test for the single case of unique_together being a string?

However, we should probably allow the single tuple, rather than the tuple of tuples for convenience. I've opened #5460 for this new issue.