[Django] #26085: contenttypes.views.shortcut fails if a FK to Site returns None
Django
-------------------------------+--------------------
Reporter: jaap3 | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
The `contenttypes.views.shortcut` assumes that a FK to Site is never
nullable. Causing it to raise an `AttributeError` when the field actually
is null. This is happens e.g. when clicking the "view on site" link in the
admin on an object that has an empty Site relation.
{{{
# Next, look for a many-to-one relationship to Site.
if object_domain is None:
for field in obj._meta.fields:
if field.rel and field.rel.to is Site:
try:
object_domain = getattr(obj, field.name).domain
except Site.DoesNotExist:
pass
}}}
Either explicitly checking that the result of the `getattr` call is not
`None` or catching a `AttributeError` will fix this.
--
Ticket URL: <https://code.djangoproject.com/ticket/26085>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* needs_better_patch: => 0
* component: Uncategorized => contrib.contenttypes
* needs_tests: => 0
* easy: 0 => 1
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:1>
Django
Reporter: jaap3 | Owner: nobody
Type: Bug | Status: new
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by jibaku):
Created a PR. Feedback needed, it's my first PR to Django.
https://github.com/django/django/pull/5986
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:2>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* owner: nobody => jibaku
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:3>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/5986 PR] (test failures)
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:4>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* cc: daniponi (added)
* needs_better_patch: 1 => 0
Comment:
Created a PR to fix the test problem as suggested by claudep.
[https://github.com/django/django/pull/6427]
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:5>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
Comment:
Left some comments for improvement on the updated PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:6>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:7>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by jaap3):
Sorry, I'm of the opinion that raising a 404 is the wrong solution. All
other exceptions that are caught (IndexError, Site.DoesNotExist) are
silently ignored, I think the same should be done for a None value.
The reason I stumbled upon this issue in the first place is that in my use
case objects with no explicit site relation are available on all sites. So
raising a 404 would be not be much of an improvement over raising a 500
;-).
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:8>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
Comment:
Makes sense.
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:9>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 1 => 0
Comment:
Changed the PR according to jaap3's suggestion
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:10>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
Component: contrib.contenttypes | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* needs_better_patch: 0 => 1
Comment:
Left a few cosmetic comments and tests aren't passing on databases other
than SQLite.
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:11>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: assigned
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:12>
Django
Reporter: jaap3 | Owner: jibaku
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d29d11b026e8acea4778dd596aaf82d55b11f44d" d29d11b]:
{{{
#!CommitTicketReference repository=""
revision="d29d11b026e8acea4778dd596aaf82d55b11f44d"
Fixed #26085 -- Fixed contenttypes shortcut() view crash with a null fk to
Site.
Thanks Fabien Schwob for the initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:13>
Django
Reporter: jaap3 | Owner: jibaku
Type: Bug | Status: closed
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"2a9bcb503f2aa1b7462174cf95852a2aafbe90ec" 2a9bcb5]:
{{{
#!CommitTicketReference repository=""
revision="2a9bcb503f2aa1b7462174cf95852a2aafbe90ec"
Refs #26085, #11505 -- Cleared Site cache in contenttypes_tests.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26085#comment:14>