Opened 11 years ago
Closed 11 years ago
#23338 closed New feature (fixed)
Warn when unique=True is set on ForeignKey
| Reported by: | Curtis Maloney | Owned by: | |
|---|---|---|---|
| Component: | Core (System checks) | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
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?
Change History (13)
comment:1 by , 11 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 11 years ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
comment:3 by , 11 years ago
| Keywords: | afraid-to-commit added |
|---|
comment:5 by , 11 years ago
| Needs documentation: | unset |
|---|---|
| Needs tests: | unset |
New pull request with tests and updated documentation at https://github.com/django/django/pull/3262
comment:6 by , 11 years ago
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
comment:7 by , 11 years ago
On the PR Carl says,
Is this check worth adding? A unique
ForeignKeyworks just fine and doesn't hurt anyone. IfForeignKey(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 theOneToOneFieldaccessor 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.
comment:9 by , 11 years ago
| Easy pickings: | unset |
|---|---|
| Has patch: | set |
| Patch needs improvement: | set |
comment:10 by , 11 years ago
| Owner: | removed |
|---|---|
| Status: | assigned → new |
comment:11 by , 11 years ago
| Keywords: | afraid-to-commit removed |
|---|---|
| Patch needs improvement: | unset |
comment:12 by , 11 years ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
comment:13 by , 11 years ago
| Owner: | set to |
|---|---|
| Resolution: | → fixed |
| Status: | new → closed |
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.