[change] Making batch imports atomic #551

[stktyagi]

Used transaction.atomic() to ensure all operations inside succeed together. Incase of failure rollback will occure.

Fixes #551

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Closes #551.

Description of Changes

Added usage of transaction.atomic() functions to introduce atomicity and rollback in case of transaction operation failure.

Summary by CodeRabbit

• Bug Fixes

  • Batch user creation from CSV uploads is now atomic. All users in a batch are created together or not at all, preventing partial failures and ensuring database consistency.

• Tests

  • Added comprehensive tests to verify batch operation atomicity and proper rollback behavior on errors.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coveralls]

coverage: 97.271% (+0.002%) from 97.269%
when pulling 5b6c5da on stktyagi:issues/551
into 4053a46 on openwisp:master.

[nemesifier]

In #551 I wrote:

What if the import operation fails in the middle?
I believe we would end up with half users created.

Have you tried making the import fail on purpose in the middle to see what happens?
Can we recreate this in the tests?

Example from chatgpt to raise an error the second time the save method is called:

from unittest.mock import patch
from django.db import DatabaseError
from myapp.models import MyModel

def fail_on_second_call(real_method):
    calls = [True, False]  # First call real, second call raises

    def wrapper(self, *args, **kwargs):
        if calls.pop(0):
            return real_method(self, *args, **kwargs)
        else:
            raise DatabaseError("Simulated DB failure on second save")

    return wrapper

with patch.object(MyModel, 'save', autospec=True) as mocked_save:
    # Wrap original method
    mocked_save.side_effect = fail_on_second_call(MyModel.save)

    obj = MyModel(field="test")
    obj.save()  # ✅ Real save

    obj.name = "test2"
    try:
        obj.save()  # ❌ Fails with simulated DatabaseError
    except DatabaseError as e:
        print(f"Caught simulated error: {e}")

We could do something like this to cause a failure the second time the save_user() method is called. Without the patch, I expect that the save operations wouldn't be rolledback, and the first user created during the import would remain in the DB.

With the fix, that shouldn't happen and any imported data should be automatically rolledback.

Just adding the transaction.atomic() block could solve it, but it doesn't give us any confidence that it's really working, let's validate our assumptions.

[nemesifier]

Closing due to lack of follow up, feel free to follow up and reopen this.

[stktyagi]

Closing due to lack of follow up, feel free to follow up and reopen this.

hey @nemesifier, sorry for the delay, I must've missed this while updating my all other PRs. I have had some previous uncomitted work done here so I'll follow up with a commit now after some refinements.

[stktyagi]

In #551 I wrote:

What if the import operation fails in the middle? I believe we would end up with half users created.

Have you tried making the import fail on purpose in the middle to see what happens? Can we recreate this in the tests?

I've implemented 2 tests that check if both the add() and csvfile_upload() functions are truly atomic and would fail without transaction.atomic().

Example from chatgpt to raise an error the second time the save method is called:

from unittest.mock import patch
from django.db import DatabaseError
from myapp.models import MyModel

def fail_on_second_call(real_method):
    calls = [True, False]  # First call real, second call raises

    def wrapper(self, *args, **kwargs):
        if calls.pop(0):
            return real_method(self, *args, **kwargs)
        else:
            raise DatabaseError("Simulated DB failure on second save")

    return wrapper

with patch.object(MyModel, 'save', autospec=True) as mocked_save:
    # Wrap original method
    mocked_save.side_effect = fail_on_second_call(MyModel.save)

    obj = MyModel(field="test")
    obj.save()  # ✅ Real save

    obj.name = "test2"
    try:
        obj.save()  # ❌ Fails with simulated DatabaseError
    except DatabaseError as e:
        print(f"Caught simulated error: {e}")

We could do something like this to cause a failure the second time the save_user() method is called. Without the patch, I expect that the save operations wouldn't be rolledback, and the first user created during the import would remain in the DB.

With the fix, that shouldn't happen and any imported data should be automatically rolledback.

Just adding the transaction.atomic() block could solve it, but it doesn't give us any confidence that it's really working, let's validate our assumptions.

Athough making a test for failure for this was complicated as standard test tools hide the very database behavior needed to observe, in the test I am triggering a database IntegrityError via duplicate email constraints. This verifies that a failure midway through the import loop correctly triggers a ROLLBACK of both the partially created users and the RadiusBatch metadata record.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes introduce database transaction atomicity to batch user import operations in OpenWISP RADIUS. Both the CSV file upload and the internal add() method for user creation are now wrapped in transaction.atomic() blocks, ensuring that if any row fails during batch import, the entire batch is rolled back to prevent partial commits. Supporting tests validate rollback behavior on failures.

Changes

