Opened 9 years ago

Closed 8 years ago

#1658 closed defect (duplicate)

db.models.base.ModelBase.__new__ fails to deduce app_label with model packages

Reported by: bjorn@… Owned by: nobody
Component: Core (Other) Version: master
Severity: major Keywords: egg, distutils, pkg_resources
Cc: erob@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

In the latest magic-removal branch, if models are in a separate directory (package) instead of just a module they will have one more component in their path.

db.models.base.ModelBase.__new__ tries to deduce the app_label from the model's module, but fails in this case. It will call register_module() with the parent directory as its app_label, but in this case the parent directory is the model package itself, resulting in it registering the module wrongly. The code in question is:

        if getattr(new_class._meta, 'app_label', None) is None:
            # Figure out the app_label by looking one level up.
            # For 'django.contrib.sites.models', this would be 'sites'.
            new_class._meta.app_label = model_module.__name__.split('.')[-2]

For example, a model in myproj/apps/myapp/models/mymodel.py will register itself as belonging to myproj.apps.myapp.models with app_label models.

-- Bjorn

Attachments (2)

base.diff (822 bytes) - added by erob@… 9 years ago.
patch for this issue -- is this working?
test_models.py (3.2 KB) - added by erob@… 9 years ago.
updated test case

Download all attachments as: .zip

Change History (19)

comment:1 Changed 9 years ago by adrian

#1686 was a duplicate.

comment:2 Changed 9 years ago by erob@…

  • Version set to SVN

Hi,

I'm getting this problem too when unit testing django with unittest.

Here's the code in question:

from django.conf import settings

import os, os.path
import unittest

class DjangoTest(unittest.TestCase):
      def setUp(self):
          # Test fixture method
          # Must configure django settings first
          try:
              settings.configure(DEBUG=1,
              DATABASE_NAME=":memory:",
              DATABASE_ENGINE="sqlite3", 
              TEMPLATE_DEBUG=1,
              TEMPLATE_DIRS=(os.getcwd())) # current directory
              #self.settings = settings
              reload(settings)
          except Exception, e:
              import warnings
              warnings.warn(e, UserWarning, stacklevel=2)
              pass
          self.settings = settings
         
      def tearDown(self):
          # test closure method
          self.settings = None

      def testSettings(self):
          # put some code here
          self.failIf(self.settings.DATABASE_ENGINE != "sqlite3",
self.settings.DATABASE_ENGINE)

      def testApp(self):
          from django.db import models
          class App(models.Model):
                pass

if __name__ == '__main__':
   unittest.main()

The traceback msg:

% python test_models.py
/usr/local/lib/python2.4/unittest.py:251: UserWarning: reload() argument must be module
  self.setUp()
E/usr/local/lib/python2.4/unittest.py:251: UserWarning: Settings already configured.
  self.setUp()
.
======================================================================
ERROR: testApp (__main__.DjangoTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test_models.py", line 48, in testApp
    class App(models.Model):
  File "/usr/local/lib/python2.4/site-packages/Django-0.95-py2.4.egg/django/db/models/base.py", line 45, in __new__
    new_class._meta.app_label = model_module.__name__.split('.')[-2]
IndexError: list index out of range

----------------------------------------------------------------------
Ran 2 tests in 0.396s

FAILED (errors=1)

comment:3 Changed 9 years ago by erob@…

  • Cc erob@… added

is there a known work-around for this ??

comment:4 Changed 9 years ago by anonymous

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

Here is partial work-around, which works with os.getcwd and os.path.basename.
Its sufficient for passing a really simple unit test but comments would be of
courses welcome.

Index: base.py
===================================================================
--- base.py     (revision 3350)
+++ base.py     (working copy)
@@ -42,7 +42,13 @@
         if getattr(new_class._meta, 'app_label', None) is None:
             # Figure out the app_label by looking one level up.
             # For 'django.contrib.sites.models', this would be 'sites'.
-            new_class._meta.app_label = model_module.__name__.split('.')[-2]
+            try:
+               new_class._meta.app_label = model_module.__name__.split('.')[-2]
+           except IndexError, e:
+                cwd = os.getcwd()
+               new_class._meta.app_label = os.path.basename(cwd)
+               print new_class._meta.app_label
+

         # Bail out early if we have already created this class.
         m = get_model(new_class._meta.app_label, name)

Changed 9 years ago by erob@…

patch for this issue -- is this working?

comment:5 Changed 9 years ago by erob@…

  • Resolution worksforme deleted
  • Status changed from closed to reopened

needs some feedback/inputs on this patch..

comment:6 Changed 9 years ago by mtredinnick

A quick review of this patch. It has a few problems:

  1. os.getcwd() can't be right: the result returned by that depends on the current working directory, not the import path of the model. We don't change directories at any point during the import process, so the results returned here will be dependent upon factors completely outside Django's control.
  1. It is not a full solution to the problem because it only works for a single hierarchy level (it looks like an import of models/foo/bar.py will still fail). We should try to get something that works for the general case.
  1. some very minor points: the indentation is inconsistent and there is a print statement in there. If you're not going to use the result of the exception, there's no point catching "e".

comment:7 Changed 9 years ago by erob@…

mtredinnick: Thanks for the patch review.
I've made a updated diff but I cant upload..

I'm not sure what you mean by 'single hierarchhy level', can u please elaborate on that?
However, for simple unit testing with unittest things seems ok here.




comment:8 Changed 9 years ago by mtredinnick

My second point in my previous comment was talking about an application structure that looked like this:

application/
   __init__.py
   models/
      __init__.py
      foo/
         __init__.py
         bar.py
      baz/
         __init__.py
         blam.py

So you would be importing models as

from application.models.foo.bar import MyModel

and so on. Does the change still work then?

comment:9 Changed 9 years ago by erob@…

I have modified the test case in order to reproduce
the application structure/environment as you suggested.

The test still works, so now I don't know how to break it ;)

Here's the app structure:

models/
 test_models.py <-- the test case
 foo/
    __init__.py
    bar/
    __init__.py
    baz.py 
}}

