Opened 9 years ago

Closed 9 years ago

#4928 closed (invalid)

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

Reported by: MarioGonzalez <gonzalemario @…> Owned by: deryck
Component: Core (Other) Version: master
Severity: Keywords:
Cc: gonzalemario@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

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.

Attachments (2)

unique_together (1.7 KB) - added by MarioGonzalez <gonzalemario @…> 9 years ago.
unique_together.diff (585 bytes) - added by MarioGonzalez <gonzalemario @…> 9 years ago.
It just needed a very little change :-)

Download all attachments as: .zip

Change History (10)

Changed 9 years ago by MarioGonzalez <gonzalemario @…>

Attachment: unique_together added

comment:1 Changed 9 years ago by Chris Beaven

Needs documentation: unset
Needs tests: unset
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed

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.

comment:2 Changed 9 years ago by 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.

comment:3 Changed 9 years ago by 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".

comment:4 Changed 9 years ago by Malcolm Tredinnick

Triage Stage: Design decision neededAccepted

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.

Changed 9 years ago by MarioGonzalez <gonzalemario @…>

Attachment: unique_together.diff added

It just needed a very little change :-)

comment:5 Changed 9 years ago by MarioGonzalez <gonzalemario @…>

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

comment:6 Changed 9 years ago by MarioGonzalez <gonzalemario @…>

Is this ticket going to be added to the trunk?

comment:7 Changed 9 years ago by deryck

Owner: changed from nobody to deryck
Status: newassigned

comment:8 Changed 9 years ago by deryck

Resolution: invalid
Status: assignedclosed

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.

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