Code

Opened 4 years ago

Closed 5 weeks ago

#12842 closed Bug (fixed)

Importing a non-installed app assumes the non-installed DB table exists

Reported by: simonb Owned by: niall
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: installed, model, relation
Cc: bnomis@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The project example attached demonstrates the problem with the assumption built into Django that if a model is "seen" it is installed and its database table exists. This is particularly a problem with model inheritance. In this example AppB inherits from AppA but AppB is not installed. AppC imports AppB causing AppB to be "seen" and added to the list of installed apps together with the assumption that its table exists in the database. So when an AppA object is deleted Django also issues SQL to delete from the AppB table - which throws an error because it does not exist.

See the test.py file which gives the following error:

django.db.utils.DatabaseError: relation "AppB_appb" does not exist
LINE 1: ...ppB_appb"."appa_ptr_id", "AppB_appb"."nameb" FROM "AppB_appb...

The ModelBase meta class calls register_models when the class is created. I suggest that register_models should do one of the following:

  1. only register the model if it is actually installed and the table exists in the db
  2. if not installed issue a debug log message
  3. if not installed throw an exception

Number 1 with number 2 is my preference.

If you have a library with lots of models and lots of inheritance you probably do not want to install all the models for all projects. But some of the apps which are installed might reference a non-installed app merely by, quite legitimately, importing its class.

It's possible to work around this issue by checking all reference to models are installed before using them. See AppC.models, test.py and the APPB_IS_INSTALLED setting.

I guess, fundamentally, it's a question of philosophy: is it Django's philosophy that if a Model is seen, it is installed and exists in the database? The current behaviour. If so, this and the consequence of it should be stated in large letters in the documentation.

I suggest that if the current behaviour is as intended then a check should be put in to register_models() (if DEBUG is set?) to catch references to non-installed models and to log an error message that your site is probably going to crash soon. This, at least, would make the source of the error more obvious.

Thanks

Simon

Attachments (1)

test_app_install.zip (8.5 KB) - added by simonb 4 years ago.
test project demonstrating issue of assumption of existence

Download all attachments as: .zip

Change History (10)

Changed 4 years ago by simonb

test project demonstrating issue of assumption of existence

comment:1 Changed 4 years ago by simonb

  • Cc bnomis@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 4 years ago by russellm

  • Triage Stage changed from Unreviewed to Accepted

The non-existence of tables will always be a problem - Django doesn't force you to run syncdb, so it would be possible to get the errors you describe even if AppB *was* in INSTALLED_APPS.

However, that said, cases where it can be predicted that syncdb won't work should be caught. The last suggestion (that register_models should raise an error if the app isn't mentioned in INSTALLED_APPS) sounds like a plausible solution to me.

comment:3 Changed 4 years ago by niall

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

I have a patch that adds a test for app_label or "django.contrib." + app_label being in INSTALLED_APPS but it is causing problems with the other tests (the "apps" the models belong to aren't in the settings). Is there a better way to test for this?

comment:4 Changed 3 years ago by lukeplant

  • Type set to Bug

comment:5 Changed 3 years ago by lukeplant

  • Severity set to Normal

comment:6 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:7 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:8 Changed 16 months ago by aaugustin

  • Component changed from Core (Other) to Database layer (models, ORM)

comment:9 Changed 5 weeks ago by aaugustin

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

After the app loading refactor, you aren't allowed to import models that aren't in an installed application.

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.