Re: [Django] #12288: Validate that values in INSTALLED_APPS are unique (was: Application name must be unique in a project - should be emphasized in docs)
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
Updating summary per Russ's last comment.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:15>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by susan):
@Timo, I'm fairly new to writing tests in Django. Should the new test be
added in tests/settings_tests?
In principle, the test would look like this (and feel free to correct me
if I'm wrong):
{{{
from settings import INSTALLED_APPS
def test_unique_installed_apps(TestCase):
self.assertEqual(len(INSTALLED_APPS), len(set(INSTALLED_APPS))
}}}
I'm not quite sure of the package names and the details that this test
would require. What are your thoughts?
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
This would be a new feature (validating that the values in a user's
`settings.INSTALLED_APPS` are unique), not just a test. I'm not sure where
the best place to put this new code would be.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by susan):
Perhaps I should ask on the core django mentorship list, of which Jeremy
is in charge of?
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:18>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by charettes):
As answered on [https://groups.google.com/d/msg/django-core-
mentorship/pv824RuH_IQ/W2bKS60lXkUJ Django Core Membership] (message has
been deleted for no reasons):
There's already a check in `django.conf.Settings` to make sure
`INSTALLED_APPS` is not a string so I'd suggest you add this check at the
same place.
Concerning the tests I suggest you take a look at the `settings_tests`
test application. However you might have to move the check to
`BaseSettings.__setattr__` if you want to use the testing strategy used in
`settings_tests.TrailingSlashURLTests`.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by susan):
Thanks for the advice. I understand now. I have an additional question
related to debugging. I often like to put print statements to check the
values and the variable types. Is there a way to do that in the django
project? For example, in django/conf/__init__.py, I want to print out the
result of line 124: `mod = importlib.import_module(self.SETTINGS_MODULE)`
Are there debugging tools I should be aware of that can be particularly
helpful for this project?
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by susan):
I've made a PR. This PR is in initial stages of development:
https://github.com/django/django/pull/1445 I haven't tested any of this
code that I wrote. It's a work in progress.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:21>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
Regarding debugging, if you put a print statement in there, that code will
be run when using `runserver` for example.
The logic in the PR looks correct. As Simon said, if you put the logic in
`BaseSettings.__setattr__`, you should be able to use a similar testing
style as `settings_tests.TrailingSlashURLTests`.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: nobody
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by susan):
Updated the PR based on the feedback here. Unit tests pass. Let me know if
there's anything I left out or forgot to consider. PR is here:
https://github.com/django/django/pull/1445/files
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:23>
Django
-------------------------------------+-------------------------------------
<zimnyx@…> | Status: assigned
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* owner: nobody => susan
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"2ac89012d8ff750ea5443b6f6f347dacb697e059"]:
{{{
#!CommitTicketReference repository=""
revision="2ac89012d8ff750ea5443b6f6f347dacb697e059"
Fixed #12288 -- Added unique validation for INSTALLED_APPS
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by dominicrodger):
There's a typo in the test class name - tiny pull request at
https://github.com/django/django/pull/1461.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:26>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"9c711ee3a6a638add26d19dad70447c981371598"]:
{{{
#!CommitTicketReference repository=""
revision="9c711ee3a6a638add26d19dad70447c981371598"
Fixed test failures on Python 3 - refs #12288
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:27>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
Type: | Version: master
Cleanup/optimization | Resolution:
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: fixed =>
Comment:
Looking at the [https://docs.djangoproject.com/en/dev/ref/settings
/#installed-apps documentation], it appears we didn't make the validation
strict enough: "The application names (that is, the final dotted part of
the path to the module containing models.py) defined in INSTALLED_APPS
must be unique. For example, you can’t include both django.contrib.auth
and myproject.auth in INSTALLED_APPS."
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:28>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* easy: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:29>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: new
Type: | Version: master
Cleanup/optimization | Resolution:
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* cc: e0ne (added)
Comment:
I've updated tests and fix for this ticket in
https://github.com/django/django/pull/1626
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:30>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"c1ec08998d1b690855d5e69a1f4d9d2f01d44ae6"]:
{{{
#!CommitTicketReference repository=""
revision="c1ec08998d1b690855d5e69a1f4d9d2f01d44ae6"
Fixed #12288 -- Validated that app names in INSTALLED_APPS are unique
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:31>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by loic84):
Latest commit is very much backward incompatible and is IMO a bad idea.
I can't run master anymore, and fixing it would require renaming tons of
packages.
"X.Y.Z" is *not* the same as "A.B.Z".
Also lot of third party apps would now be conflicting with each other,
particularly the namespaced ones like "mezzanine" or "armstrong".
What's the gain exactly? I've been using this for years and never
encountered any issues.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:32>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
Django assumes in some places that app_name.model_name is unique key (IIRC
things like contenttypes, permissions and app-cache assume this). I think
if you happen to have somepackage.app1.User and otherpackage.app1.User you
will have problems.
That being said backwards compatibility could be a bigger concern that
making sure the above doesn't happen.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:33>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by mjtamlyn):
Also worth noting that this patch does not actually prevent the issue
discussed - you can just as easily create a `User` model with an
`app_label` of `auth`, and have just as much of a problem. Can we not
apply this validation at the app cache level if this is what we're trying
to avoid? The first patch is good as it catches easy mistakes, the second
seems overkill. I'm not sure it's a pattern we should encourage, perhaps
print a warning on server start, but I think it should be allowed.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:34>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"886bb9d8780303b4c8f45c55e0ac0a6b644b73af"]:
{{{
#!CommitTicketReference repository=""
revision="886bb9d8780303b4c8f45c55e0ac0a6b644b73af"
Revert "Fixed #12288 -- Validated that app names in INSTALLED_APPS are
unique"
This reverts commit c1ec08998d1b690855d5e69a1f4d9d2f01d44ae6.
There are backwards compatability concerns with this.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:35>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
Type: | Version: master
Cleanup/optimization | Resolution:
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* status: closed => new
* resolution: fixed =>
* easy: 1 => 0
Comment:
Here's a discussion on the mailing list that looks into why app names have
to be unique.
https://groups.google.com/forum/#!topic/django-
developers/gzBWU_fUdgc/discussion
For now, I've reverted the latest commit given the backwards
incompatibility concerns.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:36>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* status: new => closed
* resolution: => fixed
Comment:
Closing this -- if someone feels the app cache validation described by
Marc in comment 34 is worthwhile, please open a new ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:37>
Django
-------------------------------------+-------------------------------------
Reporter: Piotr Czachur | Owner: susan
<zimnyx@…> | Status: closed
Type: | Version: master
Cleanup/optimization | Resolution: fixed
Component: Core (Other) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: sprint200912 | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
I fixed this properly in the app loading project, after introducing the
ability to relabel an application, which provides an exit path for people
who rely on the current lax behavior.
--
Ticket URL: <https://code.djangoproject.com/ticket/12288#comment:38>