Merge pull request #20 from edx/will/upgrade-django-rest-framework

Upgrade djangorestframework to v3.1

requirements.txt

@@ -2,6 +2,7 @@ dogapi==1.2.1
 django>=1.4,<1.5
 django-extensions==1.5.5
 django-model-utils==2.3.1
-djangorestframework<2.4
+djangorestframework>=3.1,<3.2
+jsonfield==1.0.3
 pytz==2015.2
-South==0.7.6
+South==1.0.1

settings.py

@@ -46,6 +46,7 @@ INSTALLED_APPS = (
 
     # Third party
     'django_extensions',
+    'south',
 
     # Test
     'django_nose',

setup.py

@@ -33,7 +33,7 @@ def load_requirements(*requirements_paths):
 
 setup(
     name='edx-submissions',
-    version='0.0.8',
+    version='0.1.0',
     author='edX',
     description='An API for creating submissions and scores.',
     url='http://github.com/edx/edx-submissions.git',

submissions/admin.py

@@ -56,7 +56,7 @@ class SubmissionAdmin(admin.ModelAdmin, StudentItemAdminMixin):
         'student_item_id',
         'course_id', 'item_id', 'student_id',
         'attempt_number', 'submitted_at', 'created_at',
-        'raw_answer', 'all_scores',
+        'answer', 'all_scores',
     )
     search_fields = ('id', 'uuid') + StudentItemAdminMixin.search_fields
 

submissions/api.py

@@ -14,7 +14,7 @@ from django.db import IntegrityError, DatabaseError
 from dogapi import dog_stats_api
 
 from submissions.serializers import (
-    SubmissionSerializer, StudentItemSerializer, ScoreSerializer, JsonFieldError
+    SubmissionSerializer, StudentItemSerializer, ScoreSerializer
 )
 from submissions.models import Submission, StudentItem, Score, ScoreSummary, score_set, score_reset
 
@@ -177,11 +177,6 @@ def create_submission(student_item_dict, answer, submitted_at=None, attempt_numb
 
         return sub_data
 
-    except JsonFieldError:
-        error_message = u"Could not serialize JSON field in submission {} for student item {}".format(
-            model_kwargs, student_item_dict
-        )
-        raise SubmissionRequestError(msg=error_message)
     except DatabaseError:
         error_message = u"An error occurred while creating submission {} for student item: {}".format(
             model_kwargs,

submissions/migrations/0005_raw_answer_to_jsonfield.py

+# -*- coding: utf-8 -*-
+from south.utils import datetime_utils as datetime
+from south.db import db
+from south.v2 import SchemaMigration
+from django.db import models
+
+
+class Migration(SchemaMigration):
+
+    def forwards(self, orm):
+        # For the upgrade to Django Rest Framework v3, we replaced the `raw_answer` TextField
+        # with an `answer` JsonField that maps to the `raw_answer` DB column.
+        # This doesn't require any changes to the database schema, but we generate a migration
+        # anyway so that South knows about the change.
+        pass
+
+    def backwards(self, orm):
+        pass
+
+    models = {
+        'submissions.score': {
+            'Meta': {'object_name': 'Score'},
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'points_earned': ('django.db.models.fields.PositiveIntegerField', [], {'default': '0'}),
+            'points_possible': ('django.db.models.fields.PositiveIntegerField', [], {'default': '0'}),
+            'reset': ('django.db.models.fields.BooleanField', [], {'default': 'False'}),
+            'student_item': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.StudentItem']"}),
+            'submission': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.Submission']", 'null': 'True'})
+        },
+        'submissions.scoresummary': {
+            'Meta': {'object_name': 'ScoreSummary'},
+            'highest': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'+'", 'to': "orm['submissions.Score']"}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'latest': ('django.db.models.fields.related.ForeignKey', [], {'related_name': "'+'", 'to': "orm['submissions.Score']"}),
+            'student_item': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.StudentItem']", 'unique': 'True'})
+        },
+        'submissions.studentitem': {
+            'Meta': {'unique_together': "(('course_id', 'student_id', 'item_id'),)", 'object_name': 'StudentItem'},
+            'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'item_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),
+            'item_type': ('django.db.models.fields.CharField', [], {'max_length': '100'}),
+            'student_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'})
+        },
+        'submissions.submission': {
+            'Meta': {'ordering': "['-submitted_at', '-id']", 'object_name': 'Submission'},
+            'answer': ('jsonfield.fields.JSONField', [], {'db_column': "'raw_answer'", 'blank': 'True'}),
+            'attempt_number': ('django.db.models.fields.PositiveIntegerField', [], {}),
+            'created_at': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'db_index': 'True'}),
+            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
+            'student_item': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['submissions.StudentItem']"}),
+            'submitted_at': ('django.db.models.fields.DateTimeField', [], {'default': 'datetime.datetime.now', 'db_index': 'True'}),
+            'uuid': ('django.db.models.fields.CharField', [], {'db_index': 'True', 'max_length': '36', 'blank': 'True'})
+        }
+    }
+
+    complete_apps = ['submissions']

