Opened 7 years ago

Closed 3 years ago

Last modified 3 years ago

#12288 closed Cleanup/optimization (fixed)

Validate that values in INSTALLED_APPS are unique

Reported by: Piotr Czachur <zimnyx@…> Owned by: Susan Tan
Component: Core (Other) Version: master
Severity: Normal Keywords: sprint200912
Cc: Ivan Kolodyazhny Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Docs for INSTALLED_APPS says:
"A tuple of strings designating all applications that are enabled in this Django installation. Each string should be a full Python path to a Python package that contains a Django application, as created by django-admin.py startapp."
Fact that application name must be unique in INSTALLED_APPS otherwise Django will go crazy isn't mentioned at all.
It really should be in bold.

Attachments (1)

settings.diff (844 bytes) - added by sbj3 7 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 7 years ago by Russell Keith-Magee

Triage Stage: UnreviewedAccepted

There may also be some room validate this formally on startup, but at the very least, this should be documented.

Changed 7 years ago by sbj3

Attachment: settings.diff added

comment:2 in reply to:  description Changed 7 years ago by sbj3

Has patch: set
Needs documentation: set
Needs tests: set
Resolution: fixed
Status: newclosed

Replying to Piotr Czachur <zimnyx@gmail.com>:

Docs for INSTALLED_APPS says:
"A tuple of strings designating all applications that are enabled in this Django installation. Each string should be a full Python path to a Python package that contains a Django application, as created by django-admin.py startapp."
Fact that application name must be unique in INSTALLED_APPS otherwise Django will go crazy isn't mentioned at all.
It really should be in bold.

comment:3 Changed 7 years ago by sbj3

Added clarifying sentence.

comment:4 Changed 7 years ago by sbj3

Needs documentation: unset
Needs tests: unset
Resolution: fixed
Status: closedreopened

Attached patch. (Closed ticket by mistake. noobi)

comment:5 Changed 7 years ago by Jeremy Dunck

Triage Stage: AcceptedReady for checkin

comment:6 Changed 7 years ago by Jeremy Dunck

Keywords: sprint200912 added

comment:7 Changed 7 years ago by Russell Keith-Magee

(In [13219]) Refs #12288 -- Clarified that application names must be unique. Thanks to Piotr Czachur for the report.

comment:8 Changed 7 years ago by Russell Keith-Magee

(In [13224]) [1.1.X] Refs #12288 -- Clarified that application names must be unique. Thanks to Piotr Czachur for the report.

Backport of r13219 from trunk.

comment:9 Changed 7 years ago by Russell Keith-Magee

Has patch: unset
Triage Stage: Ready for checkinAccepted

The uniqueness has now been reinforced in docs; I'm leaving this ticket open as a placeholder for the need for better validation of the contents of INSTALLED_APPS

comment:10 Changed 7 years ago by Russell Keith-Magee

Component: DocumentationCore framework

comment:11 Changed 6 years ago by Matt McClanahan

Severity: Normal
Type: Cleanup/optimization

comment:12 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:13 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:14 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:15 Changed 3 years ago by Tim Graham

Summary: Application name must be unique in a project - should be emphasized in docsValidate that values in INSTALLED_APPS are unique

Updating summary per Russ's last comment.

comment:16 Changed 3 years ago by Susan Tan

@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?

comment:17 Changed 3 years ago by Tim Graham

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.

comment:18 Changed 3 years ago by Susan Tan

Perhaps I should ask on the core django mentorship list, of which Jeremy is in charge of?

comment:19 Changed 3 years ago by Simon Charette

As answered on 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.

comment:20 in reply to:  19 Changed 3 years ago by Susan Tan

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?

comment:21 Changed 3 years ago by Susan Tan

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.

Version 0, edited 3 years ago by Susan Tan (next)

comment:22 Changed 3 years ago by Tim Graham

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.

comment:23 Changed 3 years ago by Susan Tan

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

comment:24 Changed 3 years ago by Susan Tan

Owner: changed from nobody to Susan Tan
Status: newassigned

comment:25 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 2ac89012d8ff750ea5443b6f6f347dacb697e059:

Fixed #12288 -- Added unique validation for INSTALLED_APPS

comment:26 Changed 3 years ago by Dominic Rodger

There's a typo in the test class name - tiny pull request at https://github.com/django/django/pull/1461.

comment:27 Changed 3 years ago by Tim Graham <timograham@…>

In 9c711ee3a6a638add26d19dad70447c981371598:

Fixed test failures on Python 3 - refs #12288

comment:28 Changed 3 years ago by Tim Graham

Resolution: fixed
Status: closednew

Looking at the 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."

comment:29 Changed 3 years ago by Tim Graham

Easy pickings: set

comment:30 Changed 3 years ago by Ivan Kolodyazhny

Cc: Ivan Kolodyazhny added

I've updated tests and fix for this ticket in https://github.com/django/django/pull/1626

comment:31 Changed 3 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In c1ec08998d1b690855d5e69a1f4d9d2f01d44ae6:

Fixed #12288 -- Validated that app names in INSTALLED_APPS are unique

comment:32 Changed 3 years ago 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.

comment:33 Changed 3 years ago by Anssi Kääriäinen

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.

comment:34 Changed 3 years ago by Marc Tamlyn

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.

comment:35 Changed 3 years ago by Tim Graham <timograham@…>

In 886bb9d8780303b4c8f45c55e0ac0a6b644b73af:

Revert "Fixed #12288 -- Validated that app names in INSTALLED_APPS are unique"

This reverts commit c1ec08998d1b690855d5e69a1f4d9d2f01d44ae6.

There are backwards compatability concerns with this.

comment:36 Changed 3 years ago by Tim Graham

Easy pickings: unset
Resolution: fixed
Status: closednew

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.

comment:37 Changed 3 years ago by Tim Graham

Resolution: fixed
Status: newclosed

Closing this -- if someone feels the app cache validation described by Marc in comment 34 is worthwhile, please open a new ticket.

comment:38 Changed 3 years ago by Aymeric Augustin

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.

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