Code

Opened 7 years ago

Closed 6 years ago

Last modified 3 years ago

#4562 closed (wontfix)

unique_together uses 'iexact' instead of 'exact'

Reported by: calvin@… Owned by: jacob
Component: contrib.admin Version: master
Severity: Keywords:
Cc: v.oostveen@…, bronger@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

The unique_together flag promises unique entries in a database. However the manipulator deploys case insensitive checks with "iexact" instead of "exact". The attached patch enforces the correct uniqueness check with "exact".

Attachments (2)

django_unique_together_exact.diff (1.2 KB) - added by calvin@… 7 years ago.
4562_tests.diff (968 bytes) - added by PhiR 6 years ago.
tests unique_together with models

Download all attachments as: .zip

Change History (18)

Changed 7 years ago by calvin@…

comment:1 Changed 7 years ago by SmileyChris

  • Component changed from Uncategorized to Admin interface
  • Needs documentation unset
  • Needs tests unset
  • Owner changed from jacob to adrian
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

See #4778 for an example of what the current behaviour breaks.

However, I'm not sure if this'll get much attention due to manipulators being on the "way out". Marking as DDR to confirm if it's worth fixing.

comment:2 Changed 7 years ago by rokclimb15@…

I just applied this patch to svn HEAD on one of my production servers last night. It fixed the problem and didn't seem to have any side-effects. Is anyone interested in fixing it?

comment:3 Changed 6 years ago by lukeplant

Bug #5859 depends on this, it would seem, so it's probably worth fixing.

comment:4 Changed 6 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

Changed 6 years ago by PhiR

tests unique_together with models

comment:5 Changed 6 years ago by PhiR

  • Patch needs improvement set

Allowing instances which only differ by case will currently raise an exception with mysql (with case-insensitive collation, which is the default). See attached tests.

comment:6 follow-up: Changed 6 years ago by Bastian Kleineidam <calvin@…>

It seems this is another bug. Queries like name = 'John' should be case sensitive, at least that is what Django expects. The solution whould be to use the binary keyword in mysql, like this:

select from app_users where binary name = 'John'

But this should probably discussed in a separate ticket.

comment:7 in reply to: ↑ 6 Changed 6 years ago by trbs

Or modify the backend driver for mysql to use the case-sensitive collation

comment:8 Changed 6 years ago by trbs

  • Needs documentation set

FYI: this is also related to ticket #6523 which hits the same problem specifically with PostgreSQL 8.3

Any reason still open why it cannot be applied ? It seems to me that there is no need for a better patch:

Cause i feel strongly that "just because MySQL doesn't do this" is a very weak argument for needing a better patch. That is MySQL's fault not Django's and Django should not hot-patch it's code to fix problems with MySQL, the same for any other DB that is not case-sensitive per default. Thus there should be a seperate ticket for MySQL that applies the case-sensitive collation to every table or uses the binary keyword as pointed out above by Bastian.

But i would like to raise some thoughts about the potential problems that may arise from making uniques case-sensitive...

I can imagine people that are now accustomed to having Person(first_name="John",last_name="Doe") reject Person(first_name="JOHN",last_name="DOE") where unique_together = ('first_name', 'last_name') So maybe this should be done with some careful changes to the documentation outlining case sensitivity. Possibly introducing a new parameter which gives the user a way to change how matching should be done. As normally per default you would want to have case-sensitive matching but maybe sometimes (for instance with names) you would want the fields to be case-sensitive but the matching to be case insensitive. In other words normally use exact and give the user an option to specify when to use iexact... (But this is probably more of an after-thought)

Now I'm all in favor of pushing this patch in Django and then forcing all database backend to make sure the database uses case-sensitive behavior, because i feel that the reverse case which is now happening is far worse then the above. Especially as this is currently handled ambiguously:

>>> Test.objects.all()
[<Test: john doe>]

>>> foo=Test(first_name="JOHN", last_name="DOE")
>>> foo.save()
>>> Test.objects.all()
[<Test: john doe>, <Test: JOHN DOE>]

While doing the same from the admin pages will raise ValidationError:

Test with this first name already exists for the given last name.

And after saving the object from command line or view the admin will be a little angry as it now violates the case_insensitive_unique_together constrained:

MultipleObjectsReturned at /admin/uniquetogether/test/2/
get() returned more than one Test -- it returned 2! Lookup parameters were {'last_name__iexact': u'DOE', 'first_name__iexact': u'JOHN'}

Sorry for the long comment this time :)
And enabled the needs_documentation as i feel that this change does require some documentation, either in the normal django docs or otherwise at least in the BackwardsIncompatibleChanges list.

comment:9 Changed 6 years ago by trbs

  • Cc v.oostveen@… added

comment:10 Changed 6 years ago by honeyman

By the way, should not the manipulator_validator_unique_for_date() function be changed similarly?

Too bad it is not fixed yet, cause having neither this ticket now 6523 one fixed renders unique_together options utilizing foreign keys completely unusable on PG8.3.

comment:11 Changed 6 years ago by gwilson

  • milestone set to 1.0

comment:12 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

comment:13 Changed 6 years ago by Torsten Bronger <bronger@…>

  • Cc bronger@… added

comment:14 Changed 6 years ago by jacob

  • Owner changed from mtredinnick to jacob

comment:15 Changed 6 years ago by jacob

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

Manipulators are gone, so this is wontfix. However, this is part of a bigger problem that needs addressing; see #7789, for one.

comment:16 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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.