Merge pull request #3969 from kobotoolbox/3967-asset-defer-content

Optimize code to avoid loading not required fields

Author: jnm

kobo/apps/reports/report_data.py

@@ -108,6 +108,7 @@ def _infer_version_id(submission):
     return pack, submission_stream
 
 
+# TODO validate if this function is still in used.
 def _vnames(asset, cache=False):
     if not cache or not hasattr(asset, '_available_report_uids'):
         content = deepcopy(asset.content)

kpi/exceptions.py

@@ -19,6 +19,10 @@ def __init__(self, *args, **kwargs):
         )
 
 
+class AssetAdjustContentError(Exception):
+    pass
+
+
 class AttachmentNotFoundException(Exception):
     pass
 

kpi/mixins/object_permission.py

@@ -209,8 +209,8 @@ def _get_effective_perms(
         # Add the calculated `delete_` permission for the owner
         content_type = ContentType.objects.get_for_model(self)
         if (
-            self.owner is not None
-            and (user is None or user.pk == self.owner.pk)
+            self.owner_id is not None
+            and (user is None or user.pk == self.owner_id)
             and (codename is None or codename.startswith('delete_'))
         ):
             matching_permissions = self.__get_permissions_for_content_type(
@@ -229,7 +229,7 @@ def _get_effective_perms(
                     # doesn't match exactly. Necessary because `Asset` has
                     # `delete_submissions` in addition to `delete_asset`
                     continue
-                effective_perms.add((self.owner.pk, perm_pk))
+                effective_perms.add((self.owner_id, perm_pk))
 
         # We may have calculated more permissions for anonymous users
         # than they are allowed to have. Remove them.
@@ -280,7 +280,7 @@ def _recalculate_inherited_perms(
             # if we use it when we're not supposed to
             objects_to_return = []
         # The owner gets every assignable permission
-        if self.owner is not None:
+        if self.owner_id is not None:
             for perm in Permission.objects.filter(
                 content_type=content_type,
                 codename__in=self.get_assignable_permissions(

kpi/models/asset.py

@@ -46,6 +46,7 @@
 )
 from kpi.deployment_backends.mixin import DeployableMixin
 from kpi.exceptions import (
+    AssetAdjustContentError,
     BadPermissionsException,
     DeploymentDataException,
 )
@@ -698,16 +699,46 @@ def save(
             )
             return
 
-        if self.content is None:
+        update_content_field = update_fields and 'content' in update_fields
+
+        # Raise an exception if we want to adjust asset content
+        # (i.e. `adjust_content` is True) but we are trying to update only
+        # certain fields and `content` is not part of them, or if we
+        # specifically ask to not adjust asset content, but trying to
+        # update only certain fields and `content` is one of them.
+        if (
+            (adjust_content and update_fields and 'content' not in update_fields)
+            or
+            (not adjust_content and update_content_field)
+        ):
+            raise AssetAdjustContentError
+
+        # If `content` is part of the updated fields, `summary` and
+        # `report_styles` must be too.
+        if update_content_field:
+            update_fields += ['summary', 'report_styles']
+            # Avoid duplicates
+            update_fields = list(set(update_fields))
+
+        # `self.content` must be the second condition. We do not want to get
+        # the value of `self.content` if first condition is false.
+        # The main purpose of this is avoid to load `self.content` when it is
+        # deferred (see `AssetNestedObjectViewsetMixin.asset`) and does not need
+        # to be updated.
+        if (
+            (not update_fields or update_content_field)
+            and self.content is None
+        ):
             self.content = {}
 
         # in certain circumstances, we don't want content to
         # be altered on save. (e.g. on asset.deploy())
         if adjust_content:
             self.adjust_content_on_save()
 
-        # populate summary
-        self._populate_summary()
+        # populate summary (only when required)
+        if not update_fields or update_fields and 'summary' in update_fields:
+            self._populate_summary()
 
         # infer asset_type only between question and block
         if self.asset_type in [ASSET_TYPE_QUESTION, ASSET_TYPE_BLOCK]:
@@ -721,7 +752,12 @@ def save(
                 elif row_count > 1:
                     self.asset_type = ASSET_TYPE_BLOCK
 
-        self._populate_report_styles()
+        # populate report styles (only when required)
+        if (
+            not update_fields
+            or update_fields and 'report_styles' in update_fields
+        ):
+            self._populate_report_styles()
 
         # Ensure `_deployment_data` is not saved directly
         try:

kpi/models/paired_data.py

@@ -161,7 +161,9 @@ def get_source(self) -> Union['Asset', None]:
         # Avoid circular import
         Asset = self.asset.__class__  # noqa
         try:
-            source_asset = Asset.objects.get(uid=self.source_uid)
+            source_asset = Asset.objects.only(
+                'asset_type', 'data_sharing', 'owner_id', '_deployment_data'
+            ).get(uid=self.source_uid)
         except Asset.DoesNotExist:
             return None
 
@@ -280,4 +282,3 @@ def update(self, updated_values):
             setattr(self, key, value)
 
         self.void_external_xml_cache()
-

kpi/utils/viewset_mixins.py

@@ -9,7 +9,9 @@ class AssetNestedObjectViewsetMixin:
     @property
     def asset(self):
         if not hasattr(self, '_asset'):
-            asset = get_object_or_404(Asset, uid=self.asset_uid)
+            asset = get_object_or_404(
+                Asset.objects.defer('content'), uid=self.asset_uid
+            )
             setattr(self, '_asset', asset)
         return self._asset
 

kpi/views/v2/asset_snapshot.py

@@ -46,6 +46,11 @@ class AssetSnapshotViewSet(OpenRosaViewSetMixin, NoUpdateModelViewSet):
     @property
     def asset(self):
         asset_snapshot = self.get_object()
+        # Calling `snapshot.asset.__class__` instead of `Asset` to avoid circular
+        # import
+        asset_snapshot.asset = asset_snapshot.asset.__class__.objects.defer(
+            'content'
+        ).get(pk=asset_snapshot.asset_id)
         return asset_snapshot.asset
 
     def filter_queryset(self, queryset):