Cohort / File(s)	Summary
Core atomicity implementation openwisp_radius/base/models.py	Added transaction import; wrapped add() method's batch user creation and csvfile_upload() flow in transaction.atomic() blocks to ensure all-or-nothing batch processing with automatic rollback on exceptions.
Atomicity test suite openwisp_radius/tests/test_batch_add_users.py	Added TestBatchAtomicity class with two tests (test_csv_upload_total_rollback, test_add_method_internal_atomicity) to validate transaction rollback and verify no partial data is committed on failures; added get_user_model and IntegrityError imports; removed module-level cleanup statements.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 Hippity-hop, no more halfway drops!
Our batches now roll back when trouble pops,
All-or-nothing now, the data stays whole,
With atomic transactions, we're in control! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title '[change] Making batch imports atomic #551' clearly summarizes the main objective of wrapping batch operations in transaction.atomic().
Description check	✅ Passed	The description addresses the primary objective, references issue #551, and indicates checklist completion except for documentation; test coverage and atomicity implementation are described.
Linked Issues check	✅ Passed	The code changes successfully wrap batch import operations in transaction.atomic() to ensure atomicity and rollback on failure, directly addressing issue #551's requirement.
Out of Scope Changes check	✅ Passed	All changes are in-scope: transaction wrapping in base models, new atomicity tests, and import statements; no unrelated modifications are present.

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

openwisp_radius/tests/test_batch_add_users.py (1)

150-186: Well-designed tests for atomicity verification.

The test class correctly inherits from BaseTransactionTestCase and uses duplicate email constraints to trigger IntegrityError, which validates rollback behavior. Both tests effectively verify that partial imports are prevented.

Minor enhancement suggestion for test_add_method_internal_atomicity:

Consider also asserting that the batch itself still exists (since it was created outside the add() method's atomic block) but has no associated users. This would more comprehensively validate the atomic boundary:

🔎 Suggested enhancement

     def test_add_method_internal_atomicity(self):
         User = get_user_model()
         org = self._get_org()
         data = [
             ["user_one", "pass123", "duplicate@example.com", "John", "Doe"],
             ["user_two", "pass123", "duplicate@example.com", "Jane", "Doe"],
         ]
         batch = self._create_radius_batch(
             name="atomic-integrity-test",
             strategy="csv",
             organization=org,
             csvfile=self._get_csvfile(data),
         )
         with self.assertRaises(IntegrityError):
             batch.add(data)
         self.assertFalse(User.objects.filter(username="user_one").exists())
+        # Verify batch still exists but has no users (validates atomic boundary)
+        self.assertTrue(RadiusBatch.objects.filter(name="atomic-integrity-test").exists())
+        self.assertEqual(batch.users.count(), 0)

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4053a46 and 5b6c5da.

📒 Files selected for processing (2)

• openwisp_radius/base/models.py
• openwisp_radius/tests/test_batch_add_users.py

🧰 Additional context used

🧬 Code graph analysis (1)

openwisp_radius/tests/test_batch_add_users.py (4)

openwisp_radius/tests/__init__.py (2)

• FileMixin (174-200)
• _get_csvfile (179-188)

openwisp_radius/tests/mixins.py (1)

• BaseTransactionTestCase (234-237)

tests/openwisp2/sample_users/models.py (1)

• User (20-29)

openwisp_radius/base/models.py (2)

• csvfile_upload (1008-1019)
• add (985-1006)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)

• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.11 | django~=5.2.0

🔇 Additional comments (4)

openwisp_radius/base/models.py (3)

20-20: LGTM! Import added for atomicity support.

The transaction import is necessary for the atomic blocks introduced in this PR.

---

988-999: Excellent implementation of atomicity in batch user creation.

The atomic block correctly ensures all user creation operations succeed or roll back together. Notably, email sending (lines 999-1006) is correctly placed outside the transaction block, which is important because:

• Email sending is a side effect that cannot be rolled back
• Emails should only be sent after successful database commit
• Email failures shouldn't trigger database rollback

---

1016-1019: Nested transaction handling is correct.

The csvfile_upload() method wraps full_clean(), save(), and add() in a transaction.atomic() block. Since add() also uses transaction.atomic() at line 988, Django handles this with savepoints: the inner atomic block becomes a savepoint within the outer transaction. If any exception is raised within add() and propagates uncaught, the entire outer transaction is rolled back, ensuring the RadiusBatch record and any user records are not partially committed. The send_mail() calls outside the inner transaction guarantee emails are sent only after successful commit.

openwisp_radius/tests/test_batch_add_users.py (1)

3-5: LGTM! Imports added for atomicity tests.

The imports of get_user_model and IntegrityError are necessary for the new atomicity test cases.