#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: | dev |
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 by , 9 years ago
Component: | Uncategorized → contrib.contenttypes |
---|---|
Easy pickings: | set |
Triage Stage: | Unreviewed → Accepted |
Type: | Uncategorized → Bug |
comment:2 by , 9 years ago
comment:3 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:5 by , 9 years ago
Cc: | 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 by , 9 years ago
Patch needs improvement: | set |
---|
Left some comments for improvement on the updated PR.
comment:7 by , 9 years ago
Patch needs improvement: | unset |
---|
comment:8 by , 9 years ago
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:10 by , 9 years ago
Patch needs improvement: | unset |
---|
Changed the PR according to jaap3's suggestion
comment:11 by , 9 years ago
Patch needs improvement: | set |
---|
Left a few cosmetic comments and tests aren't passing on databases other than SQLite.
comment:12 by , 9 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
Created a PR. Feedback needed, it's my first PR to Django.
https://github.com/django/django/pull/5986