submissions/models.py

@@ -9,7 +9,6 @@ need to then generate a matching migration for it using:
     ./manage.py schemamigration submissions --auto
 
 """
-import json
 import logging
 
 from django.db import models, DatabaseError
@@ -17,6 +16,7 @@ from django.db.models.signals import post_save
 from django.dispatch import receiver, Signal
 from django.utils.timezone import now
 from django_extensions.db.fields import UUIDField
+from jsonfield import JSONField
 
 
 logger = logging.getLogger(__name__)
@@ -25,14 +25,14 @@ logger = logging.getLogger(__name__)
 score_set = Signal(providing_args=[
     'points_possible', 'points_earned', 'anonymous_user_id',
     'course_id', 'item_id'
-    ]
-)
+])
 
 # Signal to inform listeners that a score has been reset
 score_reset = Signal(
     providing_args=['anonymous_user_id', 'course_id', 'item_id']
 )
 
+
 class StudentItem(models.Model):
     """Represents a single item for a single course for a single user.
 
@@ -99,7 +99,12 @@ class Submission(models.Model):
     created_at = models.DateTimeField(editable=False, default=now, db_index=True)
 
     # The answer (JSON-serialized)
-    raw_answer = models.TextField(blank=True)
+    # NOTE: previously, this field was a TextField named `raw_answer`.
+    # Since JSONField is a subclass of TextField, we can use it as a drop-in
+    # replacement for TextField that performs JSON serialization/deserialization.
+    # For backwards compatibility, we override the default database column
+    # name so it continues to use `raw_answer`.
+    answer = JSONField(blank=True, db_column="raw_answer")
 
     def __repr__(self):
         return repr(dict(
@@ -108,7 +113,7 @@ class Submission(models.Model):
             attempt_number=self.attempt_number,
             submitted_at=self.submitted_at,
             created_at=self.created_at,
-            raw_answer=self.raw_answer,
+            answer=self.answer,
         ))
 
     def __unicode__(self):

submissions/serializers.py

 """
 Serializers are created to ensure models do not have to be accessed outside the
-scope of the Tim APIs.
+scope of the submissions API.
 """
 import json
+
 from rest_framework import serializers
+from rest_framework.fields import Field, DateTimeField, IntegerField
 from submissions.models import StudentItem, Submission, Score
 
 
-class JsonFieldError(Exception):
-    """
-    An error occurred while serializing/deserializing JSON.
-    """
-    pass
-
-
-class JsonField(serializers.WritableField):
-    """
-    JSON-serializable field.
+class RawField(Field):
     """
