Code

Opened 7 years ago

Closed 7 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 @…> 7 years ago.
unique_together.diff (585 bytes) - added by MarioGonzalez <gonzalemario @…> 7 years ago.
It just needed a very little change :-)

Download all attachments as: .zip

Change History (10)

Changed 7 years ago by MarioGonzalez <gonzalemario @…>

comment:1 Changed 7 years ago by SmileyChris

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design 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 7 years ago by ubernostrum

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 7 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 7 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

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 7 years ago by MarioGonzalez <gonzalemario @…>

It just needed a very little change :-)

comment:5 Changed 7 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 7 years ago by MarioGonzalez <gonzalemario @…>

Is this ticket going to be added to the trunk?

comment:7 Changed 7 years ago by deryck

  • Owner changed from nobody to deryck
  • Status changed from new to assigned

comment:8 Changed 7 years ago by deryck

  • Resolution set to invalid
  • Status changed from assigned to closed

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.

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.