Code

Opened 3 years ago

Last modified 3 months ago

#16368 new Bug

Custom sites model overridden by contrib.sites.model.Site

Reported by: briehan.lombaard@… Owned by: nobody
Component: contrib.sites Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

We have a custom 'sites framework' implementation that basically consists of a Site model and SiteManager. For some reason, when running our application, the Site model from django.contrib.sites is used instead of our own.

I did the following from within the app:

from myapp.sites.models import Site 
print Site.objects

<class 'django.contrib.sites.models.Site'>


When doing the same from the management shell (./manage.py shell), I get the expected output:

>>> from myapp.sites.models import Site 
>>> type(Site.objects) 

<class 'foobar.sites.models.Site'>

Our application used to run fine on Django 1.0 and subsequently on 1.1 and 1.2 during my current upgrade attempt to get the application onto version 1.3. I also tried this using the current trunk (revision 16483) and got the same result.

I'm attaching a minimal Django project that replicates this behavior on my machine. You will see that running ./manage syncdb actually creates a 'django_site' table instead of 'sites'.

Attachments (1)

foobar.tar.gz (5.1 KB) - added by briehan.lombaard@… 3 years ago.

Download all attachments as: .zip

Change History (19)

Changed 3 years ago by briehan.lombaard@…

comment:1 in reply to: ↑ description Changed 3 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Apologies for the inconsistent naming in my code examples. They should be:

from foobar.sites.models import Site 
print Site

<class 'django.contrib.sites.models.Site'>

and the expected output

>>> from foobar.sites.models import Site 
>>> print Site 

<class 'foobar.sites.models.Site'>

comment:2 Changed 3 years ago by BernhardEssl

  • Version changed from 1.3 to SVN

I can also reproduce this error (django trunk SVN-16452).

#settings.py
INSTALLED_APPS = ('xxx.sites',)
#sites/models.py
from django.db import models

class Site(models.Model):
    domain = models.CharField(unique=True, max_length=100, db_index=True)
$ python manage.py syncdb
Creating tables ...
Creating table django_site
Installing custom SQL ...
Installing indexes ...
No fixtures found.

$ mysql -uuser -p test -e"show tables;"
+----------------+
| Tables_in_test |
+----------------+
| django_site    |
+----------------+

comment:3 Changed 3 years ago by aaugustin

This is most likely because django.contrib.sites gets imported at some point, and it clashes with your app — app names must be unique.

comment:4 Changed 3 years ago by ramiro

Maybe this has the same root reason that causes #16283?. can you try undoing the changes introduced in r14563 and see if that solve your problem?

Last edited 3 years ago by ramiro (previous) (diff)

comment:5 Changed 3 years ago by BernhardEssl

I removed the code from r14563 in django/core/management/validation.py and django/utils/itercompat.py as you suggest and now it works.

+----------------+
| Tables_in_test |
+----------------+
| sites_site     |
+----------------+

comment:6 Changed 3 years ago by briehan.lombaard@…

Applying the patches from r14563 on a working copy of 1.3 does not fix the issue for me. I tried on trunk as well and that seems to leave the code in a broken state.

For what it is worth when I rename my 'sites' app to something else the problem goes away. If I'm not allowed to have a Django app called 'sites' (even when I'm not using django.contrib.sites) then surely Django needs to enforce that somehow instead of allowing me to do so and then trampling all over my imports.

comment:7 Changed 3 years ago by BernhardEssl

Strange on trunk it works fine for me without the changes in r14563. Did you undoing the changes from r14563 or patching the files with r14563?

comment:8 Changed 3 years ago by briehan.lombaard@…

I just did a patch -uR which as I understand it reverts the changes from r14563.

comment:9 Changed 3 years ago by aaugustin

It's likely that r16493 fixed this problem. Could you test it?

comment:10 Changed 3 years ago by anonymous

Thanks for the update. I just tried with r16500 and still no luck. Let me know if there is anything else I can do to help track down the problem.

comment:11 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

I still believe the root cause of the problem is that django.contrib.sites gets imported at some point. Can you figure out why? For instance, patch django/contrib/sites/models.py to raise an exception, and paste the traceback here.

comment:12 Changed 3 years ago by briehan.lombaard@…

Raising an exception from django.contrib.sites.models gave me this traceback:

