Skip to content

Commit 101e200

Browse files
rtibblesclaude
andcommitted
feat(migrations): add almost zero-downtime migration capability and linting
- CI linting of new migrations on pull requests - Declarative dual-write trigger decorator (mirror_field) - Reusable batched-backfill command (idempotent, resumable, throttled) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LfZvkigk8hdsKdEif3hzBi
1 parent 8a56525 commit 101e200

11 files changed

Lines changed: 410 additions & 2 deletions

File tree

.github/workflows/pythontest.yml

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,8 @@ jobs:
6262
- 6379:6379
6363
steps:
6464
- uses: actions/checkout@v6
65+
with:
66+
fetch-depth: 0
6567
- name: Set up minio
6668
run: |
6769
docker run -d -p 9000:9000 --name minio \
@@ -79,6 +81,17 @@ jobs:
7981
run: |
8082
# Use uv to install dependencies directly from requirements files
8183
uv pip sync requirements.txt requirements-dev.txt
84+
- name: Lint new migrations for unsafe operations
85+
if: github.event_name == 'pull_request'
86+
env:
87+
BASE_REF: ${{ github.base_ref }}
88+
DJANGO_SETTINGS_MODULE: contentcuration.not_production_settings
89+
run: |
90+
set -euo pipefail
91+
git fetch --no-tags origin "$BASE_REF"
92+
base="$(git merge-base "origin/$BASE_REF" HEAD)"
93+
test -n "$base"
94+
python contentcuration/manage.py lintmigrations --git-commit-id "$base" --no-cache --warnings-as-errors
8295
- name: Test pytest
8396
run: |
8497
sh -c './contentcuration/manage.py makemigrations --check'
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import hashlib
2+
3+
import pgtrigger
4+
5+
6+
def mirror_field(source, target):
7+
"""Mirror Django field `source` into `target` via a BEFORE INSERT/UPDATE
8+
trigger (expand/contract dual-write)."""
9+
10+
def decorator(model):
11+
source_col = model._meta.get_field(source).column
12+
target_col = model._meta.get_field(target).column
13+
name = "mirror_{}_to_{}".format(source_col, target_col)
14+
if len(name) > 43: # stay safely under pgtrigger's trigger-name limit
15+
digest = hashlib.sha1(
16+
"{}_{}".format(source_col, target_col).encode()
17+
).hexdigest()[:8]
18+
name = "mirror_{}".format(digest)
19+
# Change-guard (IS DISTINCT FROM): keeps a read cutover from clobbering
20+
# writes to the repointed column with the stale source value.
21+
trigger = pgtrigger.Trigger(
22+
name=name,
23+
when=pgtrigger.Before,
24+
operation=pgtrigger.Insert | pgtrigger.Update,
25+
func="IF NEW.{s} IS DISTINCT FROM OLD.{s} THEN NEW.{t} = NEW.{s}; END IF; RETURN NEW;".format(
26+
s=source_col, t=target_col
27+
),
28+
)
29+
return pgtrigger.register(trigger)(model)
30+
31+
return decorator
Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,100 @@
1+
from django.apps import apps
2+
from django.core.exceptions import FieldDoesNotExist
3+
from django.core.management.base import BaseCommand
4+
from django.core.management.base import CommandError
5+
from django.db import transaction
6+
from django.db.models import F
7+
8+
9+
class Command(BaseCommand):
10+
help = (
11+
"Idempotent, resumable online backfill of one column into another, in batches."
12+
)
13+
14+
def add_arguments(self, parser):
15+
parser.add_argument("--model", required=True, help="app_label.ModelName")
16+
parser.add_argument("--source-field", required=True)
17+
parser.add_argument("--target-field", required=True)
18+
parser.add_argument("--batch-size", type=int, default=10000)
19+
parser.add_argument("--start-id", default=None, help="resume from this pk")
20+
parser.add_argument(
21+
"--progress-check",
22+
action="store_true",
23+
help="report unbackfilled rows, exit nonzero if any",
24+
)
25+
26+
def _resolve_model_fields(self, model_label, source, target):
27+
try:
28+
model = apps.get_model(model_label)
29+
except (LookupError, ValueError) as e:
30+
raise CommandError("Bad --model {!r}: {}".format(model_label, e))
31+
try:
32+
model._meta.get_field(source)
33+
model._meta.get_field(target)
34+
except FieldDoesNotExist as e:
35+
raise CommandError(str(e))
36+
return model
37+
38+
def _batch_end_pk(self, queryset, pk_name, start_pk, batch_size):
39+
"""Last pk of the batch of `batch_size` rows starting at `start_pk`.
40+
41+
Returns None when fewer than `batch_size` rows remain at/after
42+
`start_pk` — the final, short batch. Keyset paging by pk, so it works
43+
for any pk type (int or UUID).
44+
"""
45+
return (
46+
queryset.filter(pk__gte=start_pk)
47+
.order_by(pk_name)
48+
.values_list("pk", flat=True)[batch_size - 1 : batch_size]
49+
.first()
50+
)
51+
52+
def handle(self, *args, **options):
53+
if options["batch_size"] < 1:
54+
raise CommandError("--batch-size must be >= 1")
55+
source = options["source_field"]
56+
target = options["target_field"]
57+
model = self._resolve_model_fields(options["model"], source, target)
58+
59+
pk_name = model._meta.pk.name
60+
batch_size = options["batch_size"]
61+
only_unfilled = {target + "__isnull": True, source + "__isnull": False}
62+
unfilled = model.objects.filter(**only_unfilled)
63+
unfilled_pks = unfilled.order_by(pk_name).values_list("pk", flat=True)
64+
65+
if options["progress_check"]:
66+
# exists(), not count() — the target table can have millions of rows.
67+
if unfilled.exists():
68+
raise CommandError("backfill incomplete: rows still pending")
69+
self.stdout.write("Backfill complete: no rows pending.")
70+
return
71+
72+
# Start at the first unfilled pk (>= --start-id if given); re-runs and
73+
# resumes skip straight past an already-filled prefix.
74+
batch_start = unfilled_pks
75+
if options["start_id"] is not None:
76+
batch_start = batch_start.filter(pk__gte=options["start_id"])
77+
batch_start = batch_start.first()
78+
79+
total = 0
80+
while batch_start is not None:
81+
batch_end = self._batch_end_pk(
82+
model.objects, pk_name, batch_start, batch_size
83+
)
84+
if batch_end is None:
85+
window = {"pk__gte": batch_start}
86+
else:
87+
window = {"pk__gte": batch_start, "pk__lte": batch_end}
88+
with transaction.atomic():
89+
total += model.objects.filter(**window, **only_unfilled).update(
90+
**{target: F(source)}
91+
)
92+
self.stdout.write(
93+
"backfilled through pk={} (updated {} so far)".format(
94+
batch_start if batch_end is None else batch_end, total
95+
)
96+
)
97+
if batch_end is None:
98+
break
99+
batch_start = unfilled_pks.filter(pk__gt=batch_end).first()
100+
self.stdout.write("Done. {} rows updated.".format(total))

