Opened 3 years ago

Last modified 2 weeks ago

#23718 assigned Bug

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

Reported by: Ilya Baryshev Owned by: eng. Ilian Iliev
Component: Testing framework Version: 1.7
Severity: Normal Keywords: replica testing
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
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 (14)

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 3 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 13 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 13 months ago by Tim Graham

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

comment:13 Changed 5 weeks ago by eng. Ilian Iliev

I have created a small patch, the PR is here https://github.com/django/django/pull/9603

The solution I went for is that when the MIRROR is specified then the mirror uses the same connection as the mirrored one.
If the tests look a bit redundant please let me know.

comment:14 Changed 5 weeks ago by eng. Ilian Iliev

Owner: changed from nobody to eng. Ilian Iliev
Status: newassigned

comment:15 Changed 3 weeks ago by Asif Saifuddin Auvi

Has patch: set

comment:16 Changed 2 weeks ago by Carlton Gibson

Patch needs improvement: set

Comments on PR

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