fix(controller): properly serialize JSONField objects

JSONField is a finnicky thing. When serializing the object in the API,
it will just dump the object as a string. This ends in an invalid JSON
response. In the case of the Config object:

    {"owner": "bacongobbler", "app": "go", "values": "{\"HELLO\": \"world\"}", "memory": "{}", "cpu": "{}", "tags": "{}", ...}

This fix changes the JSON output to the following:

    {"owner": "bacongobbler", "app": "go", "values": {"HELLO": "world"}, "memory": {}, "cpu": {}, "tags": {}}

This is completely backwards compatible with previous requests. The
JSONSerializer will accept both strings and JSON objects in the request.

Author: Matthew Fisher

api/serializers.py

@@ -4,6 +4,7 @@
 
 from __future__ import unicode_literals
 
+import json
 import re
 
 from django.conf import settings
@@ -32,6 +33,18 @@ def from_native(self, data):
         return serializers.SlugRelatedField.from_native(self, data)
 
 
+class JSONFieldSerializer(serializers.WritableField):
+    def to_native(self, obj):
+        return obj
+
+    def from_native(self, value):
+        try:
+            val = json.loads(value)
+        except TypeError:
+            val = value
+        return val
+
+
 class UserSerializer(serializers.ModelSerializer):
     """Serialize a User model."""
 
@@ -64,6 +77,7 @@ class ClusterSerializer(serializers.ModelSerializer):
     """Serialize a :class:`~api.models.Cluster` model."""
 
     owner = serializers.Field(source='owner.username')
+    options = JSONFieldSerializer(source='options', required=False)
 
     class Meta:
         """Metadata options for a :class:`ClusterSerializer`."""
@@ -98,6 +112,7 @@ class BuildSerializer(serializers.ModelSerializer):
 
     owner = serializers.Field(source='owner.username')
     app = serializers.SlugRelatedField(slug_field='id')
+    procfile = JSONFieldSerializer(source='procfile', required=False)
 
     class Meta:
         """Metadata options for a :class:`BuildSerializer`."""
@@ -110,14 +125,10 @@ class ConfigSerializer(serializers.ModelSerializer):
 
     owner = serializers.Field(source='owner.username')
     app = serializers.SlugRelatedField(slug_field='id')
-    values = serializers.ModelField(
-        model_field=models.Config()._meta.get_field('values'), required=False)
-    memory = serializers.ModelField(
-        model_field=models.Config()._meta.get_field('memory'), required=False)
-    cpu = serializers.ModelField(
-        model_field=models.Config()._meta.get_field('cpu'), required=False)
-    tags = serializers.ModelField(
-        model_field=models.Config()._meta.get_field('tags'), required=False)
+    values = JSONFieldSerializer(source='values', required=False)
+    memory = JSONFieldSerializer(source='memory', required=False)
+    cpu = JSONFieldSerializer(source='cpu', required=False)
+    tags = JSONFieldSerializer(source='tags', required=False)
 
     class Meta:
         """Metadata options for a :class:`ConfigSerializer`."""
@@ -185,6 +196,7 @@ class AppSerializer(serializers.ModelSerializer):
     id = serializers.SlugField(default=utils.generate_app_name)
     cluster = serializers.SlugRelatedField(slug_field='id')
     url = serializers.Field(source='url')
+    structure = JSONFieldSerializer(source='structure', required=False)
 
     class Meta:
         """Metadata options for a :class:`AppSerializer`."""
@@ -203,14 +215,6 @@ def validate_id(self, attrs, source):
             raise serializers.ValidationError("App IDs cannot be 'deis'")
         return attrs
 
-    def validate_structure(self, attrs, source):
-        """
-        Check that the structure JSON dict has non-negative ints as its values.
-        """
-        value = attrs[source]
-        models.validate_app_structure(value)
-        return attrs
-
 
 class ContainerSerializer(serializers.ModelSerializer):
     """Serialize a :class:`~api.models.Container` model."""

api/tests/test_app.py