..snip...
    from django.contrib.contenttypes import generic
  File "/home/briehan/.virtualenvs/django-trunk/src/django/django/contrib/contenttypes/generic.py", line 14, in <module>
    from django.contrib.admin.options import InlineModelAdmin, flatten_fieldsets
  File "/home/briehan/.virtualenvs/django-trunk/src/django/django/contrib/admin/__init__.py", line 6, in <module>
    from django.contrib.admin.sites import AdminSite, site
  File "/home/briehan/.virtualenvs/django-trunk/src/django/django/contrib/admin/sites.py", line 4, in <module>
    from django.contrib.admin.forms import AdminAuthenticationForm
  File "/home/briehan/.virtualenvs/django-trunk/src/django/django/contrib/admin/forms.py", line 4, in <module>
    from django.contrib.auth.forms import AuthenticationForm
  File "/home/briehan/.virtualenvs/django-trunk/src/django/django/contrib/auth/forms.py", line 10, in <module>
    from django.contrib.sites.models import get_current_site
  File "/home/briehan/.virtualenvs/django-trunk/src/django/django/contrib/sites/models.py", line 5, in <module>
    raise Exception

So it looks like the problem is caused by importing generic from django.contrib.contenttypes. When I remove those imports from my project everything works.

comment:13 Changed 3 years ago by ramiro

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

Can't reproduce the reported issue using the project contained in foobar.tar.gz and the following revisions:

trunk as of now (post r16493):

django/conf/__init__.py:75: DeprecationWarning: The ADMIN_MEDIA_PREFIX setting has been removed; use STATIC_URL instead.
  "use STATIC_URL instead.", DeprecationWarning)
django/conf/__init__.py:75: DeprecationWarning: The ADMIN_MEDIA_PREFIX setting has been removed; use STATIC_URL instead.
  "use STATIC_URL instead.", DeprecationWarning)
Validating models...

0 errors found
Django version 1.4 pre-alpha, using settings 'foobar.settings'
Development server is running at http://127.0.0.1:8000/
Quit the server with CONTROL-C.
<class 'foobar.sites.models.Site'>
[13/Jul/2011 22:03:45] "GET / HTTP/1.1" 200 0

trunk as of r16500 (mentioned in comment:10):

django/conf/__init__.py:75: DeprecationWarning: The ADMIN_MEDIA_PREFIX setting has been removed; use STATIC_URL instead.
  "use STATIC_URL instead.", DeprecationWarning)
django/conf/__init__.py:75: DeprecationWarning: The ADMIN_MEDIA_PREFIX setting has been removed; use STATIC_URL instead.
  "use STATIC_URL instead.", DeprecationWarning)
Validating models...

0 errors found
Django version 1.4 pre-alpha, using settings 'foobar.settings'
Development server is running at http://127.0.0.1:8000/
Quit the server with CONTROL-C.
<class 'foobar.sites.models.Site'>
[13/Jul/2011 21:53:01] "GET / HTTP/1.1" 200 0

1.3.X branch as of now (post r16541):

Validating models...

0 errors found
Django version 1.3, using settings 'foobar.settings'
Development server is running at http://127.0.0.1:8000/
Quit the server with CONTROL-C.
<class 'foobar.sites.models.Site'>
[13/Jul/2011 21:54:02] "GET / HTTP/1.1" 200 0

And, as expected, I can reproduce it with 1.3:

Validating models...

0 errors found
Django version 1.3, using settings 'foobar.settings'
Development server is running at http://127.0.0.1:8000/
Quit the server with CONTROL-C.
<class 'django.contrib.sites.models.Site'>
[13/Jul/2011 21:51:06] "GET / HTTP/1.1" 200 0

So I'm going to close this as duplicate of #16283.

Last edited 3 years ago by ramiro (previous) (diff)

comment:14 Changed 3 years ago by briehan.lombaard@…

Thanks ramiro, but when I add from django.contrib.contenttypes import generic to my sites model I get the following:

./manage.py runserver
django/conf/__init__.py:75: DeprecationWarning: The ADMIN_MEDIA_PREFIX setting has been removed; use STATIC_URL instead.
  "use STATIC_URL instead.", DeprecationWarning)
django/conf/__init__.py:75: DeprecationWarning: The ADMIN_MEDIA_PREFIX setting has been removed; use STATIC_URL instead.
  "use STATIC_URL instead.", DeprecationWarning)
Validating models...