contentcuration/contentcuration/not_production_settings.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,5 +20,14 @@
2020

2121
AWS_AUTO_CREATE_BUCKET = True
2222

23+
INSTALLED_APPS += ("django_migration_linter",) # noqa F405
24+
25+
MIGRATION_LINTER_OPTIONS = {
26+
"exclude_apps": [
27+
"kolibri_content"
28+
], # SQLite content-export app; not on the safe-DDL Postgres backend
29+
"sql_analyser": "postgresql",
30+
}
31+
2332
# Use local instance for curriculum automation for development
2433
CURRICULUM_AUTOMATION_API_URL = "http://localhost:8000"

contentcuration/contentcuration/settings.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@
9292
"django_celery_results",
9393
"kolibri_public",
9494
"automation",
95+
"pgtrigger",
9596
)
9697

9798
SESSION_ENGINE = "django.contrib.sessions.backends.cached_db"
Lines changed: 191 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
import uuid
2+
from io import StringIO
3+
from unittest.mock import patch
4+
5+
from django.core.management import call_command
6+
from django.core.management import CommandError
7+
from django.db import connection
8+
from django.db import models
9+
from django.db.models import F
10+
from django.test import SimpleTestCase
11+
from django.test import TransactionTestCase
12+
from django.test.utils import isolate_apps
13+
14+
15+
def _make_probe_class():
16+
class Probe(models.Model):
17+
source = models.IntegerField(null=True)
18+
shadow = models.IntegerField(null=True)
19+
20+
class Meta:
21+
app_label = "contentcuration"
22+
23+
return Probe
24+
25+
26+
def _make_uuid_probe_class():
27+
class UUIDProbe(models.Model):
28+
id = models.UUIDField(primary_key=True, default=uuid.uuid4)
29+
source = models.IntegerField(null=True)
30+
shadow = models.IntegerField(null=True)
31+
32+
class Meta:
33+
app_label = "contentcuration"
34+
35+
return UUIDProbe
36+
37+
38+
@isolate_apps("contentcuration")
39+
class BackfillColumnTestCase(TransactionTestCase):
40+
def _create_model(self, model):
41+
with connection.schema_editor(atomic=False) as editor:
42+
editor.create_model(model)
43+
self.addCleanup(self._delete_model, model)
44+
45+
def _delete_model(self, model):
46+
with connection.schema_editor(atomic=False) as editor:
47+
editor.delete_model(model)
48+
49+
def _run_backfill(self, model, **kwargs):
50+
out = StringIO()
51+
with patch(
52+
"contentcuration.management.commands.backfill_column.apps",
53+
model._meta.apps,
54+
):
55+
call_command(
56+
"backfill_column",
57+
stdout=out,
58+
model="contentcuration.{}".format(model.__name__),
59+
source_field="source",
60+
target_field="shadow",
61+
**kwargs,
62+
)
63+
return out.getvalue()
64+
65+
def _assert_all_synced(self, model):
66+
self.assertEqual(
67+
model.objects.count(), model.objects.filter(shadow=F("source")).count()
68+
)
69+
70+
def _assert_synced_from(self, model, resume_pk):
71+
below = model.objects.filter(pk__lt=resume_pk)
72+
at_or_above = model.objects.filter(pk__gte=resume_pk)
73+
self.assertEqual(below.count(), below.filter(shadow__isnull=True).count())
74+
self.assertEqual(
75+
at_or_above.count(), at_or_above.filter(shadow=F("source")).count()
76+
)
77+
78+
def test_backfills_all_rows(self):
79+
Probe = _make_probe_class()
80+
self._create_model(Probe)
81+
for i in range(1, 6):
82+
Probe.objects.create(source=i * 10, shadow=None)
83+
84+
# batch_size=2 over 5 rows exercises the multi-batch loop.
85+
self._run_backfill(Probe, batch_size=2)
86+
87+
self._assert_all_synced(Probe)
88+
89+
def test_idempotent(self):
90+
Probe = _make_probe_class()
91+
self._create_model(Probe)
92+
for i in range(1, 4):
93+
Probe.objects.create(source=i * 10, shadow=None)
94+
95+
self._run_backfill(Probe, batch_size=10)
96+
output = self._run_backfill(Probe, batch_size=10)
97+
98+
self.assertIn("Done. 0 rows updated.", output)
99+
self._assert_all_synced(Probe)
100+
101+
def test_resumable(self):
102+
Probe = _make_probe_class()
103+
self._create_model(Probe)
104+
objs = sorted(
105+
[Probe.objects.create(source=i * 10, shadow=None) for i in range(1, 6)],
106+
key=lambda o: o.pk,
107+
)
108+
resume_pk = objs[2].pk
109+
110+
self._run_backfill(Probe, batch_size=10, start_id=resume_pk)
111+
112+
self._assert_synced_from(Probe, resume_pk)
113+
114+
def test_null_source_safe(self):
115+
Probe = _make_probe_class()
116+
self._create_model(Probe)
117+
Probe.objects.create(source=None, shadow=None)
118+
Probe.objects.create(source=42, shadow=None)
119+
120+
output = self._run_backfill(Probe, batch_size=10)
121+
122+
self.assertIn("Done.", output)
123+
self.assertIsNone(Probe.objects.get(source__isnull=True).shadow)
124+
self.assertEqual(Probe.objects.get(source=42).shadow, 42)
125+
126+
def test_backfills_uuid_pk_across_batches(self):
127+
"""Regression: paging must not assume an integer pk (File has a UUID pk)."""
128+
UUIDProbe = _make_uuid_probe_class()
129+
self._create_model(UUIDProbe)
130+
for i in range(1, 6):
131+
UUIDProbe.objects.create(source=i * 10, shadow=None)
132+
133+
# batch_size=2 forces the lower-bound advance where integer arithmetic on a
134+
# UUID pk would blow up.
135+
self._run_backfill(UUIDProbe, batch_size=2)
136+
137+
self._assert_all_synced(UUIDProbe)
138+
139+
def test_resumable_uuid_pk(self):
140+
"""--start-id must accept a UUID and resume from it."""
141+
UUIDProbe = _make_uuid_probe_class()
142+
self._create_model(UUIDProbe)
143+
objs = sorted(
144+
[UUIDProbe.objects.create(source=i * 10, shadow=None) for i in range(1, 6)],
145+
key=lambda o: o.pk,
146+
)
147+
resume_pk = objs[2].pk
148+
149+
self._run_backfill(UUIDProbe, batch_size=10, start_id=str(resume_pk))
150+
151+
self._assert_synced_from(UUIDProbe, resume_pk)
152+
153+
def test_progress_check_passes_when_complete(self):
154+
Probe = _make_probe_class()
155+
self._create_model(Probe)
156+
Probe.objects.create(source=1, shadow=1)
157+
Probe.objects.create(source=None, shadow=None) # null source doesn't count
158+
159+
output = self._run_backfill(Probe, progress_check=True)
160+
161+
self.assertIn("no rows pending", output)
162+
163+
def test_progress_check_fails_and_writes_nothing_when_incomplete(self):
164+
Probe = _make_probe_class()
165+
self._create_model(Probe)
166+
Probe.objects.create(source=7, shadow=None)
167+
168+
with self.assertRaisesRegex(
169+
CommandError, "backfill incomplete: rows still pending"
170+
):
171+
self._run_backfill(Probe, progress_check=True)
172+
173+
# --progress-check must not write
174+
self.assertIsNone(Probe.objects.get(source=7).shadow)
175+
176+
177+
class BackfillColumnArgValidationTestCase(SimpleTestCase):
178+
def _call(self, **kwargs):
179+
call_command(
180+
"backfill_column",
181+
model="contentcuration.Channel",
182+
source_field="name",
183+
target_field="name",
184+
**kwargs,
185+
)
186+
187+
def test_non_positive_batch_size_raises(self):
188+
for bad in (0, -1):
189+
with self.subTest(batch_size=bad):
190+
with self.assertRaisesRegex(CommandError, "--batch-size must be >= 1"):
191+
self._call(batch_size=bad)

0 commit comments

Comments
 (0)