Code

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#17762 closed Bug (duplicate)

Multi-db apps (no Django test suite) testing fails to create in-memory sqlite DBs

Reported by: agriffis Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: aron@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When testing a project with multiple databases, Django fails to create multiple in-memory SQLite databases, instead it creates only one (which is populated only with the table definitions for the default DB). The problem is that the generic test_db_signature() in [source:django/trunk/django/db/backends/creation.py db/backends/creation.py] doesn't work for in-memory SQLite databases, which all have the same NAME (":memory:").

The attached patch fixes this problem by adding a test_db_signature() method to the SQLite DatabaseCreation class.

Attachments (1)

django-17762.patch (836 bytes) - added by agriffis 2 years ago.

Download all attachments as: .zip

Change History (11)

Changed 2 years ago by agriffis

comment:1 Changed 2 years ago by ramiro

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement set
  • Summary changed from Multi-db testing fails to create multiple databases to Multi-db apps (no Django test suite) testing fails to create in-memory sqlite DBs
  • Triage Stage changed from Unreviewed to Accepted

I can reproduce the reported problem with tests of an standalone app that uses multiple database (the problem doesn't happen with Django's own test suite).

The patch doesn't seem to make any sense, though.

comment:2 Changed 2 years ago by lrekucki

Actually, see #17758 how to reproduce this with Django's test suite.

comment:3 follow-up: Changed 2 years ago by ramiro

Sorry, I can't reproduce this, my previous attempt failed because it wasn't correctly set up.

This is how I tested:

  1. Created a project.
  2. Added an application by simply copying the tests/regressiontests/multiple_database Django trunk regression test (here I'm trusting it to assure us multi-DB functionality is effectively working) to a mdb application directory and adding it to the INSTALLED_APPS setting.

This means some 'multiple_database' -> 'mdb' replacements must be made in the app files:

$ cd mdb
$ ack -l multiple_database --type-set json=.json --py --json |xargs sed -i s/multiple_database/mdb/g
  1. Edited the DATABASES setting to look like this:
    DATABASES = {
        'default': {
            'ENGINE': 'django.db.backends.sqlite3',
            'NAME': 'data.db',
        },
        'other': {
            'ENGINE': 'django.db.backends.sqlite3',
            #'TEST_NAME': ':memory:',
        }
    }
    
  1. Now I can run the tests for the 'mdb' app and they run without failures:
    /manage.py test -v2 mdb
    Creating test database for alias 'default' (':memory:')...
    [...]
    Creating test database for alias 'other' (':memory:')...
    [...many lines of tests output...]
    ----------------------------------------------------------------------
    Ran 51 tests in 1.099s
    
    OK
    Destroying test database for alias 'default' (':memory:')...
    Destroying test database for alias 'other' (':memory:')...
    
    Note how with verbosity=2 the test runner reports the names of the test DBs it is creating/destroying.
  1. I tested this with: 1.3, 1.3.1, current 1.3.X branch tip and trunk, always with the same results.

Could you give us more details how are you getting the "Django fails to create multiple in-memory SQLite databases, instead it creates only one (which is populated only with the table definitions for the default DB)" failure you describe so others can reproduce it?

Last edited 2 years ago by ramiro (previous) (diff)

comment:4 in reply to: ↑ 3 ; follow-up: Changed 2 years ago by agriffis

Replying to ramiro:

Could you give us more details how are you getting the "Django fails to create multiple in-memory SQLite databases, instead it creates only one (which is populated only with the table definitions for the default DB)" failure you describe so others can reproduce it?

Thanks for the time you've put into this, and the instructions for creating the mdb app! I wasn't sure how to quickly make an isolated test case for this. I see the problem using the mdb app with the following DATABASES definition:

DATABASES = {
    'default': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': ':memory:',
    },
    'other': {
        'ENGINE': 'django.db.backends.sqlite3',
        'NAME': ':memory:',
    }
}

which results in:

$ ./manage.py test -v2 mdb 2>&1 | grep Creating
Creating test database for alias 'default' (':memory:')...
Creating tables ...
Creating table auth_permission
Creating table auth_group_permissions
Creating table auth_group
Creating table auth_user_user_permissions
Creating table auth_user_groups
Creating table auth_user
Creating table auth_message
Creating table django_content_type
Creating table django_session
Creating table django_site
Creating table mdb_review
Creating table mdb_person
Creating table mdb_book_authors
Creating table mdb_book
Creating table mdb_pet
Creating table mdb_userprofile

