Skip to content

Commit

Permalink
Fix issues updating triggers in non-atomic migrations
Browse files Browse the repository at this point in the history 
Fields of trigger conditions can now have their types updated in non-atomic migrations.

Type: bug
  • Loading branch information
@kekekekule @wesleykendall
kekekekule authored and wesleykendall committed Nov 23, 2023
1 parent dbc5fed commit 888c4cf
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 15 deletions.
1 change: 1 addition & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,3 +109,4 @@ For information on setting up django-pgtrigger for development and contributing
- @ralokt
- @adamchainz
- @danifus
- @kekekekule
4 changes: 2 additions & 2 deletions pgtrigger/migrations.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ def temporarily_drop_trigger(self, trigger, table):
self.temporarily_dropped_triggers.add((trigger, table))

try:
with self.atomic, self.connection.cursor() as cursor:
with transaction.atomic(self.connection.alias), self.connection.cursor() as cursor:
cursor.execute(
f"""
SELECT pg_get_triggerdef(oid) FROM pg_trigger
Expand All @@ -377,7 +377,7 @@ def execute(self, *args, **kwargs):
"""
if self.is_altering_field_type:
try:
with self.atomic:
with transaction.atomic(self.connection.alias):
return super().execute(*args, **kwargs)
except Exception as exc:
match = re.search(
Expand Down
43 changes: 30 additions & 13 deletions pgtrigger/tests/test_migrations.py
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Tests behavior related to migrations"""
import pathlib
import shutil
import time

import ddf
import django.contrib.auth.models as auth_models
Expand Down Expand Up @@ -171,14 +172,31 @@ def test_makemigrations_existing_models(settings, request):
tm.save()


def make_migrations(atomic: bool):
"""Call makemigrations. Set atomic property of last migration if it is specified"""
call_command("makemigrations", name=f"a{time.time()}".replace(".", ""))

last_migration = sorted(pathlib.Path(migration_dir()).glob("[0-9]*.py"))[-1]
with open(last_migration, "r") as f:
contents = f.read()

contents = contents.replace(
"class Migration(migrations.Migration):\n",
f"class Migration(migrations.Migration):\n atomic = {atomic}\n",
)
with open(last_migration, "w") as f:
f.write(contents)


@pytest.mark.django_db(
databases=["default", "other", "receipt", "order", "sqlite"], transaction=True
)
@pytest.mark.usefixtures("reset_triggers", "reset_migrations")
@pytest.mark.order(-1) # This is a possibly leaky test if it fails midway. Always run last
# Run independently of core test suite since since this creates/removes models
@pytest.mark.independent
def test_makemigrations_create_remove_models(settings):
@pytest.mark.parametrize("atomic", [True, False])
def test_makemigrations_create_remove_models(settings, atomic):
"""
Tests migration scenarios where models are dynamically added and
removed.
Expand All @@ -192,10 +210,9 @@ def test_makemigrations_create_remove_models(settings):
###
# Make the initial trigger migrations
###
call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations

call_command("migrate")
assert_all_triggers_installed()

Expand Down Expand Up @@ -226,7 +243,7 @@ class DynamicTestModel(BaseDynamicTestModel):

test_models.DynamicTestModel = DynamicTestModel

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -252,7 +269,7 @@ class DynamicTestModel(BaseDynamicTestModel):

test_models.DynamicTestModel = DynamicTestModel

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -276,7 +293,7 @@ class DynamicTestModel(BaseDynamicTestModel):
]
DynamicTestModel._meta.original_attrs["triggers"] = DynamicTestModel._meta.triggers

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -296,7 +313,7 @@ class DynamicTestModel(BaseDynamicTestModel):
del apps.app_configs["tests"].models["dynamictestmodel"]
apps.clear_cache()

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand Down Expand Up @@ -330,7 +347,7 @@ class Meta:

test_models.DynamicProxyModel = DynamicProxyModel

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -354,7 +371,7 @@ class Meta:
]
DynamicProxyModel._meta.original_attrs["triggers"] = DynamicProxyModel._meta.triggers

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -374,7 +391,7 @@ class Meta:
del apps.app_configs["tests"].models["dynamicproxymodel"]
apps.clear_cache()

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -399,7 +416,7 @@ class Meta:
protected_model = ddf.G(auth_models.User)
protected_model.groups.add(ddf.G(auth_models.Group))

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -419,7 +436,7 @@ class Meta:
]
DynamicThroughModel._meta.original_attrs["triggers"] = DynamicThroughModel._meta.triggers

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand All @@ -436,7 +453,7 @@ class Meta:
del apps.app_configs["tests"].models["dynamicthroughmodel"]
apps.clear_cache()

call_command("makemigrations")
make_migrations(atomic)
num_expected_migrations += 1
assert num_migration_files() == num_expected_migrations
call_command("migrate")
Expand Down

0 comments on commit 888c4cf

Please sign in to comment.