Code

Opened 6 years ago

Closed 4 years ago

#6961 closed (fixed)

loaddata fails if models directory instead of models.py

Reported by: pmd Owned by: nobody
Component: Core (Management commands) Version: master
Severity: Keywords: loaddata
Cc: zilingzhao@…, mjmalone@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

If the models for an app have been split into multiple files within a models subdirectory of the app then loaddata looks for fixtures in appname/models/fixtures instead of appname/fixtures.

To reproduce:

   django-admin.py startproject proj1
   cd proj1
   django-admin.py startapp foo
   cd foo
   vi ../settings.py  # set INSTALLED_APPS to just proj1.foo
   mkdir fixtures
   touch fixtures/abc.json

   ../manage.py loaddata --verbosity=2 abc.json
   Problem installing fixture '/data/home/leotest/projects/proj1/../proj1/foo/fixtures/abc.json': No JSON object could be decoded
   # location is correct, error is because file is empty

   rm models.py
   mkdir models
   touch models/__init__.py
   ../manage.py loaddata --verbosity=2 abc.json

   Loading 'abc' fixtures...
   Checking '/data/home/leotest/projects/proj1/../proj1/foo/models/fixtures' for fixtures...
   Trying '/data/home/leotest/projects/proj1/../proj1/foo/models/fixtures' for json fixture      'abc'...
   No json fixture 'abc' in '/data/home/leotest/projects/proj1/../proj1/foo/models/fixtures'.
   Checking absolute path for fixtures...
   Trying absolute path for json fixture 'abc'...
   No json fixture 'abc' in absolute path.
   No fixtures found.

The problem is that loaddata uses app.__file__ and app object is actually the app.models
module rather than the app module. This is around line 141 in core/management/commands/loaddata.py.

Note that setting app_label in Meta subclass for each model has no bearing on this problem.

Attachments (6)

patch1.txt (865 bytes) - added by pmd 6 years ago.
patch to make loaddata use app dir and not models module dir
model_folder_fixture_load_test.patch (1.6 KB) - added by zhaoz 6 years ago.
needs init.py file in loaddata folder. Tests fixture loading with models folder as opposed to models.py
patch2.txt (916 bytes) - added by mmalone 5 years ago.
Patch that checks whether models module is a package by checking for path attribute
model_folder_fixture_load_test_for_patch2.patch (5.7 KB) - added by mmalone 5 years ago.
Tests for patch 2
patch3.txt (690 bytes) - added by HuCy 5 years ago.
Another approach. Found this Ticket too late, so i did a patch by myself.
patch_3_tests_combined.diff (6.6 KB) - added by justinlilly 5 years ago.
patch 3 combined with tests.

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by pmd

patch to make loaddata use app dir and not models module dir

comment:1 Changed 6 years ago by pmd

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The attached patch (patch1) is against r7403 and tested on Linux. Not sure if it would work on windows.

comment:2 Changed 6 years ago by programmerq

  • Triage Stage changed from Unreviewed to Accepted

This definitely seems like a bug.

comment:3 Changed 6 years ago by zhaoz

  • Component changed from Core framework to django-admin.py

What is the status of this bug? I recently bumped into this issue, it looks like it hasn't been touched for quite awhile.

What needs to be done?

comment:4 Changed 6 years ago by anonymous

  • Cc zilingzhao@… added

Changed 6 years ago by zhaoz

needs init.py file in loaddata folder. Tests fixture loading with models folder as opposed to models.py

comment:5 follow-up: Changed 6 years ago by zhaoz

  • Patch needs improvement set

The patch did not work for me when patched against svn trunk's 9222, failed test.

comment:6 in reply to: ↑ 5 Changed 6 years ago by zhaoz

  • Patch needs improvement unset

Replying to zhaoz:

The patch did not work for me when patched against svn trunk's 9222, failed test.

Scratch what I last said, it's working, but it doesn't seem to be done in a very elegant way.

comment:7 Changed 5 years ago by mmalone

  • Cc mjmalone@… added

Seems what we're really trying to do here is determine whether the models module (return by get_apps()) is a package (a directory with an __init__.py and submodules) or a normal module. According to
an essay that was published when package support landed in Python, the only difference between a package and an ordinary module is that a package has a __path__ attribute. Handily enough, the __path__ attribute is an array that, by default, contains a single string pointing to the package directory, which is at the equivalent level in the file system directory hierarchy as models.py.

I've attached a patch that checks the modules for the existence of a __path__ attribute and handles them properly (and another patch containing some tests). I'm fairly confident this is the right way to do this, given the essay linked to above. I think this method may also be used to remove the Meta.app_label requirement for models submodules.

Changed 5 years ago by mmalone

Patch that checks whether models module is a package by checking for path attribute

Changed 5 years ago by mmalone

Tests for patch 2

Changed 5 years ago by HuCy

Another approach. Found this Ticket too late, so i did a patch by myself.

comment:8 Changed 5 years ago by stephaner

I'm not sure if it's related to this bug but the loaddata command works only if there's a models.py file (empty) in the app directory:

That doesn't work:

app/__init__.py
    fixtures/data.json

that works:

app/__init__.py
    models.py
    fixtures/data.json

and the documentation says (or I didn't find) nothing about that requirement.
Tested with trunk r11032.

comment:9 Changed 5 years ago by justinlilly

  • Triage Stage changed from Accepted to Ready for checkin

Patch looks good, applied cleanly. I've combined the tests for patch3 and patch3 into this patch.

Changed 5 years ago by justinlilly

patch 3 combined with tests.

comment:10 Changed 5 years ago by lukeplant

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

(In [11914]) Fixed #6961 - loaddata fails if models is a package instead of a module

Thanks to pmd for report, zhaoz, mmalone and justinlilly for patch

comment:11 Changed 5 years ago by lukeplant

(In [11918]) [1.1.X] Fixed #6961 - loaddata fails if models is a package instead of a module

Thanks to pmd for report, zhaoz, mmalone and justinlilly for patch

Backport of 11914 from trunk.

comment:12 Changed 4 years ago by xcco3x@…

  • Resolution fixed deleted
  • Status changed from closed to reopened

Bump on stephaner's comment. Why are models needed at all? Consider the case where you have an app for doing various things for your site. There may not be any models, but there may be an initial_data fixture to add in a default user or Site information.

comment:13 Changed 4 years ago by Alex

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

Requiring models for various things is a seperate issue, closing this as the bug here was fixed.

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.