Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#37] Explicitly handle transactions in the runner #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
from pathlib import Path

from django.core.management import BaseCommand, CommandError
from django.db import transaction

from django_setup_configuration.exceptions import ValidateRequirementsFailure
from django_setup_configuration.runner import SetupConfigurationRunner
Expand Down Expand Up @@ -40,7 +39,6 @@ def add_arguments(self, parser):
"from source, without actually executing the steps.",
)

@transaction.atomic
def handle(self, **options):
validate_only = options["validate_only"]
yaml_file = Path(options["yaml_file"]).resolve()
Expand Down
25 changes: 22 additions & 3 deletions django_setup_configuration/runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from typing import Any, Generator

from django.conf import settings
from django.db import transaction
from django.utils.module_loading import import_string

from pydantic import ValidationError
Expand Down Expand Up @@ -157,7 +158,8 @@ def _execute_step(
step_exc = None

try:
step.execute(config_model)
with transaction.atomic():
step.execute(config_model)
except BaseException as exc:
step_exc = exc
finally:
Expand Down Expand Up @@ -207,8 +209,25 @@ def execute_all_iter(self) -> Generator[StepExecutionResult, Any, None]:
Generator[StepExecutionResult, Any, None]: The results of each step's
execution.
"""
for step in self.enabled_steps:
yield self._execute_step(step)

# Not the most elegant approach to rollbacks, but it's preferable to the
# pitfalls of manual transaction management. We want all steps to run and only
# rollback at the end, hence intra-step exceptions are caught and persisted.
class Rollback(BaseException):
pass

try:
with transaction.atomic():
results = []
for step in self.enabled_steps:
result = self._execute_step(step)
results.append(result)
yield result

if any(result.run_exception for result in results):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I try this locally (for instance with Open Notificaties and have an incorrect service identifier) and add a breakpoint at line 227, the command output is:

CommandError: Error while executing step `Configuration for Notificaties API`: Service matching query does not exist. (identifier = incorrect)

And it seems that line 227 is never reached, probably because the entire thing crashes due to the CommandError which isn't caught. Is this a mistake in the implementation of the step in notifications_api_common or should that be handled here?

raise Rollback # Trigger the rollback
except Rollback:
pass

def execute_all(self) -> list[StepExecutionResult]:
"""
Expand Down
2 changes: 2 additions & 0 deletions tests/test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
from django_setup_configuration.test_utils import execute_single_step
from tests.conftest import TestStep, TestStepConfig

pytestmark = pytest.mark.django_db


def test_runner_raises_on_non_existent_step_module_path(test_step_yaml_path):
with pytest.raises(ConfigurationException):
Expand Down
2 changes: 2 additions & 0 deletions tests/test_test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from django_setup_configuration.test_utils import execute_single_step
from tests.conftest import TestStep

pytestmark = pytest.mark.django_db


def test_exception_during_execute_step_is_immediately_raised(
step_execute_mock,
Expand Down
105 changes: 105 additions & 0 deletions tests/test_transactions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
from unittest import mock

from django.contrib.auth.models import User

import pytest

from django_setup_configuration.configuration import BaseConfigurationStep
from django_setup_configuration.models import ConfigurationModel
from tests.conftest import TestStep

pytestmark = pytest.mark.django_db


def side_effect_test_func():
pass


class TransactionTestConfigurationModel(ConfigurationModel):

username: str


class TransactionTestConfigurationStep(
BaseConfigurationStep[TransactionTestConfigurationModel]
):

config_model = TransactionTestConfigurationModel
enable_setting = "transaction_test_configuration_enabled"
namespace = "transaction_test_configuration"
verbose_name = "Transaction Test Configuration"

def execute(self, model) -> None:
User.objects.create_user(
username=model.username,
password="secret",
)

side_effect_test_func()


@pytest.fixture()
def valid_config_object(test_step_valid_config):
return {
"transaction_test_configuration_enabled": True,
"transaction_test_configuration": {"username": "alice"},
} | test_step_valid_config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with this, isn't | an unsupported operand for two dicts?



def test_runner_rolls_back_all_on_failing_step(
runner_factory, valid_config_object, step_execute_mock
):
runner = runner_factory(
steps=[TransactionTestConfigurationStep, TestStep],
object_source=valid_config_object,
)
exc = Exception()
step_execute_mock.side_effect = exc

user_configuration_step_result, test_step_result = runner.execute_all()

# Initial run is rolled back, so no objects created
assert test_step_result.has_run
assert test_step_result.run_exception is exc

assert user_configuration_step_result.has_run
assert user_configuration_step_result.run_exception is None
assert User.objects.count() == 0

# Subsequent run does not raise, so the objects are created
step_execute_mock.side_effect = None

user_configuration_step_result, test_step_result = runner.execute_all()

assert test_step_result.has_run
assert test_step_result.run_exception is None

assert user_configuration_step_result.has_run
assert user_configuration_step_result.run_exception is None
assert User.objects.count() == 1


def test_runner_rolls_back_on_executing_single_step(
runner_factory, valid_config_object
):
runner = runner_factory(
steps=[TransactionTestConfigurationStep, TestStep],
object_source=valid_config_object,
)
with mock.patch("tests.test_transactions.side_effect_test_func") as m:
exc = Exception()
m.side_effect = exc

user_configuration_step_result = runner._execute_step(
runner.configured_steps[0]
)

assert user_configuration_step_result.has_run
assert user_configuration_step_result.run_exception is exc
assert User.objects.count() == 0

user_configuration_step_result = runner._execute_step(runner.configured_steps[0])

assert user_configuration_step_result.has_run
assert user_configuration_step_result.run_exception is None
assert User.objects.count() == 1
Loading