0 errors found
Django version 1.4 pre-alpha SVN-16541, using settings 'foobar.settings'
Development server is running at http://127.0.0.1:8000/
Quit the server with CONTROL-C.
<class 'django.contrib.sites.models.Site'>
[14/Jul/2011 03:38:39] "GET / HTTP/1.1" 200 0

Should I open a new ticket for this issue?

comment:15 Changed 3 years ago by lakin

  • Resolution duplicate deleted
  • Status changed from closed to reopened

I'm re-opening this as I feel that I can better explain what is happening here. It is the combination of a few different, but important aspects and there may be a few different fixes that are in order.

First off - from the sites documentation - https://docs.djangoproject.com/en/dev/ref/contrib/sites/#requestsite-objects There are parts of django that use the sites framework even if it's not installed. So ultimately, there valid use cases where the sites app is NOT installed, but it IS imported. This is an important distinction. In these cases, I feel that it is reasonable that one should be able to make a new sites app with a model called Site.

The above is not a bug, in and of itself. However, when the sites app is imported, its models.py is also imported. When the models.py is imported the ModelBase metaclass runs and prepares this model. It also caches this model for future use so that subsequent imports don't go through the costly process of preparing the model a second time. However, it caches the model based on the app_label and model class name, which in this case is 'sites.Site'. When the new, custom app is imported, the same process occurs. However - this time around it notices that it already prepared a 'sites.Site' - so rather than making a new one, it returns a reference to the old one. However, in this particular case it will always the wrong reference. It will always be a reference to the non-used django.contrib.sites.models.Site model instead of the custom one. For reference, this is the code that does the caching: https://code.djangoproject.com/browser/django/trunk/django/db/models/base.py#L91

There are many potential solutions to this - but I'm not sure which is the appropriate one. Ultimately I feel that django's use of app_labels as unique across all installed apps is the actual errant decision - but this ticket isn't about fixing that. This ticket is about fixing it so that one can use a custom sites.models.Site model when the django.contrib.sites.models.Site is not being used.

I think the appropriate usage would be to update it so that the Meta class for django's models only caches models that it has prepared when they are part of an installed application. If they are not part of an installed application then they should be ignored.

You can test this using the same attached example, but this time include django.contrib.admin in the INSTALLED_APPS (which imports sites). Or you can import the contenttypes app which also uses it.

Does that help explain the issue?

Last edited 3 years ago by lakin (previous) (diff)

comment:16 Changed 3 years ago by ramiro

Some findings and notes:

Bisection betwen 1.2 and 1.3 (the two releases the OP reports things changed from working to not working e.g. the custom Sites model gets replaced by the contrib.sites app one even when that app isn't listed in INSTALLED_APPS) shows the commit where that change in behavior appeared is r14769.

(Conditions are: Using the simple foobar project attached to this ticket and adding from django.contrib.contenttypes import generic to its sites/models.py file).

I don't think the changes from that commit in particular are wrong regarding this issue. Looking at the traceback in comment:12 IMHO what's the culprit of this unexpected behavior is the fact that django.contrib.contenttypes.generic contains both the definitions of the model-related generic stuff (GenericForeignKey, etc.) AND the admin app-related specialized inlines (GenericInlineModelAdmin, GenericStackedInline, GenericTabularInline.)

So, for example, if you import django.contrib.contenttypes.generic from your models.py because you need GenericForeignKey then the Generic*Inline* stuff imports django.contrib.admin and it in its own turn imports django.contrib.sites causing the observed failure. Again, the Sites framework isn't listed in INSTALLED_APPS and what is worse: The admin app isn't either (!).

Maybe it's time we move GenericInlineModelAdmin, GenericStackedInline, GenericTabularInline from django.contrib.contenttypes.generic to, say, django.contrib.contenttypes.generic_admin? (of course this would need a deprecation process)

Last edited 5 months ago by ramiro (previous) (diff)

comment:17 Changed 13 months ago by aaugustin

  • Status changed from reopened to new

comment:18 Changed 3 months ago by Simon Charette <charette.s@…>

In 10e3faf191d8f230dde8534d1c8fad8c8717816e:

Fixed #19774 -- Deprecated the contenttypes.generic module.

It contained models, forms and admin objects causing undesirable
import side effects. Refs #16368.

Thanks to Ramiro, Carl and Loïc for the review.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.