Skip to content

Commit 8f12d91

Browse files
committed
Resolve merge conflicts
2 parents 3f22ca7 + 635f8d9 commit 8f12d91

22 files changed

Lines changed: 1021 additions & 116 deletions

CHANGELOG.md

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,23 @@
22

33
List of the most important changes for each release.
44

5+
## 0.8.15
6+
- Improves buffer serialization performance by performing bulk counter lookup
7+
- Corrects an initialization issue with `SessionContext` that leads to an incorrect value for `is_push`
8+
- Save calls to Morango models during a sync are now scoped to only the changed fields
9+
10+
## 0.8.14
11+
- Adds utility for addressing immediate FK constraints caused by Django upgrade, automatically performed for morango models in a Django migration.
12+
- Adds retry behavior for low-level connection issues not handled by `urllib3` retries
13+
- Allows repeat pushes and pulls of buffers during transfer
14+
- Ignores HTTP 404 errors during sync or transfer session closure, which may occur if they're already closed
15+
16+
## 0.8.13
17+
- Removes multiprocessing fallback for RSA key generation to avoid leaving zombie processes; key generation now stays in-process
18+
19+
## 0.8.12
20+
- Fixes issue where dirty-bit isn't updated when calling save on a syncable model with `update_fields`
21+
522
## 0.8.11
623
- Adds additional `deserialization_exception` field to `Store` model to track the fully qualified exception path
724

morango/api/serializers.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
from collections import defaultdict
2+
13
from rest_framework import exceptions
24
from rest_framework import serializers
35
from rest_framework.fields import ReadOnlyField
@@ -157,11 +159,40 @@ class Meta:
157159
read_only_fields = fields
158160

159161

162+
class BufferListSerializer(serializers.ListSerializer):
163+
def to_representation(self, data):
164+
buffers = list(data)
165+
if buffers:
166+
rmcb_map = defaultdict(list)
167+
transfer_session_ids = {b.transfer_session_id for b in buffers}
168+
# Morango implementation will only ever call this for one transfer session ID at a time,
169+
# but a loop makes the code straightforward regardless and limits the quantity of
170+
# SQLite variables in use
171+
for transfer_session_id in transfer_session_ids:
172+
# bulk-fetch all RMCB records needed for this batch of buffers in a
173+
# single query, then cache the relevant subset on each buffer, instead
174+
# of letting each buffer's nested rmcb_list serializer issue its own query
175+
rmcb_queryset = RecordMaxCounterBuffer.objects.filter(
176+
transfer_session_id=transfer_session_id,
177+
model_uuid__in={
178+
b.model_uuid
179+
for b in buffers
180+
if b.transfer_session_id == transfer_session_id
181+
},
182+
)
183+
for rmcb in rmcb_queryset:
184+
rmcb_map[(transfer_session_id, rmcb.model_uuid)].append(rmcb)
185+
for buffer in buffers:
186+
buffer._rmcb_list = rmcb_map[(buffer.transfer_session_id, buffer.model_uuid)]
187+
return super(BufferListSerializer, self).to_representation(buffers)
188+
189+
160190
class BufferSerializer(serializers.ModelSerializer):
161191
rmcb_list = RecordMaxCounterBufferSerializer(many=True)
162192

