Opened 3 years ago

Last modified 11 months ago

#23718 new Bug

TEST_MIRROR setting doesn't work as expected (and has no tests)

Reported by: Ilya Baryshev Owned by: nobody
Component: Testing framework Version: 1.7
Severity: Normal Keywords: replica testing
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

TEST_MIRROR promises "connection to slave will be redirected to point at default. As a result, writes to default will appear on slave"
I've set up a minimum django project (using postgres backed) to demonstrate behavior.

    def test_fixture(self):
        MyModel.objects.using('default').create(name=1)
        MyModel.objects.using('slave').create(name=2)
        MyModel.objects.using('slave').create(name=3)
        self.assertEqual(list(map(repr, MyModel.objects.using('default').all())),
                         list(map(repr, MyModel.objects.using('slave').all())))

Both lists should be equal, because replica queries should be hitting default instead.
It appears not to be the case for Django>=1.4 up to latest 1.7.1 (but actually passes against 1.3.7)

AssertionError: Lists differ: ['<MyModel: 1>', '<MyModel: 2>... != ['<MyModel: 2>', '<MyModel: 3>...

First differing element 0:
<MyModel: 1>
<MyModel: 2>

First list contains 1 additional elements.
First extra element 2:
<MyModel: 3>

- ['<MyModel: 1>', '<MyModel: 2>', '<MyModel: 3>']
?             ^               ----------------

+ ['<MyModel: 2>', '<MyModel: 3>']
?             ^

Here is a project I used to test: https://github.com/coagulant/test_mirror

Change History (10)

comment:1 Changed 3 years ago by Tim Graham

It would be quite helpful if you could convert your test to one for Django's test suite and then bisect to determine the commit in Django that introduced the regression.

comment:2 Changed 3 years ago by Ilya Baryshev

I've run git bisect, and first commit with regression happened to be: 905e33f84a1a10e4f0183d879c52076ef876fc3b, which is related to #16885

Here is a log of my bisect: https://gist.github.com/coagulant/d04b3e729b851784a93a

comment:3 Changed 3 years ago by Tim Graham

Summary: TEST_MIRROR setting doesn't work as expectedTEST_MIRROR setting doesn't work as expected (and has no tests)
Triage Stage: UnreviewedAccepted

I haven't verified the issue, but I noticed there are no tests for TEST['MIRROR'] (renamed on master) so we should at least add some. Also the test in the "regression commit" seems to be gone -- not sure if its removal was intentional or not.

comment:6 Changed 3 years ago by Shai Berger

The behavior in the bug description is sort-of expected, because of transactions (default and slave are the same database -- in the test project, they are defined this way even regardless of TEST_MIRROR -- but accessed through different connections, and so, separate transactions).

I'm saying sort-of expected because it seems the test is running in a transaction on default but not on slave, which is a little surprising -- but not the problem claimed.

I don't think we want to force test-cases to run each test in transactions on all databases -- I'm not sure that even makes sense; but we should probably document that tests using more than one database should be TransactionTestCases.

comment:7 Changed 3 years ago by harrykao

Is it possible to have the mirrors share a connection? Using TransactionTestCase has the additional disadvantage of preventing multiprocessed test runs, which works fine when each test runs in a transaction.

comment:8 Changed 2 years ago by Erick Yellott

I just ran into this as well. Based on the documentation, I expected to set

DATABASES = {
    'default': {
        ...
    },
    'replica': {
        ...
        'TEST_MIRROR': 'default', 
        # 'TEST': {'MIRROR': 'default'},  # I tried this too.
    }
}

and have my tests read from replica and write to default, which should be the same database and act as if I had just one database defined as default.

comment:9 Changed 2 years ago by Aymeric Augustin

I just rediscovered that test mirrors require TransactionTestCase.

I'm wondering if it would be possible to share the database connection instead of having two connections with the same parameters.

If that doesn't work, we can document this limitation instead.

comment:10 Changed 2 years ago by Asser Schrøder Femø

Just ran into this, but with raw queries instead. Using the connection directly does not work even with TransactionTestCase.

So for now I'm using this hacky workaround:

from django.db import connections

class ReplicationTestCase(TestCase):
    """
    Redirect raw queries to the replica, ie.

        from django.db import connections

        cursor = connections['replica'].cursor()
        cursor.execute(...)  # Is run directly on master in tests
    """
    def setUp(self):
        super(ReplicationTestCase, self).setUp()
        connections['replica']._orig_cursor = connections['replica'].cursor
        connections['replica'].cursor = connections['master'].cursor

    def tearDown(self):
        connections['replica'].cursor = connections['replica']._orig_cursor
        super(ReplicationTestCase, self).tearDown()

As an added benefit it works for model managers too even though I'm not using TransactionTestCase indicating that the connection might be able to be shared in a similar way?

comment:11 Changed 11 months ago by Kyle Agronick

Is this ever going to be fixed? Setting the mirror should just copy the cursor so no matter where the slave DB is called it redirects to the master.

comment:12 Changed 11 months ago by Tim Graham

It will be fixed when someone submits a patch and another person reviews it.

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