Code review updates

Author: bjester

CHANGELOG.md

@@ -5,6 +5,7 @@ List of the most important changes for each release.
 ## 0.6.11
 - Added deferred processing of foreign keys to allow bulk processing and to improve performance.
 - Eliminated extraneous SQL queries for the transfer session when querying for buffers.
+- Added database index to Store's partition field to improve querying performance.
 
 ## 0.6.10
 - Fixes Django migration issue introduced in 0.6.7 allowing nullable fields with PostgreSQL backends

morango/models/core.py

@@ -4,7 +4,6 @@
 import json
 import logging
 import uuid
-
 from collections import defaultdict
 from collections import namedtuple
 from functools import reduce
@@ -778,7 +777,7 @@ class RecordMaxCounterBuffer(AbstractCounter):
     model_uuid = UUIDField(db_index=True)
 
 
-ForeignKeyReference = namedtuple("ForeignKeyReference", ["from_pk", "to_pk"])
+ForeignKeyReference = namedtuple("ForeignKeyReference", ["from_field", "from_pk", "to_pk"])
 
 
 class SyncableModel(UUIDModelMixin):
@@ -905,8 +904,12 @@ def deferred_clean_fields(self):
             if getattr(self, field.attname) is None:
                 continue
             excluded_fields.append(field.name)
-            deferred_fks[field.related_model.__name__].append(
-                ForeignKeyReference(from_pk=self.pk, to_pk=getattr(self, field.attname))
+            deferred_fks[field.related_model._meta.verbose_name].append(
+                ForeignKeyReference(
+                    from_field=field.attname,
+                    from_pk=self.pk,
+                    to_pk=getattr(self, field.attname)
+                )
             )
 
         self.clean_fields(exclude=excluded_fields)

morango/sync/backends/utils.py

@@ -144,8 +144,8 @@ def bulk_insert(self, values):
 
     class Meta:
         """
-        HACK: some Django code bits require a model, only to access meta information, which we use
-        this to mimic that on this non-model class
+        HACK: some Django code bits require a model, only to access meta information, so we use
+        this to mimic a model class
         """
 
         db_tablespace = None

morango/sync/operations.py

@@ -11,6 +11,7 @@
 from django.db import connections
 from django.db import router
 from django.db import transaction
+from django.db.models import CharField
 from django.db.models import Q
 from django.db.models import signals
 from django.utils import timezone
@@ -278,7 +279,7 @@ def _validate_missing_store_foreign_keys(from_model_name, to_model_name, temp_ta
     """
     invalid_pks = []
     select_sql = """
-        SELECT t.from_pk, t.to_pk
+        SELECT t.from_field, t.from_pk, t.to_pk
         FROM {temp_table} t
         WHERE NOT EXISTS (
             SELECT 1
@@ -297,18 +298,27 @@ def _validate_missing_store_foreign_keys(from_model_name, to_model_name, temp_ta
     store_update_fields = [Store._meta.pk, store_deserialization_error]
 
     from_pk_field = temp_table.get_field("from_pk")
+    to_pk_field = temp_table.get_field("to_pk")
     update_values = []
     with connection.cursor() as c:
         c.execute(select_sql)
-        for from_pk, to_pk in c.fetchall():
-            err = "Error deserializing instance of {from_model} with id {from_pk}: missing {to_model} with id {to_pk}".format(
-                from_model=from_model_name,
-                from_pk=from_pk,
-                to_model=to_model_name,
-                to_pk=to_pk,
+        for from_field, from_pk, to_pk in c.fetchall():
+            err = dict(
+                {
+                    from_field: "{to_model_name} instance with id '{to_pk}' does not exist".format(
+                        to_model_name=to_model_name,
+                        to_pk=to_pk_field.to_python(to_pk),
+                    )
+                }
+            )
+            logger.warning(
+                "Error deserializing instance of {from_model} with id {from_pk}: {err}".format(
+                    from_model=from_model_name,
+                    from_pk=from_pk,
+                    err=str(err),
+                )
             )
-            logger.warning(err)
-            update_values.extend([from_pk, err])
+            update_values.extend([from_pk, str(err)])
             invalid_pks.append(from_pk_field.to_python(from_pk))
         if update_values:
             # update Store with errors
@@ -384,7 +394,11 @@ def _validate_store_foreign_keys(from_model_name, fk_references):
 
     for to_model_name, to_fk_references in fk_references.items():
         with TemporaryTable(
-            connection, "fks", from_pk=UUIDField(), to_pk=UUIDField()
+            connection,
+            "fks",
+            from_field=CharField(max_length=255),
+            from_pk=UUIDField(),
+            to_pk=UUIDField(),
         ) as temp_table:
             # insert all the FK references into a temp table in the database
             temp_table.bulk_insert([fks._asdict() for fks in to_fk_references])
@@ -517,7 +531,11 @@ def _deserialize_from_store(profile, skip_erroring=False, filter=None):
                             # deserialize for reasons other than broken FKs at this point
                             for fk_ref in fk_refs:
                                 if fk_ref.to_pk in excluded_list:
-                                    raise exceptions.ValidationError("{} with id {} failed to deserialize".format(fk_model, fk_ref.to_pk))
+                                    raise exceptions.ValidationError(
+                                        "{} with id {} failed to deserialize".format(
+                                            fk_model, fk_ref.to_pk
+                                        )
+                                    )
                             deferred_fks[fk_model].extend(fk_refs)
                     except (
                         exceptions.ValidationError,

tests/testapp/tests/sync/test_controller.py

@@ -1,16 +1,16 @@
+import contextlib
 import json
 import uuid
-import contextlib
+from test.support import EnvironmentVarGuard
 
 import factory
 import mock
 from django.test import SimpleTestCase
 from django.test import TestCase
 from facility_profile.models import Facility
-from facility_profile.models import MyUser
 from facility_profile.models import InteractionLog
+from facility_profile.models import MyUser
 from facility_profile.models import SummaryLog
-from test.support import EnvironmentVarGuard
 
 from ..helpers import serialized_facility_factory
 from ..helpers import TestSessionContext
@@ -641,7 +641,7 @@ def test_deserialization_of_model_with_missing_foreignkey_referent(self):
 
         new_log.refresh_from_db()
         self.assertTrue(new_log.dirty_bit)
-        self.assertIn("missing MyUser", new_log.deserialization_error)
+        self.assertIn("my user instance with id '{}'".format(data["user_id"]), new_log.deserialization_error)
 
     def test_deserialization_of_model_with_disallowed_null_foreignkey(self):