In baz.py:
{{{
#!python
from django.db import models
}}} 


Here's the test-case:

{{{
#! /usr/bin/env python

__ident__ = "$Id: test_models.py 48 2006-07-04 17:07:37Z erob $"

from test.test_support import run_unittest, verbose
from django.conf import settings

import os, os.path
import unittest

class DjangoTest(unittest.TestCase):
      def setUp(self):
          # Test fixture method
          # Must configure django settings first
          try:
              settings.configure(DEBUG=1,
              DATABASE_NAME=":memory:",
              DATABASE_ENGINE="sqlite3", 
              TEMPLATE_DEBUG=1,
              TEMPLATE_DIRS=(os.getcwd())) # current directory
              #self.settings = settings
              #reload(settings)
          except Exception, e:
              import warnings
              warnings.warn(e, UserWarning, stacklevel=2)

          if settings is not None:
             self.settings = settings
             
          try:
              from foo.bar.baz import models
          finally:
              self.models = models
         
      def tearDown(self):
          # test closure method
          self.settings = None

      def testSettings(self):
          # put some code here
          self.failIf(self.settings.DATABASE_ENGINE != "sqlite3",
self.settings.DATABASE_ENGINE)

      def testApp(self):
          # this requires a patch to base.py
          # in order to make this happen...
          models = self.models
          class BaseApp(models.Model):
                name = models.CharField(maxlength=200)
                def __str__(self):
                    return self.name
          class App(BaseApp):
                class Admin:
                      pass
          app = App()
          app.name = "Hello, World!"
          self.failUnless(isinstance(app, App), app.__module__)
          self.assertEqual(app.name, "Hello, World!", app.name)

def test_main():
    tests = [ DjangoTest ]
    run_unittest(*tests)

if __name__ == '__main__':
   test_main()
}}}

The test results:
{{{
% python test_models.py
testApp (__main__.DjangoTest) ... ok
/usr/local/lib/python2.4/unittest.py:251: UserWarning: Settings already configured.
  self.setUp()
testSettings (__main__.DjangoTest) ... ok

----------------------------------------------------------------------
Ran 2 tests in 0.606s

OK
}}}

comment:10 Changed 9 years ago by anonymous

This test case does not test the problem reported in the ticket. You are not trying to load any actual models from baz.py and your test is inside your model framework, which can potentially change things.

Further, you have not correct the main problem I pointed out: any patch that requires a call to os.getcwd() to work is not a portable solution to this problem.

comment:11 Changed 9 years ago by mtredinnick

(That last comment was from me, Malcolm, again. Accidently not logged in.)

Changed 9 years ago by erob@…

updated test case

comment:12 Changed 9 years ago by erob@…

Ok, I've updated the test case, but it just highlights several things
that seems incorrect in Django. I havent changed anything so far in the
Django code so its only good for diagnosis of problems.

As for the actual problem described here, I've managed to reproduce it
twice, but its far from being resolved (design bug??).

Moreover, the unit test also grab several design flaws similar to the
original issue, theses problems seems to happens more django/db/models/base.py
and django/db/models/loading.py.

For using the test case in your own project, replace the 'notmm.models' occurances
with your own model package.


comment:13 Changed 9 years ago by erob@…

  • Keywords egg distutils added

maybe its a good idea to use pkg_resources here.
pkg_resources.resource_string and pkg_resouces.Requirement
may be used to 'deduce' the package location. Is it
possible to distribute packages (django projects or apps)
in Eggs format ?

Example:

import pkg_resources
_app_label = pkg_resources.resource_string(module_path, module)
...


comment:14 Changed 9 years ago by erob@…

  • Keywords pkg_resources added

update on patch above and should works for getting the app_label object.

from pkg_resources import resource_string, Requirement
model_module = 'foo/bar/models.py'
project_name = 'quux'
requirement = Requirement.parse(project_name)
app_label = resource_string(requirement, module_name)
# do stuff with app_label here

comment:15 follow-up: Changed 8 years ago by Gary Wilson <gary.wilson@…>

  • Triage Stage changed from Unreviewed to Accepted

Joseph is working on fixing the app loading code and plans on opening a new ticket for it.

comment:16 in reply to: ↑ 15 Changed 8 years ago by jkocherhans

Replying to Gary Wilson <gary.wilson@gmail.com>:

Joseph is working on fixing the app loading code and plans on opening a new ticket for it.

Ticket opened. #3591

comment:17 Changed 8 years ago by ubernostrum

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

Closing in favor of #3591.

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