Restore transactions for data import #480
Conversation
…dle semantics of use_transactions to actually only use a transaction when set to True or when dry_run is True. Set default of use_transactions to True since that is the functional effect of using @atomic, so existing clients of the library are not impacted.
…or IMPORT_EXPORT_USE_TRANSACTIONS to True, set default value for use_transactions to None. Means code will only break for clients who have have set IMPORT_EXPORT_USE_TRANSACTIONS to False but were still expecting atomicity on import_data (i.e. hopefully no one!)
|
This code looks good. Not sure about removing the comments in before_import. Really like the sane defaults of this 👍 |
|
@arj03 The comments in both |
|
Thanks for your work @thauk-copperleaf I am little concered that this brings some backward incompatible changes:
What do you think? |
|
1-- Because |
…re-transactions-for-data-import # Conflicts: # import_export/resources.py
|
Since the semantics of We pass |
|
Fixes #411 |
|
Also fixes #488 :) |
PR django-import-export#480 introduced proper handling of transactions during import, but with additional check if DB engine does support transactions. When transaction is already in atomic block (for example by using `ATOMIC_REQUESTS` in Django DB conf) and `import_data` performs check if DB support transaction, in some cases (like MySQL), this check tries to call `set_autocommit`, which is forbidden in atomic block. This fix it not to use `supports_transaction` at all (it's not used in whole Django at all either, besides tests) - previously user receives `ImproperlyConfigured` exception - now `transaction.atomic` will go through silently (for example in MyISAM case) or raise another exception to user, but it'll work for all engines that support DB transactions when transaction block is already entered.
PR django-import-export#480 introduced proper handling of transactions during import, but with additional check if DB engine does support transactions. When transaction is already in atomic block (for example by using `ATOMIC_REQUESTS` in Django DB conf) and `import_data` performs check if DB support transaction, in some cases (like MySQL), this check tries to call `set_autocommit`, which is forbidden in atomic block. This fix it not to use `supports_transaction` at all (it's not used in whole Django at all either, besides tests) - previously user receives `ImproperlyConfigured` exception - now `transaction.atomic` will go through silently (for example in MyISAM case) or raise another exception to user, but it'll work for all engines that support DB transactions when transaction block is already entered.
Commit e9869f0 added the
@atomicdecorator toResource.import_data(), which resulted in every import being performed atomically, irrespective of the value ofuse_transactions.This restores the sematics for
use_transactionsto be used correctly, but still wraps the call toResource.import_data()with thetransaction.atomiccontext manager when necessary, to ensure atomic behaviour for existing clients of the library.To achieve this,
Resource.import_data()is refactored to have an "outer" wrapper to perform the atomicity check, and the remainder of the function as an "inner".