At the time I originally saw the problem, I was working on a large, venerable Django app that calls out its test DB settings explicitly as shown above. It's not an ordinary way to declare DATABASES, but I don't think Django should be mistaking the two databases as one since the ':memory:' name is special for SQLite.

Regarding:

The patch doesn't seem to make any sense, though.

The base test_db_signature() distinguishes unique databases by the combination of ENGINE, NAME, etc. SQLite databases are distinguished by NAME except for ':memory:' and '' (on-disk temporary DB) which are unique per-connection even if the NAME is the same as another connection. I figured id(self.connection) accurately identifies unique databases in this case. Does it make sense with that explanation?

comment:5 follow-up: Changed 2 years ago by aaugustin

IIRC there's a special case to make Django do the right thing if you omit 'NAME': ':memory:', from the definition of both databases. Could you try that?

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

Replying to aaugustin:

IIRC there's a special case to make Django do the right thing if you omit 'NAME': ':memory:', from the definition of both databases. Could you try that?

That works, but I actually don't need a workaround (I have those). Thanks though! I filed this ticket to get the underlying issue fixed of Django recognizing explicitly temporary SQLite databases.

comment:7 in reply to: ↑ 4 Changed 2 years ago by ramiro

Replying to agriffis:

Replying to ramiro:

Could you give us more details how are you getting the "Django fails to create multiple in-memory SQLite databases, instead it creates only one (which is populated only with the table definitions for the default DB)" failure you describe so others can reproduce it?

Thanks for the time you've put into this, and the instructions for creating the mdb app! I wasn't sure how to quickly make an isolated test case for this. I see the problem using
[...]

which results in:

$ ./manage.py test -v2 mdb 2>&1 | grep Creating
Creating test database for alias 'default' (':memory:')...
Creating tables ...
[...]

That's weird will try to reproduce it.

Regarding:

The patch doesn't seem to make any sense, though.

The base test_db_signature() distinguishes unique databases by the combination of ENGINE, NAME, etc. SQLite databases are distinguished by NAME except for ':memory:' and '' (on-disk temporary DB) which are unique per-connection even if the NAME is the same as another connection. I figured id(self.connection) accurately identifies unique databases in this case. Does it make sense with that explanation?

I understand that part. I was talking about the fact that the name variable is used without being defined before so the code is broken.

Last edited 2 years ago by ramiro (previous) (diff)

comment:8 Changed 2 years ago by ramiro

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

Now I realise you are using ":memory:" for the NAME keys in DATABASES. That's an odd setup it means all your production databases are no persistent.

Something similar was reported in #16329 and I made a comment about that setup there and how to achieve creating in-memory test databases that led to the closing of that ticket.

I will close this ticket as a duplicate but I will try to revisit the issue and ask other developers' opinions about this topic.

comment:9 follow-up: Changed 2 years ago by agriffis

Replying to ramiro:

I understand that part. I was talking about the fact that the name variable is used without being defined before so the code is broken.

How embarrassing. You're right of course. I made final changes to the patch to make it more presentable and goofed it. :-(

Now I realise you are using ":memory:" for the NAME keys in DATABASES. That's an odd setup it means all your production databases are no persistent.

Actually they're test-specific settings. In production that project had real databases...

Regarding ticket #16329 I agree this is a duplicate of that ticket. Sorry I didn't find that one when I looked. I think the patch in this ticket, once unbroken, might be the most correct way to fix the problem. Let me know if you'd like me to repair it and attach it over there.

comment:10 in reply to: ↑ 9 Changed 2 years ago by ramiro

Replying to agriffis:

Now I realise you are using ":memory:" for the NAME keys in DATABASES. That's an odd setup it means all your production databases are no persistent.

Actually they're test-specific settings. In production that project had real databases...

Regarding ticket #16329 I agree this is a duplicate of that ticket. Sorry I didn't find that one when I looked. I think the patch in this ticket, once unbroken, might be the most correct way to fix the problem. Let me know if you'd like me to repair it and attach it over there.

I actually have a patch based on your code combined with something lrecucki did for #17758 in a GitHub fork, still trying to find a way to write tests for it. Thanks!

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.