@@ -10,12 +10,12 @@
 import os.path
 
 from django.test import TestCase
-
 from django.conf import settings
 
+from api.models import App
 
-class AppTest(TestCase):
 
+class AppTest(TestCase):
     """Tests creation of applications"""
 
     fixtures = ['tests.json']
@@ -135,6 +135,23 @@ def test_app_errors(self):
             response = self.client.get(url)
             self.assertEquals(response.status_code, 404)
 
+    def test_app_structure_is_valid_json(self):
+        """Application structures should be valid JSON objects."""
+        url = '/api/apps'
+        body = {'cluster': 'autotest'}
+        response = self.client.post(url, json.dumps(body), content_type='application/json')
+        self.assertEqual(response.status_code, 201)
+        app_id = response.data['id']
+        self.assertIn('structure', response.data)
+        self.assertEqual(response.data['structure'], {})
+        app = App.objects.get(id=app_id)
+        app.structure = {'web': 1}
+        app.save()
+        url = '/api/apps/{}'.format(app_id)
+        response = self.client.get(url)
+        self.assertIn('structure', response.data)
+        self.assertEqual(response.data['structure'], {"web": 1})
+
     def test_admin_can_manage_other_apps(self):
         """Administrators of Deis should be able to manage all applications.
         """

api/tests/test_cluster.py

@@ -42,7 +42,7 @@ def test_cluster(self):
         self.assertIn('auth', response.data)
         self.assertIn('options', response.data)
         self.assertEqual(response.data['hosts'], 'host1,host2')
-        self.assertEqual(json.loads(response.data['options']), {'key': 'val'})
+        self.assertEqual(response.data['options'], {'key': 'val'})
         response = self.client.get('/api/clusters')
         self.assertEqual(response.status_code, 200)
         self.assertEqual(len(response.data['results']), 1)
