Opened 5 years ago

Closed 19 months ago

Last modified 16 months ago

#12288 closed Cleanup/optimization (fixed)

Validate that values in INSTALLED_APPS are unique

Reported by: Piotr Czachur <zimnyx@…> Owned by: susan
Component: Core (Other) Version: master
Severity: Normal Keywords: sprint200912
Cc: e0ne 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 5 years ago.

Download all attachments as: .zip

Change History (39)

comment:1 Changed 5 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 5 years ago by sbj3

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

  • Has patch set
  • Needs documentation set
  • Needs tests set
  • Resolution set to fixed
  • Status changed from new to closed

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 5 years ago by sbj3

Added clarifying sentence.

comment:4 Changed 5 years ago by sbj3

  • Needs documentation unset
  • Needs tests unset
  • Resolution fixed deleted
  • Status changed from closed to reopened

Attached patch. (Closed ticket by mistake. noobi)

comment:5 Changed 5 years ago by jdunck

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 5 years ago by jdunck

  • Keywords sprint200912 added

comment:7 Changed 5 years ago by russellm

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

comment:8 Changed 5 years ago by russellm

(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 5 years ago by russellm

  • Has patch unset
  • Triage Stage changed from Ready for checkin to Accepted

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 5 years ago by russellm

  • Component changed from Documentation to Core framework

comment:11 Changed 4 years ago by mattmcc

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:12 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:13 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:14 Changed 2 years ago by aaugustin

  • Status changed from reopened to new

comment:15 Changed 22 months ago by timo

  • Summary changed from Application name must be unique in a project - should be emphasized in docs to Validate that values in INSTALLED_APPS are unique

Updating summary per Russ's last comment.

comment:16 Changed 21 months ago 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?

comment:17 Changed 21 months ago 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.

comment:18 Changed 21 months ago by susan

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

comment:19 follow-up: Changed 21 months ago by charettes

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 21 months ago 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?

comment:21 Changed 21 months ago 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.

Version 0, edited 21 months ago by susan (next)

comment:22 Changed 21 months ago 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.

comment:23 Changed 21 months ago 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

comment:24 Changed 21 months ago by susan

  • Owner changed from nobody to susan
  • Status changed from new to assigned

comment:25 Changed 21 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 2ac89012d8ff750ea5443b6f6f347dacb697e059:

Fixed #12288 -- Added unique validation for INSTALLED_APPS

comment:26 Changed 21 months ago by dominicrodger

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

comment:27 Changed 21 months ago by Tim Graham <timograham@…>

In 9c711ee3a6a638add26d19dad70447c981371598:

Fixed test failures on Python 3 - refs #12288

comment:28 Changed 20 months ago by timo

  • Resolution fixed deleted
  • Status changed from closed to new

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 20 months ago by timo

  • Easy pickings set

comment:30 Changed 20 months ago by e0ne

  • Cc e0ne added

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

comment:31 Changed 20 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In c1ec08998d1b690855d5e69a1f4d9d2f01d44ae6:

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

comment:32 Changed 20 months 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 20 months ago 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.

comment:34 Changed 20 months ago 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.

comment:35 Changed 20 months 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 20 months ago by timo

  • Easy pickings unset
  • Resolution fixed deleted
  • Status changed from closed to new

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 19 months ago by timo

  • Resolution set to fixed
  • Status changed from new to closed

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 16 months ago 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.

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