Code

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#14068 closed (fixed)

Fixture loading issue with multi database setting

Reported by: linovia Owned by: nobody
Component: Core (Serialization) Version: 1.2
Severity: Keywords:
Cc: xordoquy@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Hi there,

Here is another issue with multi database setting and tests about fixture loading.
I have a setup with one legacy database and one dedicated to django.

In order to pass my tests, I load 2 fixture files. One is some administrative data for every test case and relating only to the django dedicated DB. The second fixture file is about test data which are loaded in the two files.

The fixtures load correctly on the django database but will fail to load in the legacy database.
The fixture loader notices that the administrative data fixture file has NO data and will rollback at this point.
While this might be fine in a single database setup, it shouldn't rollback here since data were already loaded in the other database.

# If the fixture we loaded contains 0 objects, assume that an
# error was encountered during fixture loading.
if objects_in_fixture == 0:
    sys.stderr.write(
        self.style.ERROR("No fixture data found for '%s'. (File format may be invalid.)" %
            (fixture_name)))
    transaction.rollback(using=using)
    transaction.leave_transaction_management(using=using)
    return

According to me:

  • it would make sense to issue a warning, but one should probably add the database name in order to discard any confusing error message.
  • one should rollback if such error occur (the user has been notified about it)

Maybe a better solution would be to let the test framework load a fixture in both database in a single call but that would require a major rework of the loaddata command and could possibly have side effects if one uses same class but different objects on different databases.

Attachments (1)

failingtest.tgz (3.5 KB) - added by linovia 4 years ago.
Project pointing inconsistencies.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by linovia

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

At some point it looks like the default behavior was not to rollback if there no object loaded from a fixture:

loaddata.py says:

        if object_count == 0:
            if verbosity > 0:
                print "No fixtures found."

unfortunatly, the rollback and function return are performed before.

comment:2 Changed 4 years ago by russellm

#14108 was a dupe, with a proposed patch.

comment:3 Changed 4 years ago by russellm

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

On closer inspection, I'm unclear as to the problem here.

Fixtures can be targeted at individual databases.

Empty fixtures, regardless of whether they are universal or targetted at a single database, are considered an error, since there's no reason to define an empty fixture; this error message is printed as part of the test process.

Marking invalid; if I've missed something important, please reopen, preferably with a clear test case that demonstrates why this is a problem.

comment:4 Changed 4 years ago by linovia

  • Resolution invalid deleted
  • Status changed from closed to reopened

In some cases, you can have a non empty fixture to be declared as empty.

Basically, the multi database dev have been made/tested with a master/slave configuration in mind. A django/legacy point of view didn't seem to be considered.

So let's say I have a django database in which I store django applications plus my own and another database which has legacy data.
Let's also consider my applications work with some custom models and a lot with legacy data.
At some point in my tests, I will probably create a fixture that will load data in only one of the two databases, let's say I load a set of complementary data in the legacy database.
And here comes the drama.
The fixture will load well for the legacy DB, but will be considered as empty for the django database.
As once the fixture is marked as empty, the fixture loaded will rollback the data. However, the test will not stop here and will keep on going.

So in the end, I am extremely confused with what happened.

  • I had en empty fixture message but no empty fixtures
  • I have half of my fixtures loaded in my test anyway (fixtures after the rollback are still loaded).
  • There is nowhere in the multi database page that states anything on the fixtures.
  • There is nowhere in the documentation that states that a fixture should have data for ALL the databases

I agree this isn't much an issue if you use the loaddata directly as you know pretty well what you did.
However, when this happens within a test case, it's just a nightmare to understand what happens.

I might write a project but it will takes some time to get it written.

comment:5 Changed 4 years ago by linovia

Ok, here's a test project (was easier than expected) to test with manage.py test myapp

Please, notice that:

  • only the default db is rolledback
  • depending on the fixture order, you won't have the same result (1 poll object in a test while none in the other).
  • none of the fixtures are really empty

Changed 4 years ago by linovia

Project pointing inconsistencies.

comment:6 Changed 4 years ago by linovia

I apologize for the spam, but the more I think about this rollback thing, the less I understand the reason.

If you load data with the command, the rollback doesn't have any sense since there will be no data.

If you load an empty fixture from a test case, the rollback doesn't have much sense either because fixtures after will still get loaded anyway.

comment:7 Changed 4 years ago by russellm

  • Component changed from Uncategorized to Serialization
  • Triage Stage changed from Unreviewed to Accepted

Now I understand the error case. The rollback isn't the actual problem. The error here is that there is now a difference between the number of objects in a fixture and the number of objects that are loaded into a given database.

If an error is found, it is rolled back because individual fixtures can be interdependent - objects in one fixture can reference objects in another. This means that if one fixture file has an error, the entire fixture setup must be called into question.

If a fixture contains no objects, it is an error. This is well established Django behavior that exists because certain serializers don't distinguish between an unparseable fixture file and an empty fixture file. In order to catch the error case, we need to raise an error if a fixture exists, but is "empty". This doesn't cause an error in the test, but it does cause many error messages to be raised to stderr.

The problem introduced by multi-db is that if your router has an allow_syncdb definition, it's possible for a fixture to have valid data, but none of those objects will be loaded into the database. However, the object count is only incremented when an object is actually loaded, not when it is parsed, so a fixture is reported as "empty" when no objects are loaded, rather than when no objects actually exist.

The fix for this is fairly easy; I should be able to commit a patch shortly.

As a side note; this problem can also be avoided using the per-database fixtures I described when I closed the ticket earlier. If a fixture nominates the fixture 'foo', and you provide 'foo.default.json' and 'foo.legacy.json', then only the relevant fixture will be loaded onto a given database.

comment:8 Changed 4 years ago by linovia

Thanks russell.

If you want I can work on a patch now that you gave the general idea. Just let me know.

comment:9 Changed 4 years ago by russellm

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

(In [13612]) Fixed #14068 -- Corrected error handling in loaddata when an allow_syncdb method is defined on the router. Thanks to Xavier Ordoquy for the report.

comment:10 Changed 4 years ago by russellm

(In [13613]) [1.2.X] Fixed #14068 -- Corrected error handling in loaddata when an allow_syncdb method is defined on the router. Thanks to Xavier Ordoquy for the report.

Backport of r13612 from 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.