From caee09f7ebefb2953ada07c05995dc3a9faf497e Mon Sep 17 00:00:00 2001
From: janmax <j.michal@stud.uni-goettingen.de>
Date: Sat, 24 Mar 2018 18:27:04 +0100
Subject: [PATCH] Used Django Silk to optimize most database queries
* All views (except the feedback view) do not have a N+1 issue
* Some serializer method fields are now precomputed in annotations
* TODO: Automate the performance tests to ensure performance stays high
* Ran isort
---
.gitignore | 1 +
core/migrations/0009_auto_20180320_2335.py | 3 +-
core/models.py | 57 +++++++++-----
core/serializers/feedback.py | 26 +++++--
core/serializers/submission.py | 4 +-
core/serializers/tutor.py | 30 +++-----
core/tests/test_custom_subscription_filter.py | 5 +-
core/tests/test_tutor_api_endpoints.py | 76 +++++++++++++++----
core/views/common_views.py | 26 +++++--
core/views/export.py | 8 +-
core/views/feedback.py | 16 ++--
11 files changed, 168 insertions(+), 84 deletions(-)
diff --git a/.gitignore b/.gitignore
index 03e37b83..6ea0d4bc 100644
--- a/.gitignore
+++ b/.gitignore
@@ -38,3 +38,4 @@ anon-export/
# node
node_modules
secret
+dump.sql
diff --git a/core/migrations/0009_auto_20180320_2335.py b/core/migrations/0009_auto_20180320_2335.py
index 692cd7e2..20c74196 100644
--- a/core/migrations/0009_auto_20180320_2335.py
+++ b/core/migrations/0009_auto_20180320_2335.py
@@ -1,8 +1,9 @@
# Generated by Django 2.0.2 on 2018-03-20 23:35
-import core.models
from django.db import migrations, models
+import core.models
+
class Migration(migrations.Migration):
diff --git a/core/models.py b/core/models.py
index 406bffc9..49ebf182 100644
--- a/core/models.py
+++ b/core/models.py
@@ -13,10 +13,10 @@ from collections import OrderedDict
from random import randrange
from typing import Dict
+from django.conf import settings
from django.contrib.auth import get_user_model
-from django.contrib.auth.models import AbstractUser
+from django.contrib.auth.models import AbstractUser, UserManager
from django.db import models, transaction
-from django.conf import settings
from django.db.models import (BooleanField, Case, Count, F, IntegerField, Q,
QuerySet, Sum, Value, When)
from django.db.models.functions import Coalesce
@@ -164,6 +164,27 @@ class SubmissionType(models.Model):
).order_by('name')
+class TutorManager(UserManager):
+
+ def get_queryset(self):
+ return super().get_queryset().filter(role=UserAccount.TUTOR)
+
+ def with_feedback_count(self):
+ def _get_counter(stage):
+ return Count(Case(
+ When(
+ Q(subscriptions__feedback_stage=stage) &
+ Q(subscriptions__assignments__is_done=True),
+ then=Value(1))),
+ output_field=IntegerField())
+
+ return self.get_queryset() \
+ .annotate(feedback_created=_get_counter(
+ SubmissionSubscription.FEEDBACK_CREATION)) \
+ .annotate(feedback_validated=_get_counter(
+ SubmissionSubscription.FEEDBACK_VALIDATION))
+
+
class UserAccount(AbstractUser):
"""
An abstract base class implementing a fully featured User model with
@@ -172,13 +193,6 @@ class UserAccount(AbstractUser):
Username and password are required. Other fields are optional.
"""
- user_id = models.UUIDField(primary_key=True,
- default=uuid.uuid4,
- editable=False)
-
- fullname = models.CharField('full name', max_length=70, blank=True)
- is_admin = models.BooleanField(default=False)
-
STUDENT = 'Student'
TUTOR = 'Tutor'
REVIEWER = 'Reviewer'
@@ -189,8 +203,20 @@ class UserAccount(AbstractUser):
(REVIEWER, 'reviewer')
)
+ # Fields
role = models.CharField(max_length=50, choices=ROLE_CHOICES)
+ user_id = models.UUIDField(primary_key=True,
+ default=uuid.uuid4,
+ editable=False)
+
+ fullname = models.CharField('full name', max_length=70, blank=True)
+ is_admin = models.BooleanField(default=False)
+
+ # Managers
+ objects = UserManager()
+ tutors = TutorManager()
+ # Helper methods
def is_student(self):
return self.role == 'Student'
@@ -200,6 +226,8 @@ class UserAccount(AbstractUser):
def is_reviewer(self):
return self.role == 'Reviewer'
+ # All of these methods are deprecated and should be replaced by custom
+ # Managers (see tutor manager)
@classmethod
def get_students(cls):
return cls.objects.filter(role=cls.STUDENT)
@@ -260,11 +288,7 @@ class StudentInfo(models.Model):
self.save()
def score_per_submission(self) -> Dict[str, int]:
- """ TODO: get rid of it and use an annotation.
-
- Returns:
- TYPE: Description
- """
+ """ TODO: get rid of it and use an annotation. """
if self.submissions.all():
return OrderedDict({
s.type: s.feedback.score if hasattr(s, 'feedback') else 0
@@ -393,10 +417,7 @@ class Submission(models.Model):
ordering = ('type__name',)
def __str__(self) -> str:
- return "Submission of type '{}' from Student '{}'".format(
- self.type,
- self.student
- )
+ return "Submission {}".format(self.pk)
class Feedback(models.Model):
diff --git a/core/serializers/feedback.py b/core/serializers/feedback.py
index 9a4317ff..43db2622 100644
--- a/core/serializers/feedback.py
+++ b/core/serializers/feedback.py
@@ -88,14 +88,26 @@ class FeedbackSerializer(DynamicFieldsModelSerializer):
feedback_stage_for_user = serializers.SerializerMethodField()
def get_feedback_stage_for_user(self, obj):
- if 'request' in self.context:
- assignments = obj.of_submission.assignments.filter( # noqa
- subscription__owner=self.context['request'].user
- )
- if len(assignments) == 0:
- return None
- else:
+ """ Search for the assignment of this feedback and report in which
+ stage the tutor has worked on it.
+
+ Note: This method is unorthodox since it mingles the rather dump
+ feedback object with assignment logic. The reverse lookups in the
+ method are not pre-fetched. Remove if possible. """
+ if 'request' not in self.context:
+ return
+
+ # This is only required for tutors
+ user = self.context['request'].user
+ if user.role == models.UserAccount.REVIEWER:
+ return None
+
+ assignments = obj.of_submission.assignments.filter(
+ subscription__owner=user)
+
+ if assignments.count() == 0:
return None
+
return assignments[0].subscription.feedback_stage
@transaction.atomic
diff --git a/core/serializers/submission.py b/core/serializers/submission.py
index 8e0b1d08..8366d173 100644
--- a/core/serializers/submission.py
+++ b/core/serializers/submission.py
@@ -2,9 +2,9 @@ from rest_framework import serializers
from core.models import Submission
from core.serializers import (DynamicFieldsModelSerializer, FeedbackSerializer,
- VisibleCommentFeedbackSerializer,
SubmissionTypeListSerializer,
- SubmissionTypeSerializer, TestSerializer)
+ SubmissionTypeSerializer, TestSerializer,
+ VisibleCommentFeedbackSerializer)
class SubmissionNoTextFieldsSerializer(DynamicFieldsModelSerializer):
diff --git a/core/serializers/tutor.py b/core/serializers/tutor.py
index 7ebf5f17..c661b097 100644
--- a/core/serializers/tutor.py
+++ b/core/serializers/tutor.py
@@ -1,12 +1,13 @@
import logging
+import django.contrib.auth.password_validation as validators
+from django.core import exceptions
from rest_framework import serializers
-from .generic import DynamicFieldsModelSerializer
-from util.factories import GradyUserFactory
+
from core import models
-from django.core import exceptions
+from util.factories import GradyUserFactory
-import django.contrib.auth.password_validation as validators
+from .generic import DynamicFieldsModelSerializer
log = logging.getLogger(__name__)
user_factory = GradyUserFactory()
@@ -21,22 +22,13 @@ class TutorSerializer(DynamicFieldsModelSerializer):
required=False
)
- @staticmethod
- def _get_completed_assignments(obj):
- return models.TutorSubmissionAssignment.objects.filter(
- is_done=True,
- subscription__owner=obj,
- )
-
- def get_feedback_created(self, obj):
- return self._get_completed_assignments(obj).filter(
- subscription__feedback_stage=models.SubmissionSubscription.FEEDBACK_CREATION # noqa
- ).count()
+ def get_feedback_created(self, t):
+ ''' It is required that this field was previously annotated '''
+ return t.feedback_created if hasattr(t, 'feedback_created') else 0
- def get_feedback_validated(self, obj):
- return self._get_completed_assignments(obj).filter(
- subscription__feedback_stage=models.SubmissionSubscription.FEEDBACK_VALIDATION # noqa
- ).count()
+ def get_feedback_validated(self, t):
+ ''' It is required that this field was previously annotated '''
+ return t.feedback_validated if hasattr(t, 'feedback_validated') else 0
def create(self, validated_data) -> models.UserAccount:
log.info("Crating tutor from data %s", validated_data)
diff --git a/core/tests/test_custom_subscription_filter.py b/core/tests/test_custom_subscription_filter.py
index 0731815c..4f111626 100644
--- a/core/tests/test_custom_subscription_filter.py
+++ b/core/tests/test_custom_subscription_filter.py
@@ -1,10 +1,9 @@
+from django.conf import settings
from rest_framework.test import APITestCase
+from core.models import Feedback, SubmissionSubscription
from util.factories import make_test_data
-from core.models import SubmissionSubscription, Feedback
-from django.conf import settings
-
class StopAfterFiftyPercent(APITestCase):
diff --git a/core/tests/test_tutor_api_endpoints.py b/core/tests/test_tutor_api_endpoints.py
index 72e399cc..95728f80 100644
--- a/core/tests/test_tutor_api_endpoints.py
+++ b/core/tests/test_tutor_api_endpoints.py
@@ -4,17 +4,17 @@
* POST /tutor/:username/:email create a new tutor and email password
* GET /tutorlist list of all tutors with their scores
"""
+from django.conf import settings
from django.contrib.auth import get_user_model
from rest_framework import status
from rest_framework.reverse import reverse
from rest_framework.test import (APIClient, APIRequestFactory, APITestCase,
force_authenticate)
-from core.models import SubmissionSubscription, TutorSubmissionAssignment
+from core.models import (Feedback, SubmissionSubscription,
+ TutorSubmissionAssignment)
from core.views import TutorApiViewSet
-from util.factories import GradyUserFactory
-
-from django.conf import settings
+from util.factories import GradyUserFactory, make_test_data
NUMBER_OF_TUTORS = 3
@@ -49,24 +49,68 @@ class TutorListTests(APITestCase):
@classmethod
def setUpTestData(cls):
- cls.factory = APIRequestFactory()
- cls.user_factory = GradyUserFactory()
+ factory = APIRequestFactory()
+
+ request = factory.get(reverse('tutor-list'))
+ view = TutorApiViewSet.as_view({'get': 'list'})
+
+ data = make_test_data(data_dict={
+ 'submission_types': [{
+ 'name': '01. Sort this or that',
+ 'full_score': 35,
+ 'description': 'Very complicated',
+ 'solution': 'Trivial!'}],
+ 'students': [
+ {'username': 'student01'},
+ {'username': 'student02'}
+ ],
+ 'tutors': [
+ {'username': 'tutor01'},
+ {'username': 'tutor02'}
+ ],
+ 'reviewers': [
+ {'username': 'reviewer'}
+ ],
+ 'submissions': [
+ {
+ 'text': 'function blabl\n'
+ ' on multi lines\n',
+ 'type': '01. Sort this or that',
+ 'user': 'student01'
+ },
+ {
+ 'text': 'function blabl\n'
+ ' on multi lines\n',
+ 'type': '01. Sort this or that',
+ 'user': 'student02'
+ }
+ ]}
+ )
- def setUp(self):
- self.tutor_list = [self.user_factory.make_tutor()
- for _ in range(NUMBER_OF_TUTORS)]
- self.reviewer = self.user_factory.make_reviewer()
- self.request = self.factory.get(reverse('tutor-list'))
- self.view = TutorApiViewSet.as_view({'get': 'list'})
+ def feedback_cycle(tutor, stage):
+ subscription = SubmissionSubscription.objects.create(
+ owner=tutor,
+ feedback_stage=stage)
+ assignment = subscription.get_or_create_work_assignment()
+ Feedback.objects.update_or_create(
+ of_submission=assignment.submission,
+ score=35)
- force_authenticate(self.request, user=self.reviewer)
- self.response = self.view(self.request)
+ tutor01 = data['tutors'][0]
+ tutor02 = data['tutors'][1]
+ reviewer = data['reviewers'][0]
+
+ feedback_cycle(tutor01, SubmissionSubscription.FEEDBACK_CREATION)
+ feedback_cycle(tutor02, SubmissionSubscription.FEEDBACK_VALIDATION)
+
+ force_authenticate(request, user=reviewer)
+ cls.response = view(request)
def test_can_access(self):
self.assertEqual(self.response.status_code, status.HTTP_200_OK)
def test_get_a_list_of_all_tutors(self):
- self.assertEqual(len(self.response.data), NUMBER_OF_TUTORS)
+ self.assertEqual(2, len(self.response.data))
def test_feedback_created_count_matches_database(self):
def verify_fields(tutor_obj):
@@ -88,7 +132,7 @@ class TutorListTests(APITestCase):
subscription__feedback_stage=SubmissionSubscription.FEEDBACK_VALIDATION, # noqa
subscription__owner=t
).count()
- return feedback_validated_cnt == tutor_obj['feedback_created']
+ return feedback_validated_cnt == tutor_obj['feedback_validated']
self.assertTrue(all(map(verify_fields, self.response.data)))
diff --git a/core/views/common_views.py b/core/views/common_views.py
index 0b4c8432..caa5ca23 100644
--- a/core/views/common_views.py
+++ b/core/views/common_views.py
@@ -5,11 +5,11 @@ import logging
from django.conf import settings
from django.db.models import Avg
-from rest_framework import generics, mixins, viewsets, status
+from rest_framework import generics, mixins, status, viewsets
from rest_framework.decorators import api_view, list_route
+from rest_framework.exceptions import PermissionDenied
from rest_framework.permissions import AllowAny
from rest_framework.response import Response
-from rest_framework.exceptions import PermissionDenied
from core import models
from core.models import ExamType, StudentInfo, SubmissionType
@@ -98,7 +98,10 @@ class TutorApiViewSet(
viewsets.GenericViewSet):
""" Api endpoint for creating, listing, viewing or deleteing tutors """
permission_classes = (IsReviewer,)
- queryset = models.UserAccount.objects.filter(role='Tutor')
+ queryset = models.UserAccount.tutors \
+ .with_feedback_count() \
+ .prefetch_related('subscriptions') \
+ .prefetch_related('subscriptions__assignments')
serializer_class = TutorSerializer
@list_route(methods=['post'], permission_classes=[AllowAny])
@@ -140,11 +143,18 @@ class StatisticsEndpoint(viewsets.ViewSet):
class SubmissionViewSet(viewsets.ReadOnlyModelViewSet):
permission_classes = (IsTutorOrReviewer, )
serializer_class = SubmissionNoTypeSerializer
+ queryset = models.Submission.objects\
+ .select_related('type')\
+ .select_related('feedback')\
+ .prefetch_related('tests')\
+ .prefetch_related('feedback__feedback_lines')\
+ .prefetch_related('feedback__feedback_lines__of_tutor')\
+ .all()
def get_queryset(self):
if self.request.user.is_reviewer():
- return models.Submission.objects.all()
- else:
- return models.Submission.objects.filter(
- assignments__subscription__owner=self.request.user
- )
+ return self.queryset
+
+ return self.queryset.filter(
+ assignments__subscription__owner=self.request.user
+ )
diff --git a/core/views/export.py b/core/views/export.py
index 81b0f53a..7628c7fe 100644
--- a/core/views/export.py
+++ b/core/views/export.py
@@ -1,9 +1,9 @@
-from core.models import StudentInfo, SubmissionType
-from core.permissions import IsReviewer
-
+from rest_framework.response import Response
from rest_framework.views import APIView
from rest_framework_csv import renderers
-from rest_framework.response import Response
+
+from core.models import StudentInfo, SubmissionType
+from core.permissions import IsReviewer
class SemicolonCSVRenderer(renderers.CSVRenderer):
diff --git a/core/views/feedback.py b/core/views/feedback.py
index 6735d167..0f681bdb 100644
--- a/core/views/feedback.py
+++ b/core/views/feedback.py
@@ -19,6 +19,8 @@ class FeedbackApiView(
queryset = models.Feedback.objects\
.select_related('of_submission')\
.select_related('of_submission__type')\
+ .select_related('of_submission__student')\
+ .select_related('of_submission__student__user')\
.all()
serializer_class = serializers.FeedbackSerializer
lookup_field = 'of_submission__pk'
@@ -72,12 +74,14 @@ class FeedbackApiView(
def get_queryset(self):
if self.request.user.is_reviewer():
- return self.queryset.all()
- else:
- # only return feedback that the requesting user has worked on
- return self.queryset.filter(
- of_submission__assignments__subscription__owner=self.request.user # noqa
- )
+ return self.queryset \
+ .prefetch_related('feedback_lines') \
+ .prefetch_related('feedback_lines__of_tutor') \
+ .all()
+
+ return self.queryset.filter(
+ of_submission__assignments__subscription__owner=self.request.user
+ )
def create(self, request, *args, **kwargs):
serializer = self.get_serializer(data=request.data)
--
GitLab