Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#26085 closed Bug (fixed)

contenttypes.views.shortcut fails if a FK to Site returns None

Reported by: Jaap Roes Owned by: Fabien Schwob
Component: contrib.contenttypes Version: master
Severity: Normal Keywords:
Cc: dani poni Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

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.

Change History (14)

comment:1 Changed 4 years ago by Tim Graham

Component: Uncategorizedcontrib.contenttypes
Easy pickings: set
Triage Stage: UnreviewedAccepted
Type: UncategorizedBug

comment:2 Changed 4 years ago by Fabien Schwob

Created a PR. Feedback needed, it's my first PR to Django.
https://github.com/django/django/pull/5986

comment:3 Changed 4 years ago by Fabien Schwob

Owner: changed from nobody to Fabien Schwob
Status: newassigned

comment:4 Changed 4 years ago by Claude Paroz

Has patch: set
Patch needs improvement: set

PR (test failures)

comment:5 Changed 4 years ago by dani poni

Cc: dani poni added
Patch needs improvement: unset

Created a PR to fix the test problem as suggested by claudep.
https://github.com/django/django/pull/6427

comment:6 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Left some comments for improvement on the updated PR.

comment:7 Changed 4 years ago by Tim Graham

Patch needs improvement: unset

comment:8 Changed 4 years ago by Jaap Roes

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 ;-).

comment:9 Changed 4 years ago by Claude Paroz

Patch needs improvement: set

Makes sense.

comment:10 Changed 4 years ago by dani poni

Patch needs improvement: unset

Changed the PR according to jaap3's suggestion

comment:11 Changed 4 years ago by Tim Graham

Patch needs improvement: set

Left a few cosmetic comments and tests aren't passing on databases other than SQLite.

comment:12 Changed 4 years ago by Tim Graham

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 Changed 4 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In d29d11b:

Fixed #26085 -- Fixed contenttypes shortcut() view crash with a null fk to Site.

Thanks Fabien Schwob for the initial patch.

comment:14 Changed 4 years ago by Tim Graham <timograham@…>

In 2a9bcb5:

Refs #26085, #11505 -- Cleared Site cache in contenttypes_tests.

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