Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#16353 closed Bug (fixed)

With multi-databases, if django.contrib.sites.models.Site is not synchronized on all databases, Django's tests fail

Reported by: aaugustin Owned by: nobody
Component: Testing framework Version: 1.3
Severity: Release blocker Keywords:
Cc: ubernostrum@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Here are the detailed steps to reproduce (in a fresh checkout):

$ PYTHONPATH=. django/bin/django-admin.py startproject testproj
$ cd testproj
$ PYTHONPATH=.. ./manage.py startapp testapp
$ vi settings.py
DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': 'default.sqlite3',
    },
    'other': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': 'other.sqlite3',
    }
}

DATABASE_ROUTERS = ['testapp.routers.MyAppRouter']
$ vi testapp/routers.py
class MyAppRouter(object):

    def db_for_read(self, model, **hints):
        return 'other' if model._meta.app_label == 'testapp' else 'default'

    def db_for_write(self, model, **hints):
        return 'other' if model._meta.app_label == 'testapp' else 'default'

    def allow_relation(self, obj1, obj2, **hints):
        pass

    def allow_syncdb(self, db, model):
        return db == ('other' if model._meta.app_label == 'testapp' else 'default')
$ PYTHONPATH=.. ./manage.py test
Creating test database for alias 'default'...
Creating test database for alias 'other'...
Traceback (most recent call last):
  File "./manage.py", line 14, in <module>
    execute_manager(settings)
  File "/Users/myk/Desktop/django-trunk/django/core/management/__init__.py", line 442, in execute_manager
    utility.execute()
  File "/Users/myk/Desktop/django-trunk/django/core/management/__init__.py", line 379, in execute
    self.fetch_command(subcommand).run_from_argv(self.argv)
  File "/Users/myk/Desktop/django-trunk/django/core/management/commands/test.py", line 34, in run_from_argv
    super(Command, self).run_from_argv(argv)
  File "/Users/myk/Desktop/django-trunk/django/core/management/base.py", line 191, in run_from_argv
    self.execute(*args, **options.__dict__)
  File "/Users/myk/Desktop/django-trunk/django/core/management/base.py", line 220, in execute
    output = self.handle(*args, **options)
  File "/Users/myk/Desktop/django-trunk/django/core/management/commands/test.py", line 54, in handle
    failures = test_runner.run_tests(test_labels)
  File "/Users/myk/Desktop/django-trunk/django/test/simple.py", line 354, in run_tests
    old_config = self.setup_databases()
  File "/Users/myk/Desktop/django-trunk/django/test/simple.py", line 291, in setup_databases
    test_db_name = connection.creation.create_test_db(self.verbosity, autoclobber=not self.interactive)
  File "/Users/myk/Desktop/django-trunk/django/db/backends/creation.py", line 256, in create_test_db
    if Site is not None and Site.objects.using(self.connection.alias).count() == 1:
  File "/Users/myk/Desktop/django-trunk/django/db/models/query.py", line 334, in count
    return self.query.get_count(using=self.db)
  File "/Users/myk/Desktop/django-trunk/django/db/models/sql/query.py", line 406, in get_count
    number = obj.get_aggregation(using=using)[None]
  File "/Users/myk/Desktop/django-trunk/django/db/models/sql/query.py", line 372, in get_aggregation
    result = query.get_compiler(using).execute_sql(SINGLE)
  File "/Users/myk/Desktop/django-trunk/django/db/models/sql/compiler.py", line 754, in execute_sql
    cursor.execute(sql, params)
  File "/Users/myk/Desktop/django-trunk/django/db/backends/sqlite3/base.py", line 226, in execute
    return Database.Cursor.execute(self, query, params)
django.db.utils.DatabaseError: no such table: django_site

Attached patch fixes this problem.

Attachments (3)

sites-and-multi-db.diff (1.3 KB) - added by aaugustin 3 years ago.
16353.diff (3.2 KB) - added by aaugustin 3 years ago.
16353.2.diff (3.2 KB) - added by aaugustin 3 years ago.

Download all attachments as: .zip

Change History (14)

Changed 3 years ago by aaugustin

comment:1 Changed 3 years ago by manfre

  • Owner changed from nobody to manfre
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by manfre

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

comment:3 Changed 3 years ago by ramiro

  • Needs tests set

comment:4 Changed 3 years ago by aaugustin

#16515 was a duplicate.

comment:5 Changed 3 years ago by ubernostrum

  • Cc ubernostrum@… added

comment:6 Changed 3 years ago by aaugustin

#16828 was a duplicate.

Version 0, edited 3 years ago by aaugustin (next)

comment:7 Changed 3 years ago by aaugustin

  • Severity changed from Normal to Release blocker

comment:8 Changed 3 years ago by aaugustin

  • Needs tests unset

This regression was introduced in r16027 (trunk) and r16028 (1.3.X). It was duplicated in GIS in r16749 (trunk) and r16751 (1.3.X).

The goal was to fix #15573, a problem linked to the re-implementation of auto-incremented sequences (which was itself a regression, but at this point the history isn't relevant any longer). However, the "fix" doesn't look optimal: it introduces in the core some code that's specific to a contrib app (sites) and also this regression. Furthermore, it doesn't include any tests.

Attached patch reverts r16027 and r16749, and proposes another fix for #15573, a much more simple and predictable one.

Note that the post_syncdb signal is fired right after the tables are created or flushed, so there's no risk to overwrite an existing Site.


I tested the patch manually with the procedure described in #16828.

I'm still stuck writing automated tests. In fact, it's extremely difficult to test the initialization of the django_site table in the test databases, because of a chicken'n'egg problem. Since the table is created before the tests start running, we can't pull a trick like in tests/modeltests/proxy_model_inheritance/tests.py. I don't think hacking the internals of the app cache to deregister the app, drop the table and resync it is a good idea.

Suggestions welcome. Otherwise, I hope we can revert a commit that didn't include tests and demonstrably broke stuff even if it's impossible to write automated tests for either behavior.

Finally, note that the new changes are already covered by all the tests that fail when SITE_ID != 1 — see #15573.


This patch must be backported to 1.3.X.

Changed 3 years ago by aaugustin

Changed 3 years ago by aaugustin

comment:9 Changed 3 years ago by aaugustin

The full test suite passes under SQLite (just a sanity check).

Under Oracle, with the patch, I get this result: http://ci.myks.org/job/Django%20+%20Oracle/5/#showFailuresLink
where the official CI server gets this: http://ci.django-cms.org/job/Django/database=oracle,python=python2.7/271/#showFailuresLink

Both have 81 failures, so it isn't more broken with my patch than without.

comment:10 Changed 3 years ago by carljm

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

In [16868]:

Fixed #16353 -- don't try to create Site objects on all databases. Refs #15573, #15346. Thanks Aymeric Augustin for the report and the patch.

comment:11 Changed 3 years ago by carljm

In [16869]:

[1.3.X] Fixed #16353 -- don't try to create Site objects on all databases. Refs #15573, #15346. Thanks Aymeric Augustin for the report and the patch.

Backport of r16868 in trunk.

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.