@@ -68,7 +68,7 @@ def test_cluster(self):
         response = self.client.patch(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertEqual(response.data['hosts'], new_hosts)
-        self.assertEqual(json.loads(response.data['options']), new_options)
+        self.assertEqual(response.data['options'], new_options)
         response = self.client.delete(url)
         self.assertEqual(response.status_code, 204)
 

api/tests/test_config.py

@@ -54,7 +54,7 @@ def test_config(self):
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
         self.assertIn('values', response.data)
-        self.assertEqual(response.data['values'], json.dumps({}))
+        self.assertEqual(response.data['values'], {})
         config1 = response.data
         # set an initial config value
         body = {'values': json.dumps({'NEW_URL1': 'http://localhost:8080/'})}
@@ -63,28 +63,28 @@ def test_config(self):
         self.assertIn('x-deis-release', response._headers)
         config2 = response.data
         self.assertNotEqual(config1['uuid'], config2['uuid'])
-        self.assertIn('NEW_URL1', json.loads(response.data['values']))
+        self.assertIn('NEW_URL1', response.data['values'])
         # read the config
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
         config3 = response.data
         self.assertEqual(config2, config3)
-        self.assertIn('NEW_URL1', json.loads(response.data['values']))
+        self.assertIn('NEW_URL1', response.data['values'])
         # set an additional config value
         body = {'values': json.dumps({'NEW_URL2': 'http://localhost:8080/'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
         config3 = response.data
         self.assertNotEqual(config2['uuid'], config3['uuid'])
-        self.assertIn('NEW_URL1', json.loads(response.data['values']))
-        self.assertIn('NEW_URL2', json.loads(response.data['values']))
+        self.assertIn('NEW_URL1', response.data['values'])
+        self.assertIn('NEW_URL2', response.data['values'])
         # read the config again
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
         config4 = response.data
         self.assertEqual(config3, config4)
-        self.assertIn('NEW_URL1', json.loads(response.data['values']))
-        self.assertIn('NEW_URL2', json.loads(response.data['values']))
+        self.assertIn('NEW_URL1', response.data['values'])
+        self.assertIn('NEW_URL2', response.data['values'])
         # unset a config value
         body = {'values': json.dumps({'NEW_URL2': None})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
@@ -118,13 +118,13 @@ def test_config_set_same_key(self):
         body = {'values': json.dumps({'PORT': '5000'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
-        self.assertIn('PORT', json.loads(response.data['values']))
+        self.assertIn('PORT', response.data['values'])
         # reset same config value
         body = {'values': json.dumps({'PORT': '5001'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
-        self.assertIn('PORT', json.loads(response.data['values']))
-        self.assertEqual(json.loads(response.data['values'])['PORT'], '5001')
+        self.assertIn('PORT', response.data['values'])
+        self.assertEqual(response.data['values']['PORT'], '5001')
 
     @mock.patch('requests.post', mock_import_repository_task)
     def test_config_set_unicode(self):
@@ -141,19 +141,19 @@ def test_config_set_unicode(self):
         body = {'values': json.dumps({'POWERED_BY': 'Деис'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
-        self.assertIn('POWERED_BY', json.loads(response.data['values']))
+        self.assertIn('POWERED_BY', response.data['values'])
         # reset same config value
         body = {'values': json.dumps({'POWERED_BY': 'Кроликов'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
-        self.assertIn('POWERED_BY', json.loads(response.data['values']))
-        self.assertEqual(json.loads(response.data['values'])['POWERED_BY'], 'Кроликов')
+        self.assertIn('POWERED_BY', response.data['values'])
+        self.assertEqual(response.data['values']['POWERED_BY'], 'Кроликов')
         # set an integer to test unicode regression
         body = {'values': json.dumps({'INTEGER': 1})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
-        self.assertIn('INTEGER', json.loads(response.data['values']))
-        self.assertEqual(json.loads(response.data['values'])['INTEGER'], 1)
+        self.assertIn('INTEGER', response.data['values'])
+        self.assertEqual(response.data['values']['INTEGER'], 1)
 
     @mock.patch('requests.post', mock_import_repository_task)
     def test_config_str(self):
@@ -179,7 +179,7 @@ def test_admin_can_create_config_on_other_apps(self):
         body = {'values': json.dumps({'PORT': '5000'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
-        self.assertIn('PORT', json.loads(response.data['values']))
+        self.assertIn('PORT', response.data['values'])
 
     @mock.patch('requests.post', mock_import_repository_task)
     def test_limit_memory(self):
@@ -197,7 +197,7 @@ def test_limit_memory(self):
         response = self.client.get(url, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertIn('memory', response.data)
-        self.assertEqual(json.loads(response.data['memory']), {})
+        self.assertEqual(response.data['memory'], {})
         # regression test for https://github.com/deis/deis/issues/1563
         self.assertNotIn('"', response.data['memory'])
         # set an initial limit
@@ -211,7 +211,7 @@ def test_limit_memory(self):
         response = self.client.get(url, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertIn('memory', response.data)
-        memory = json.loads(response.data['memory'])
+        memory = response.data['memory']
         self.assertIn('web', memory)
         self.assertEqual(memory['web'], '1G')
         # set an additional value
@@ -220,7 +220,7 @@ def test_limit_memory(self):
         self.assertEqual(response.status_code, 201)
         limit2 = response.data
         self.assertNotEqual(limit1['uuid'], limit2['uuid'])
-        memory = json.loads(response.data['memory'])
+        memory = response.data['memory']
         self.assertIn('worker', memory)
         self.assertEqual(memory['worker'], '512M')
         self.assertIn('web', memory)
@@ -230,7 +230,7 @@ def test_limit_memory(self):
         self.assertEqual(response.status_code, 200)
         limit3 = response.data
         self.assertEqual(limit2, limit3)
-        memory = json.loads(response.data['memory'])
+        memory = response.data['memory']
         self.assertIn('worker', memory)
         self.assertEqual(memory['worker'], '512M')
         self.assertIn('web', memory)
@@ -240,11 +240,11 @@ def test_limit_memory(self):
         body = {'values': json.dumps({'NEW_URL2': 'http://localhost:8080/'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
         self.assertEqual(response.status_code, 201)
-        self.assertIn('NEW_URL2', json.loads(response.data['values']))
+        self.assertIn('NEW_URL2', response.data['values'])
         # read the limit again
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200)
-        memory = json.loads(response.data['memory'])
+        memory = response.data['memory']
         self.assertIn('worker', memory)
         self.assertEqual(memory['worker'], '512M')
         self.assertIn('web', memory)
@@ -277,7 +277,7 @@ def test_limit_cpu(self):
         response = self.client.get(url, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertIn('cpu', response.data)
-        self.assertEqual(json.loads(response.data['cpu']), {})
+        self.assertEqual(response.data['cpu'], {})
         # regression test for https://github.com/deis/deis/issues/1563
         self.assertNotIn('"', response.data['cpu'])
         # set an initial limit
@@ -290,7 +290,7 @@ def test_limit_cpu(self):
         response = self.client.get(url, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertIn('cpu', response.data)
-        cpu = json.loads(response.data['cpu'])
+        cpu = response.data['cpu']
         self.assertIn('web', cpu)
         self.assertEqual(cpu['web'], '1024')
         # set an additional value
@@ -299,7 +299,7 @@ def test_limit_cpu(self):
         self.assertEqual(response.status_code, 201)
         limit2 = response.data
         self.assertNotEqual(limit1['uuid'], limit2['uuid'])
-        cpu = json.loads(response.data['cpu'])
+        cpu = response.data['cpu']
         self.assertIn('worker', cpu)
         self.assertEqual(cpu['worker'], '512')
         self.assertIn('web', cpu)
@@ -309,7 +309,7 @@ def test_limit_cpu(self):
         self.assertEqual(response.status_code, 200)
         limit3 = response.data
         self.assertEqual(limit2, limit3)
-        cpu = json.loads(response.data['cpu'])
+        cpu = response.data['cpu']
         self.assertIn('worker', cpu)
         self.assertEqual(cpu['worker'], '512')
         self.assertIn('web', cpu)
@@ -342,7 +342,7 @@ def test_tags(self):
         response = self.client.get(url, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertIn('tags', response.data)
-        self.assertEqual(json.loads(response.data['tags']), {})
+        self.assertEqual(response.data['tags'], {})
         # set some tags
         body = {'tags': json.dumps({'environ': 'dev'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')
@@ -353,7 +353,7 @@ def test_tags(self):
         response = self.client.get(url, content_type='application/json')
         self.assertEqual(response.status_code, 200)
         self.assertIn('tags', response.data)
-        tags = json.loads(response.data['tags'])
+        tags = response.data['tags']
         self.assertIn('environ', tags)
         self.assertEqual(tags['environ'], 'dev')
         # set an additional value
@@ -362,7 +362,7 @@ def test_tags(self):
         self.assertEqual(response.status_code, 201)
         tags2 = response.data
         self.assertNotEqual(tags1['uuid'], tags2['uuid'])
-        tags = json.loads(response.data['tags'])
+        tags = response.data['tags']
         self.assertIn('rack', tags)
         self.assertEqual(tags['rack'], '1')
         self.assertIn('environ', tags)
@@ -372,7 +372,7 @@ def test_tags(self):
         self.assertEqual(response.status_code, 200)
         tags3 = response.data
         self.assertEqual(tags2, tags3)
-        tags = json.loads(response.data['tags'])
+        tags = response.data['tags']
         self.assertIn('rack', tags)
         self.assertEqual(tags['rack'], '1')
         self.assertIn('environ', tags)

api/tests/test_hooks.py

@@ -163,7 +163,7 @@ def test_build_hook_procfile(self):
         self.assertIn('results', response.data)
         build = response.data['results'][0]
         self.assertEqual(build['sha'], SHA)
-        self.assertEqual(build['procfile'], json.dumps(PROCFILE))
+        self.assertEqual(build['procfile'], PROCFILE)
         # test listing/retrieving container info
         url = "/api/apps/{app_id}/containers/web".format(**locals())
         response = self.client.get(url)