Code

Opened 6 years ago

Closed 6 years ago

Last modified 2 years ago

#6776 closed Uncategorized (fixed)

Importing a model from one app into another app causes "AlreadyRegistered" exception.

Reported by: Manuel <manny@…> Owned by: brosner
Component: Uncategorized Version: newforms-admin
Severity: Normal Keywords: nfa-blocker
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

example:

in a project with two apps, app1 and app2, with the following models.py files:

app1/models.py:

from django.db import models
from django.contrib import admin

class MyFirstModel(models.Model):
    pass

admin.site.register(MyFirstModel)

...

app2/models.py

from django.db import models
from django.contrib import admin
from app1.models import MyFirstModel

...

running the command:

./manage.py runserver

raises the following exception:

Unhandled exception in thread started by <function inner_run at 0x2ab13a637c80>
Traceback (most recent call last):
...
 File "/usr/local/lib/python2.4/site-packages/django/contrib/admin/sites.py", line 82, in register
    raise AlreadyRegistered('The model %s is already registered' % model.__name__)
django.contrib.admin.sites.AlreadyRegistered: The model MyFirstModel is already registered

My quick and dirty solution is to get rid of the exception in the register method of the AdminSite class in the sites.py file. i.e. changing it from this:

for model in model_or_iterable:
     if model in self._registry:
        raise AlreadyRegistered('The model %s is already registered' % model.__name__)
     self._registry[model] = admin_class(model, self)

to this:

for model in model_or_iterable:
    if not model in self._registry:
        self._registry[model] = admin_class(model, self)

In this way models are only registered if they haven't already been registered, and are ignored otherwise (instead of raising an exception) - as far as I can tell this shouldn't be a problem - but I'm not familiar enough with the code and your objectives to know for sure.

Attachments (2)

6776_modeladmin_fix.1.diff (4.1 KB) - added by brosner 6 years ago.
initial cut for patching this. needs alot more work, but this is a proof of concept for the final patch.
6776_modeladmin_fix.2.diff (4.9 KB) - added by brosner 6 years ago.
slightly better patch and tests

Download all attachments as: .zip

Change History (14)

comment:1 Changed 6 years ago by brosner

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

I am still not 100% sure if this is a problem. Let me first explain why this is occuring. You are performing an import from app1.models import MyFirstModel and you probably have this app in your INSTALLED_APPS using the project. For example:

INSTALLED_APPS = (
    "project.app1",
)

This will result in Python importing the models twice. One in each import. If you change the first import to use the project it will work or vise-versa.

comment:2 follow-up: Changed 6 years ago by mrts

I was bitten by this as well by using relative imports (from app import models etc). Once I replaced relative imports with absolute imports (from project.app import models) everything works well.

This error is actually good for detecting unnecessary double imports, so I'd be -1 for removing it. Perhaps the error should provide a hint how to solve the problem instead, e.g. "The model MyFirstModel is already registered. This is probably caused by a double import, see docs/foo for ways to avoid double imports" as this is in no way obvious.

comment:3 Changed 6 years ago by Simon Willison

Another call for this behaviour to be changed. It's horrible to debug, and there doesn't seem to be any reason not to just ignore attempted dual registrations. Taking this to the mailing list.

comment:5 Changed 6 years ago by Simon Willison

This was discussed a few weeks ago:

http://groups.google.com/group/django-developers/browse_thread/thread/1a40c64e9c952c48

Consensus seemed to be for option number 2, quoted here:

Throw the exception based on whether the ModelAdmin is different
from a previously registered one. This will technically keep the
current behavior, but is really special casing the instances where
admin.site.register is called more than once. This currently happens
every so often with the models.py which is the documented method of
registration.

Solving this requires being able to compare two ModelAdmin subclasses for equality, but as Malcolm points out this will happen only on startup/import so doesn't need to be very efficient.

comment:6 Changed 6 years ago by brosner

  • Keywords nfa-blocker added
  • Owner changed from nobody to brosner
  • Status changed from new to assigned
  • Triage Stage changed from Design decision needed to Accepted

Marking with the correct keyword and stage. I will be working on a fix for this.

Changed 6 years ago by brosner

initial cut for patching this. needs alot more work, but this is a proof of concept for the final patch.

Changed 6 years ago by brosner

slightly better patch and tests

comment:7 Changed 6 years ago by brosner

It would seem to me that my patch is prone to some threading issues. Anyone want to shed some light on this?

comment:8 Changed 6 years ago by garcia_marc

  • milestone set to 1.0 alpha

comment:9 Changed 6 years ago by anonymous

  • Cc cmawebsite@… added

comment:10 Changed 6 years ago by brosner

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

(In [7872]) newforms-admin: Added autodiscover functionality to django.contrib.admin. This makes the admin aware of per-app admin.py modules and does an import on them when explicitly called. Docs show how this is used. Fixed #6003, #6776, #6776.

comment:11 Changed 3 years ago by jacob

  • milestone 1.0 alpha deleted

Milestone 1.0 alpha deleted

comment:12 in reply to: ↑ 2 Changed 2 years ago by Sacha Zyto <sachazyto@…>

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset

Replying to mrts:

Once I replaced relative imports with absolute imports (from project.app import models) everything works well.

Note that if you prefer, you can use only relative imports and it will work as well (just tested it on django1.3.1). The exception happens when you combine absolute and relative imports.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.