[Django] #31802: Validation for "SITE_ID" setting
Django
-----------------------------------------+------------------------------
Reporter: Gagan Deep | Owner: nobody
Type: Bug | Status: new
Component: contrib.sites | Version: 3.0
Severity: Normal | Keywords: sites, cache
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-----------------------------------------+------------------------------
The "sites" framework requires to set a `SITE_ID` to specify the database
ID of the Site object associated with that particular settings file. From
the documentation, it can be interpreted that it should be an integer.
But, it can also be set to a string literal('1'). I know this is wrong,
but the Django accepted it and worked with it anyway without showing any
warning or error.
The problem arose with `SITE_CACHE`. It was not getting invalidated when
the Site object is updated. The reason is the code which handles
invalidation uses primary key of instance object which will be an
integer. Therefore, the cache is never invalidated.
{{{
try:
del SITE_CACHE[instance.pk]
except KeyError:
pass
}}}
[https://github.com/django/django/blob/3d16496037fbb8a6bbc6b6e354fa4f5eb65e6cea/django/contrib/sites/models.py#L103-L117
Please see complete code on GitHub.]
Since working of an element of Django is dependent on this setting, I
suggest that there be a validation test for `SITE_ID`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31802>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Gagan Deep | Owner: nobody
Type: Bug | Status: new
Component: contrib.sites | Version: 3.0
Keywords: sites, cache | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
Part of the problem here is that `Site.objects.get(pk=some_string)` works
because the database uses [SQL implicit type
conversion](https://adamj.eu/tech/2020/03/06/sql-implicit-type-
conversion/) to cast the string, but we need a consistent value within
Python.
If it's only the site cache that's affected we could make `SITE_CACHE` a
subclass of dict that casts keys but that seems like overengineering when
a system check would provide enough validation at the boundary.
--
Ticket URL: <https://code.djangoproject.com/ticket/31802#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: Gagan Deep | Owner: nobody
Component: contrib.sites | Version: master
Severity: Normal | Resolution:
Keywords: sites, cache | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by felixxm):
* type: Bug => Cleanup/optimization
* version: 3.0 => master
* stage: Unreviewed => Accepted
Comment:
> ... it can be interpreted that it should be an integer.
It's clearly stated in [https://docs.djangoproject.com/en/3.0/ref/settings
/#site-id docs]: ''"The ID, as an integer, ..."'' but I agree that we can
add a system check, e.g.
{{{
sites.E003: ``SITE_ID`` must be an integer.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31802#comment:2>
Django
Reporter: Gagan Deep | Owner: parth-
Type: | verma
Cleanup/optimization | Status: assigned
Component: contrib.sites | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by parth-verma):
* owner: nobody => parth-verma
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31802#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Verma
Cleanup/optimization | Status: assigned
Component: contrib.sites | Version: master
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by felixxm):
* needs_docs: 0 => 1
* has_patch: 0 => 1
* needs_tests: 0 => 1
Comment:
[https://github.com/django/django/pull/13226 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31802#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Gagan Deep | Owner: Parth
Type: | Verma
Cleanup/optimization | Status: assigned
Component: contrib.sites | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by felixxm):
* needs_docs: 1 => 0
* needs_tests: 1 => 0
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31802#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Gagan Deep | Owner: Parth
Type: | Verma
Component: contrib.sites | Version: master
Severity: Normal | Resolution: fixed
Keywords: sites, cache | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"41065cfed56d5408dd8f267b9e70089471a7f1be" 41065cf]:
{{{
#!CommitTicketReference repository=""
revision="41065cfed56d5408dd8f267b9e70089471a7f1be"
Fixed #31802 -- Added system check for non-integer SITE_ID.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31802#comment:6>