Dont rollback transaction on errors in management/loaddata in "non-commit" mode.

[Vadim Fint]

loaddata command by default tries to use transaction for all it's work. But it accepts commit=False, which can made whole loaddata call part of outer transaction (this is usefull for call_command :)). But.. on any error - loaddata will close transaction in any way :(.

0. Start transaction
1. Create something in db
2. Call loaddata with commit=False (in try..except block -- we can have possible error and we know that)
3. Got an error while loaddata
4. Ooops.. loaddata rollbacks whole transaction, thus my changes in 1st step are gone
5. grrrrr.

But.. loaddata for easy using in non-interactive mode should also return something "okay" to callee, right now there is no way to determine that. But this is not covered by this ticket :).
p.s. not sure about component -- "Core framework" or "django-admin.py"?

[Vadim Fint]

fixup (r1).

[Vadim Fint]

auch, forgot about trac links (in attachment above "r1" means "revision 1 of patch") :).

[Vadim Fint]

0. Start transaction
1. Create something in db
2. Call loaddata with commit=False (in try..except block -- we can have possible error and we know that)
3. Got an error while loaddata
4. Ooops.. loaddata rollbacks whole transaction, thus my changes in 1st step are gone
5. grrrrr.

[Karen Tracey]

Note r8336 added this loaddata commit option (calling it "stealth" -- it's not documented and not available from the command line) with an eye to using it for implementing #8138 (speeding up tests by using transaction rollback instead of flushing/reloading the DB). As it turns out this loaddata option is not necessary given the way #8138 was ultimately implemented, but I didn't actually remove it. (I meant to ask if others thought it should be removed, but I seem to have forgotten to do that.) Given it's not needed for #8138, at this point I don't know if it would be better to remove it entirely or try to fix problems like this. I'm not exactly sure how you are going to proceed after loaddata runs into a problem but doesn't rollback -- it may have partially loaded fixtures into the DB...you're not going to want to ultimately commit that, are you?

Personally I think it would be better to remove the option entirely since I don't believe it was ever intended for outside use and the internal use it was thought to be needed for got implemented in a way that made it unnecessary. Other opinions?

[Alex Gaynor]

I agree it should be removed, partially loading fixtures seems like an all around bad idea, considering it gives way to inserting data without referential integrity, in addition to other concerns about incomplete data.

[Vadim Fint]

Note that partially loading textures is bad idea in any way - I'm completely agree with that. But aggressive rollbacks is a bad idea too.

I'm noticed this bug (feature?) to you, because one half of function tries to deal with graceful rollbacks and commits, another half - dont. This functionality should be either removed at all, or completed.

kmtracey, yes you are right. I dont want to commit changes to db at all, this is quite useful for testing (experimenting with (already working!) forked testrunner :)). In transaction test model (#8138, for example) such loaddata error raises hard to detect error -- somewhere in the test db backend will scream about out of sync in commands.

I'm not thought about that -- possibly where strict reason for not doing that.... but... what about simply replace explicit rollback/commit in loaddata to commit_unless_managed and rollback_unless_managed?

Also that should be a good idea to make some way to globally disable transaction management. #8138 introduces settings.DATABASE_SUPPORTS_TRANSACTIONS (which is AWFUL! what if my db user has no permissions to create new tables?), but it seem to be used in #8138 only right now. There is an ugly technique (again, from #8138) - replace transaction functions with nop().

[Russell Keith-Magee]

Karen - when I labeled this as a stealth option, it was because there isn't any use for this as an exposed command line option, and we don't have a place to document management command options that are available programatically but not from the command line. In this case, the option is a niche requirement - granted, it isn't needed for #8138, but I can conceive of ways that it could be used. If anyone is smart enough to realize that transactionless loaddata is what they need, they're smart enough to read the code and work out that it is possible. The code is well documented in this area, so it shouldn't be difficult to use the option (MockSoul certainly managed :-)

Alex - integrity isn't the issue here. If you're loading data in a transaction, you either get all the data or you get none of the data. By definition, you can't get into an invalid state. If you're not using a transaction, but you are using a database with referential integrity, then any non-trivial fixture (i.e., a fixture with forward references) won't even load, so you're going to hit problems pretty early. If your database doesn't have referential integrity, then you've already taken on the burden of validating references at runtime before use.

Put me down as a +0 for keeping this option; in which case, MockSoul's patch seems like the right thing to do. If we're deferring responsibility of the transaction to the external method, we should defer _everything_ to the external method. Coupled with #10200, that should give MockSoul (et al) everything they need to correctly handle transactions in an external method.

[Vadim Fint]

Change django to fit my needs is not hard. Change everything - is not hard. The main "oops" is that error is absolutely strange :). If loaddata will fail (silently for code, unless #10200) and it was surrounded by outer transaction - you will need to check all transactions before your error (which will be in first db query). I took ~1hour for me to understood that's going on. I just dont want somebody else to learn django in such way ;).

#10200 will be enough if there is no need to continue after loaddata fail (I suppose 99% users dont).

[(none)]

Milestone post-1.0 deleted

[Russell Keith-Magee]

(In [13978]) Fixed #10215 -- Ensured that there is parity between enter and leave transaction calls in loaddata when commit=False. The test case for this is the fixtures_regress unittests under MyISAM, which were failing previous to this fix. Thanks to MockSoul for the report.

[Russell Keith-Magee]

(In [13979]) [1.2.X] Fixed #10215 -- Ensured that there is parity between enter and leave transaction calls in loaddata when commit=False. The test case for this is the fixtures_regress unittests under MyISAM, which were failing previous to this fix. Thanks to MockSoul for the report.

Backport of r13978 from trunk.

[Russell Keith-Magee]

For the benefit of history: Karen and I discussed this on IRC. We came to the conclusion that although commit=False is a stealth option, it's been around for long enough that removing it wasn't really a viable option. Correcting this problem still leaves the issue of how to appropriately identify and handle fixture errors in external code (See #10200), but that's a separate problem.

[Vadim Fint]

That was a long waiting.. :) Thanks, guys!