-    def to_native(self, obj):
-        """
-        Deserialize the JSON string.
+    Serializer field that does NOT modify its value.
 
-        Args:
-            obj (str): The JSON string stored in the database.
+    This is useful when the Django model field is handling serialization/deserialization.
+    For example, `JsonField` already converts its value to JSON internally.  If we use
+    the default DRF text field, the value would be converted to a string, which would then
+    be encoded as JSON:
 
-        Returns:
-            JSON-serializable
+    1) field value is {"foo": "bar"} (a dict)
+    2) DRF's default field implementation converts the dict to a string: "{'foo': 'bar'}"
+    3) JsonField encodes the string as JSON: '"{\'foo\': \'bar\'}"'
 
-        Raises:
-            JsonFieldError: The field could not be deserialized.
-        """
-        try:
-            return json.loads(obj)
-        except (TypeError, ValueError):
-            raise JsonFieldError(u"Could not deserialize as JSON: {}".format(obj))
+    This is a problem, because when we load the data back from the database, we'll end
+    up with a string instead of a dictionary!
 
-    def from_native(self, data):
     """
-        Serialize an object to JSON.
+    def to_representation(self, obj):
+        return obj
 
-        Args:
-            data (JSON-serializable): The data to serialize.
-
-        Returns:
-            str
-
-        Raises:
-            ValueError: The data could not be serialized as JSON.
-        """
-        try:
-            return json.dumps(data)
-        except (TypeError, ValueError):
-            raise JsonFieldError(u"Could not serialize as JSON: {}".format(data))
+    def to_internal_value(self, data):
+        return data
 
 
 class StudentItemSerializer(serializers.ModelSerializer):
@@ -63,14 +41,37 @@ class StudentItemSerializer(serializers.ModelSerializer):
 
 class SubmissionSerializer(serializers.ModelSerializer):
 
-    answer = JsonField(source='raw_answer')
+    # Django Rest Framework v3 uses the Django setting `DATETIME_FORMAT`
+    # when serializing datetimes.  This differs from v2, which always
+    # returned a datetime.  To preserve the old behavior, we explicitly
+    # set `format` to None.
+    # http://www.django-rest-framework.org/api-guide/fields/#datetimefield
+    submitted_at = DateTimeField(format=None, required=False)
+    created_at = DateTimeField(format=None, required=False)
+
+    # Django Rest Framework v3 apparently no longer validates that
+    # `PositiveIntegerField`s are positive!
+    attempt_number = IntegerField(min_value=0)
+
+    # Prevent Django Rest Framework from converting the answer (dict or str)
+    # to a string.
+    answer = RawField()
 
-    def validate_answer(self, attrs, source):
-        """Check that the answer is within an acceptable size range."""
-        value = attrs[source]
-        if len(value) > Submission.MAXSIZE:
+    def validate_answer(self, value):
+        """
+        Check that the answer is JSON-serializable and not too long.
+        """
+        # Check that the answer is JSON-serializable
+        try:
+            serialized = json.dumps(value)
+        except (ValueError, TypeError):
+            raise serializers.ValidationError("Answer value must be JSON-serializable")
+
+        # Check the length of the serialized representation
+        if len(serialized) > Submission.MAXSIZE:
             raise serializers.ValidationError("Maximum answer size exceeded.")
-        return attrs
+
+        return value
 
     class Meta:
         model = Submission
@@ -80,15 +81,14 @@ class SubmissionSerializer(serializers.ModelSerializer):
             'attempt_number',
             'submitted_at',
             'created_at',
-
-            # Computed
             'answer',
         )
 
 
 class ScoreSerializer(serializers.ModelSerializer):
 
-    submission_uuid = serializers.Field(source='submission_uuid')
+    # Ensure that the created_at datetime is not converted to a string.
+    created_at = DateTimeField(format=None, required=False)
 
     class Meta:
         model = Score
@@ -102,4 +102,3 @@ class ScoreSerializer(serializers.ModelSerializer):
             # Computed
             'submission_uuid',
         )
-

submissions/tests/test_api.py

 import datetime
 import copy
 
-from ddt import ddt, file_data
-from django.db import DatabaseError
-from django.dispatch import Signal
+import ddt
+from django.db import DatabaseError, connection, transaction
 from django.core.cache import cache
 from django.test import TestCase
 from nose.tools import raises
@@ -11,7 +10,7 @@ from mock import patch
 import pytz
 
 from submissions import api as api
