[Django] #23338: Warn when unique=True is set on ForeignKey
Django
------------------------------------------------+------------------------
Reporter: funkybob | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 1 | Needs tests: 1
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
Long ago in the dark times before 1.0, there was a bug in Admin that meant
using ForeignKey(unique=True) was preferable to using a OneToOneField.
Somehow, people (on IRC) are still doing this -- even new users-- , and I
can't figure out why.
Unless I'm missing something, there is no case where a
ForeignKey(unique=True) is preferable to a OneToOneField.
Shouldn't we, then, at least add a check/warn to say this?
--
Ticket URL: <https://code.djangoproject.com/ticket/23338>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: funkybob | Owner: nobody
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by jarshwah):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:1>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by JLinden):
* owner: nobody => JLinden
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:2>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by evildmp):
* keywords: => afraid-to-commit
Comment:
I've marked this ticket as especially suitable for people following the
'''Don't be afraid to commit tutorial''' at the DjangoCon US 2014
sprints.
If you're tackling this ticket, please don't hesitate to ask me for
guidance if you'd like any, either at the sprints themselves, or here or
on the Django IRC channels, where I can be found as ''EvilDMP''.
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:3>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by JLinden):
Pull request sent at https://github.com/django/django/pull/3196
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:4>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by diegoguimaraes):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
Comment:
New pull request with tests and updated documentation at
https://github.com/django/django/pull/3262
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:5>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by diegoguimaraes):
The patch and test are basically working, but a design decision should be
made how to handle the tests violating the unique check.
I think that would be helpful, if someone that knows better the System
Checks could take a look. I detailed the problems in the PR
.https://github.com/django/django/pull/3262
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:6>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by timgraham):
On the PR Carl says,
Is this check worth adding? A unique `ForeignKey` works just fine and
doesn't hurt anyone. If `ForeignKey(unique=True)` makes more sense to
someone, or if they just changed it to be unique and have a bunch of
existing accessor code that they don't want to have to change to
OneToOneField style, is there really any value in Django complaining at
them about it?
I think if we do add this, the warning needs to be carefully worded so as
not to imply that there's anything wrong or broken about
`ForeignKey(unique=True)`, because there isn't; it's just that they may
find the `OneToOneField` accessor slightly more convenient if they weren't
aware of that option. But to me this seems like a coding-style thing that
doesn't warrant a check warning.
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:7>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by carljm):
Thanks Tim, I should have posted that here instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:8>
Django
Reporter: funkybob | Owner: JLinden
Type: New feature | Status: assigned
Keywords: afraid-to-commit | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:9>
Django
Reporter: funkybob | Owner:
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: afraid-to-commit | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* owner: JLinden =>
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:10>
Django
Reporter: funkybob | Owner:
Type: New feature | Status: new
Component: Core (System checks) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
* keywords: afraid-to-commit =>
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:11>
Django
Reporter: funkybob | Owner:
Type: New feature | Status: new
checks) | Resolution:
Severity: Normal | Triage Stage: Ready for
Keywords: | checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:12>
Django
Reporter: funkybob | Owner: Tim
Type: New feature | Graham <timograham@…>
Component: Core (System | Status: closed
checks) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Tim Graham <timograham@…>):
* owner: => Tim Graham <timograham@…>
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"f39b0421b406b411c3bcb58e8aa415d885fea505"]:
{{{
#!CommitTicketReference repository=""
revision="f39b0421b406b411c3bcb58e8aa415d885fea505"
Fixed #23338 -- Added warning when unique=True on ForeigKey
Thanks Jonathan Lindén for the initial patch, and Tim Graham
and Gabe Jackson for the suggestions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23338#comment:13>