Opened 13 months ago

Closed 9 months ago

#23338 closed New feature (fixed)

Warn when unique=True is set on ForeignKey

Reported by: funkybob Owned by: Tim Graham <timograham@…>
Component: Core (System checks) Version: master
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 Changed 13 months ago by jarshwah

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 12 months ago by JLinden

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

comment:3 Changed 12 months ago by evildmp

  • Keywords afraid-to-commit added

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.

comment:5 Changed 11 months ago by diegoguimaraes

  • Needs documentation unset
  • Needs tests unset

New pull request with tests and updated documentation at https://github.com/django/django/pull/3262

comment:6 Changed 11 months ago 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

Last edited 11 months ago by diegoguimaraes (previous) (diff)

comment:7 Changed 11 months ago 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.

comment:8 Changed 11 months ago by carljm

Thanks Tim, I should have posted that here instead.

comment:9 Changed 10 months ago by timgraham

  • Easy pickings unset
  • Has patch set
  • Patch needs improvement set

comment:10 Changed 10 months ago by JLinden

  • Owner JLinden deleted
  • Status changed from assigned to new

comment:11 Changed 10 months ago by timgraham

  • Keywords afraid-to-commit removed
  • Patch needs improvement unset

comment:12 Changed 9 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

comment:13 Changed 9 months ago by Tim Graham <timograham@…>

  • Owner set to Tim Graham <timograham@…>
  • Resolution set to fixed
  • Status changed from new to closed

In 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.

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