-from submissions.models import ScoreSummary, Submission, StudentItem, Score, score_set
+from submissions.models import ScoreSummary, Submission, StudentItem, score_set
 from submissions.serializers import StudentItemSerializer
 
 STUDENT_ITEM = dict(
@@ -32,8 +31,11 @@ ANSWER_ONE = u"this is my answer!"
 ANSWER_TWO = u"this is my other answer!"
 ANSWER_THREE = u'' + 'c' * (Submission.MAXSIZE + 1)
 
+# Test a non-string JSON-serializable answer
+ANSWER_DICT = {"text": "foobar"}
 
-@ddt
+
+@ddt.ddt
 class TestSubmissionsApi(TestCase):
     """
     Testing Submissions
@@ -45,10 +47,11 @@ class TestSubmissionsApi(TestCase):
         """
         cache.clear()
 
-    def test_create_submission(self):
-        submission = api.create_submission(STUDENT_ITEM, ANSWER_ONE)
+    @ddt.data(ANSWER_ONE, ANSWER_DICT)
+    def test_create_submission(self, answer):
+        submission = api.create_submission(STUDENT_ITEM, answer)
         student_item = self._get_student_item(STUDENT_ITEM)
-        self._assert_submission(submission, ANSWER_ONE, student_item.pk, 1)
+        self._assert_submission(submission, answer, student_item.pk, 1)
 
     def test_create_huge_submission_fails(self):
         with self.assertRaises(api.SubmissionRequestError):
@@ -121,7 +124,6 @@ class TestSubmissionsApi(TestCase):
         mock_get.side_effect = DatabaseError("Kaboom!")
         api.get_submission("000000000000000")
 
-
     def test_two_students(self):
         api.create_submission(STUDENT_ITEM, ANSWER_ONE)
         api.create_submission(SECOND_STUDENT_ITEM, ANSWER_TWO)
@@ -136,8 +138,7 @@ class TestSubmissionsApi(TestCase):
         student_item = self._get_student_item(SECOND_STUDENT_ITEM)
         self._assert_submission(submissions[0], ANSWER_TWO, student_item.pk, 1)
 
-
-    @file_data('data/valid_student_items.json')
+    @ddt.file_data('data/valid_student_items.json')
     def test_various_student_items(self, valid_student_item):
         api.create_submission(valid_student_item, ANSWER_ONE)
         student_item = self._get_student_item(valid_student_item)
@@ -164,12 +165,13 @@ class TestSubmissionsApi(TestCase):
         self._assert_submission(submissions[0], ANSWER_ONE, student_item.pk, 2)
 
     @raises(api.SubmissionRequestError)
-    @file_data('data/bad_student_items.json')
+    @ddt.file_data('data/bad_student_items.json')
     def test_error_checking(self, bad_student_item):
         api.create_submission(bad_student_item, -100)
 
     @raises(api.SubmissionRequestError)
     def test_error_checking_submissions(self):
+        # Attempt number should be >= 0
         api.create_submission(STUDENT_ITEM, ANSWER_ONE, None, -1)
 
     @patch.object(Submission.objects, 'filter')
@@ -183,12 +185,14 @@ class TestSubmissionsApi(TestCase):
             api.create_submission(STUDENT_ITEM, datetime.datetime.now())
 
     def test_load_non_json_answer(self):
-        # This should never happen, if folks are using the public API.
-        # Create a submission with a raw answer that is NOT valid JSON
         submission = api.create_submission(STUDENT_ITEM, ANSWER_ONE)
         sub_model = Submission.objects.get(uuid=submission['uuid'])
-        sub_model.raw_answer = ''
-        sub_model.save()
+
+        # This should never happen, if folks are using the public API.
+        # Create a submission with a raw answer that is NOT valid JSON
+        query = "UPDATE submissions_submission SET raw_answer = '}' WHERE id = %s"
+        connection.cursor().execute(query, [str(sub_model.id)])
+        transaction.commit_unless_managed()
 
         with self.assertRaises(api.SubmissionInternalError):
             api.get_submission(sub_model.uuid)