163193
class Meta:
164194
model = Buffer
195+
list_serializer_class = BufferListSerializer
165196
fields = (
166197
"serialized",
167198
"deleted",

morango/deferrable_foreign_keys.py

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
"""
2+
Helpers for ensuring that existing SQLite foreign key constraints are
3+
``DEFERRABLE INITIALLY DEFERRED``.
4+
5+
Prior to Django 3.1, SQLite tables were created with immediate foreign key constraints, e.g.::
6+
7+
"store_model_id" char(32) NOT NULL REFERENCES "morango_store" ("id")
8+
9+
Since Django 3.1 the same column is created as::
10+
11+
"store_model_id" char(32) NOT NULL REFERENCES "morango_store" ("id") DEFERRABLE INITIALLY DEFERRED
12+
13+
Django relies on deferred constraint checking for correct cascade-deletion ordering. Databases
14+
created before Morango 0.8 (with Django 1.11) therefore retain immediate constraints, which can
15+
raise ``IntegrityError: FOREIGN KEY constraint failed`` during operations such as sync
16+
deserialization once foreign key enforcement is enabled at the database level (the default since
17+
Django 3.2).
18+
19+
The migration framework does not regenerate these constraints on its own because the deferrable
20+
clause is not part of the field definition that migrations track. This module rebuilds the affected
21+
tables so that a migrated database ends up with the same schema as a freshly created one.
22+
"""
23+
24+
import logging
25+
from typing import List
26+
from typing import Optional
27+
28+
from django.apps import apps as global_apps
29+
from django.apps.registry import Apps
30+
from django.db import connection
31+
from django.db import DatabaseError
32+
from django.db import router
33+
from django.db.backends.base.schema import BaseDatabaseSchemaEditor
34+
from django.db.backends.sqlite3.schema import DatabaseSchemaEditor
35+
36+
logger = logging.getLogger(__name__)
37+
38+
39+
def _get_table_sql(conn, table_name: str) -> Optional[str]:
40+
"""
41+
Return the ``CREATE TABLE`` statement stored by SQLite for ``table_name``, or ``None`` if the
42+
table does not exist.
43+
"""
44+
with conn.cursor() as cursor:
45+
cursor.execute(
46+
"SELECT sql FROM sqlite_master WHERE type = 'table' AND name = %s",
47+
[table_name],
48+
)
49+
row = cursor.fetchone()
50+
return row[0] if row else None
51+
52+
53+
def _table_has_immediate_foreign_key(conn, table_name: str) -> bool:
54+
"""
55+
Determine whether ``table_name`` has at least one foreign key constraint that is not deferrable.
56+
Tables are created with all constraints in the same style, so the presence of a ``REFERENCES``
57+
clause without ``DEFERRABLE`` indicates an immediate constraint that needs to be rebuilt.
58+
"""
59+
sql = _get_table_sql(conn, table_name)
60+
if not sql:
61+
return False
62+
references_count = sql.count("REFERENCES")
63+
if references_count == 0:
64+
return False
65+
# make sure that all FK columns (identified by references) have a deferrable constraint
66+
return references_count != sql.count("DEFERRABLE")
67+
68+
69+
class MakeForeignKeysDeferrable:
70+
"""
71+
Invokable utility class that rebuilds of any table belonging to one of ``*app_labels`` whose
72+
foreign key constraints are still immediate, so that they become
73+
``DEFERRABLE INITIALLY DEFERRED``.
74+
75+
This can be utilized by passing this class to ``RunPython`` in a Django migration, or by
76+
manually invoking its ``run`` method.
77+
78+
This is a no-op on non-SQLite backends (PostgreSQL already creates deferrable
79+
constraints) and on tables that already use deferrable constraints, so it is
80+
safe to apply to any database.
81+
"""
82+
83+
def __init__(
84+
self,
85+
include_app_labels: Optional[List[str]] = None,
86+
exclude_app_labels: Optional[List[str]] = None,
87+
):
88+
self.include_app_labels = include_app_labels
89+
self.exclude_app_labels = exclude_app_labels
90+
91+
def _iter_apps(self, apps: Apps):
92+
for app_config in apps.get_app_configs():
93+
if self.exclude_app_labels is not None and app_config.label in self.exclude_app_labels:
94+
continue
95+
if self.include_app_labels is None or app_config.label in self.include_app_labels:
96+
yield app_config
97+
98+
def __call__(self, apps: Apps, schema_editor: BaseDatabaseSchemaEditor):
99+
"""
100+
Loops through all relevant django apps and their models, checking if the model's table has
101+
FKs, and if so, runs the schema editor's utility that synchronizes the schema to the model,
102+
which will make FKs deferrable
103+
"""
104+
conn = schema_editor.connection
105+
# these should be in sync, but since we call `_remake_table`, specific to this schema editor
106+
# class, we ensure we have the correct instance to begin with
107+
if conn.vendor != "sqlite" or not isinstance(schema_editor, DatabaseSchemaEditor):
108+
return
109+
110+
for app_config in self._iter_apps(apps):
111+
for model in app_config.get_models(include_auto_created=True):
112+
if not router.allow_migrate_model(conn.alias, model):
113+
continue
114+
table_name = model._meta.db_table
115+
if not _table_has_immediate_foreign_key(conn, table_name):
116+
continue
117+
logger.info(
118+
"Rebuilding table %s to make foreign key constraints deferrable",
119+
table_name,
120+
)
121+
try:
122+
# Rebuilding the table from the current model state regenerates the column
123+
# definitions, which SQLite always emits with the deferrable clause (see
124+
# DatabaseSchemaEditor.sql_create_inline_fk). Only tested with Django 3.2
125+
schema_editor._remake_table(model)
126+
except DatabaseError as e:
127+
logger.error("Failed to rebuild table %s", table_name)
128+
logger.exception(e)
129+
130+
def run(self):
131+
"""
132+
Invoke this utility outside of Django migrations, manually passing in Django's app registry
133+
and schema editor.
134+
"""
135+
# since this method is meant for manual invocation, we let the editor create a transaction
136+
with connection.schema_editor(atomic=True) as schema_editor:
137+
self.__call__(global_apps, schema_editor)
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
"""
2+
Make existing foreign key constraints ``DEFERRABLE INITIALLY DEFERRED``.
3+
4+
Databases created before Morango 0.8 (with Django 1.11) have immediate SQLite foreign key
5+
constraints, which can break cascade deletions (e.g. during sync) now that foreign keys are enforced
6+
at the database level. See https://github.com/learningequality/kolibri/issues/14884.
7+
"""
8+
9+
from django.db import migrations
10+
11+
from morango.deferrable_foreign_keys import MakeForeignKeysDeferrable
12+
13+
14+
class Migration(migrations.Migration):
15+
dependencies = [
16+
("morango", "0003_store_deserialization_errors"),
17+
]
18+
19+
operations = [
20+
migrations.RunPython(
21+
MakeForeignKeysDeferrable(include_app_labels=["morango"]),
22+
migrations.RunPython.noop,
23+
),
24+
]
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
class Migration(migrations.Migration):
99
dependencies = [
10-
("morango", "0003_store_deserialization_errors"),
10+
("morango", "0004_deferrable_foreign_keys"),
1111
]
1212

1313
operations = [

morango/models/core.py

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,7 @@ def update_state(self, stage=None, stage_status=None):
322322
:type stage: morango.constants.transfer_stages.*|None
323323
:type stage_status: morango.constants.transfer_statuses.*|None
324324
"""
325+
update_fields = []
325326
if stage is not None:
326327
if self.transfer_stage and transfer_stages.stage(
327328
self.transfer_stage
@@ -332,13 +333,18 @@ def update_state(self, stage=None, stage_status=None):
332333
)
333334
)
334335
self.transfer_stage = stage
336+
update_fields.append("transfer_stage")
337+
335338
if stage_status is not None:
336339
self.transfer_stage_status = stage_status
337-
if stage is not None or stage_status is not None:
340+
update_fields.append("transfer_stage_status")
341+
342+
if update_fields:
338343
self.last_activity_timestamp = timezone.now()
339-
self.save()
344+
update_fields.append("last_activity_timestamp")
345+
self.save(update_fields=update_fields)
340346
self.sync_session.last_activity_timestamp = timezone.now()
341-
self.sync_session.save()
347+
self.sync_session.save(update_fields=["last_activity_timestamp"])
342348

343349
def delete_buffers(self):
344350
"""
@@ -565,6 +571,11 @@ class Meta:
565571
unique_together = ("transfer_session", "model_uuid")
566572

567573
def rmcb_list(self):
574+
# allow callers (e.g. BufferListSerializer) to batch-fetch RMCB records
575+
# for many buffers at once and cache them here, to avoid an N+1 query
576+
# pattern when serializing a large number of buffers
577+
if hasattr(self, "_rmcb_list"):
578+
return self._rmcb_list
568579
return RecordMaxCounterBuffer.objects.filter(
569580
model_uuid=self.model_uuid, transfer_session_id=self.transfer_session_id
570581
)
@@ -844,6 +855,11 @@ def save(self, update_dirty_bit_to=True, *args, **kwargs):
844855
self._morango_dirty_bit = True
845856
elif not update_dirty_bit_to:
846857
self._morango_dirty_bit = False
858+
859+
# ensure the dirty bit field is in the fields to update if present, to keep it in sync
860+
if update_dirty_bit_to is not None and kwargs.get("update_fields") is not None:
861+
kwargs["update_fields"] = set(kwargs["update_fields"]) | {"_morango_dirty_bit"}
862+
847863
super(SyncableModel, self).save(*args, **kwargs)
848864

849865
def delete(self, using=None, keep_parents=False, hard_delete=False, *args, **kwargs):

morango/models/fields/crypto.py

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -178,10 +178,7 @@ class PythonRSAKey(BaseKey):
178178
_private_key = None
179179

180180
def generate_new_key(self, keysize=2048):
181-
try:
182-
self._public_key, self._private_key = PYRSA.newkeys(keysize, poolsize=4)
183-
except: # noqa: E722
184-
self._public_key, self._private_key = PYRSA.newkeys(keysize)
181+
self._public_key, self._private_key = PYRSA.newkeys(keysize)
185182

186183
def _sign(self, message):
187184

morango/sync/context.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ def __init__(
5252

5353
if self.transfer_session:
5454
self.sync_session = transfer_session.sync_session or self.sync_session
55-
self.is_push = transfer_session.push or self.is_push
55+
self.is_push = transfer_session.push
5656
if transfer_session.filter:
5757
self.filter = transfer_session.get_filter()
5858

morango/sync/controller.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,7 @@ def _invoke_middleware(self, context, middleware):
254254
return context.stage_status
255255
except Exception as e:
256256
# always log the error itself
257-
logger.error(e)
257+
logger.exception(e)
258258
context.update(stage_status=transfer_statuses.ERRORED, error=e)
259259
# fire completed signal, after context update. handlers can use context to detect error
260260
signal.completed.fire(context=prepared_context or context)

0 commit comments

Comments
 (0)