Opened 9 years ago

Closed 8 years ago

Last modified 5 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@… 9 years ago.
4562_tests.diff (968 bytes) - added by Philippe Raoult 9 years ago.
tests unique_together with models

Download all attachments as: .zip

Change History (18)

Changed 9 years ago by calvin@…

comment:1 Changed 9 years ago by Chris Beaven

Component: UncategorizedAdmin interface
Owner: changed from Jacob to Adrian Holovaty
Triage Stage: UnreviewedDesign 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 9 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 9 years ago by Luke Plant

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

comment:4 Changed 9 years ago by Jacob

Triage Stage: Design decision neededAccepted

Changed 9 years ago by Philippe Raoult

Attachment: 4562_tests.diff added

tests unique_together with models

comment:5 Changed 9 years ago by Philippe Raoult

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 Changed 9 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 9 years ago by trbs

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

comment:8 Changed 9 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 9 years ago by trbs

Cc: v.oostveen@… added

comment:10 Changed 9 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 8 years ago by Gary Wilson

milestone: 1.0

comment:12 Changed 8 years ago by Malcolm Tredinnick

Owner: changed from nobody to Malcolm Tredinnick

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

Cc: bronger@… added

comment:14 Changed 8 years ago by Jacob

Owner: changed from Malcolm Tredinnick to Jacob

comment:15 Changed 8 years ago by Jacob

Resolution: wontfix
Status: newclosed

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 5 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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