Merge pull request #1616 from mboersma/1563-fix-empty-json

fix(controller): don't encode empty JSON objects or arrays as strings

Author: mboersma

controller/api/fields.py

@@ -8,6 +8,22 @@
 from django import forms
 from django.db import models
 
+import json_field
+
+
+class JSONField(json_field.JSONField):
+
+    """
+    A subclass of json_field.JSONField that fixes empty JSON object
+    encoding behavior.
+    """
+
+    def get_db_prep_value(self, value, *args, **kwargs):
+        # if it's one of these values, it's already encoded
+        if value in ['{}', '[]']:
+            return value
+        return super(JSONField, self).get_db_prep_value(value, *args, **kwargs)
+
 
 class UuidField(models.CharField):
     """A univerally unique ID field."""

controller/api/models.py

@@ -21,7 +21,6 @@
 from django.utils.encoding import python_2_unicode_compatible
 from django_fsm import FSMField, transition
 from django_fsm.signals import post_transition
-from json_field.fields import JSONField
 
 from api import fields, tasks
 from registry import publish_release
@@ -88,7 +87,7 @@ class Cluster(UuidAuditedModel):
     domain = models.CharField(max_length=128)
     hosts = models.CharField(max_length=256)
     auth = models.TextField()
-    options = JSONField(default='{}', blank=True)
+    options = fields.JSONField(default='{}', blank=True)
 
     def __str__(self):
         return self.id
@@ -123,7 +122,7 @@ class App(UuidAuditedModel):
     owner = models.ForeignKey(settings.AUTH_USER_MODEL)
     id = models.SlugField(max_length=64, unique=True)
     cluster = models.ForeignKey('Cluster')
-    structure = JSONField(default='{}', blank=True)
+    structure = fields.JSONField(default='{}', blank=True)
 
     class Meta:
         permissions = (('use_app', 'Can use app'),)
@@ -412,7 +411,7 @@ class Build(UuidAuditedModel):
 
     # optional fields populated by builder
     sha = models.CharField(max_length=40, blank=True)
-    procfile = JSONField(default='{}', blank=True)
+    procfile = fields.JSONField(default='{}', blank=True)
     dockerfile = models.TextField(blank=True)
 
     class Meta:
@@ -433,7 +432,7 @@ class Config(UuidAuditedModel):
 
     owner = models.ForeignKey(settings.AUTH_USER_MODEL)
     app = models.ForeignKey('App')
-    values = JSONField(default='{}', blank=True)
+    values = fields.JSONField(default='{}', blank=True)
     limit = models.ForeignKey('Limit', null=True)
 
     class Meta:
@@ -454,8 +453,8 @@ class Limit(UuidAuditedModel):
 
     owner = models.ForeignKey(settings.AUTH_USER_MODEL)
     app = models.ForeignKey('App')
-    memory = JSONField(default='{}', blank=True)
-    cpu = JSONField(default='{}', blank=True)
+    memory = fields.JSONField(default='{}', blank=True)
+    cpu = fields.JSONField(default='{}', blank=True)
 
     class Meta:
         get_latest_by = 'created'

controller/api/tests/test_limit.py

@@ -55,7 +55,9 @@ 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']), json.dumps({}))
+        self.assertEqual(json.loads(response.data['memory']), {})
+        # regression test for https://github.com/deis/deis/issues/1563
+        self.assertNotIn('"', response.data['memory'])
         # set an initial limit
         mem = {'web': '1G'}
         body = {'memory': json.dumps(mem)}
@@ -119,7 +121,9 @@ 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['memory']), json.dumps({}))
+        self.assertEqual(json.loads(response.data['cpu']), {})
+        # regression test for https://github.com/deis/deis/issues/1563
+        self.assertNotIn('"', response.data['cpu'])
         # set an initial limit
         body = {'cpu': json.dumps({'web': '1024'})}
         response = self.client.post(url, json.dumps(body), content_type='application/json')

tests/limits_test.go

@@ -26,6 +26,9 @@ var (
 
 func limitsSetTest(t *testing.T, cfg *utils.DeisTestConfig, ver int) {
 	cpuCmd, memCmd := limitsSetCPUCmd, limitsSetMemCmd
+	// regression test for https://github.com/deis/deis/issues/1563
+	// previously the client would throw a stack trace with empty limits
+	utils.Execute(t, limitsListCmd, cfg, false, "Unlimited")
 	if strings.Contains(cfg.ExampleApp, "dockerfile") {
 		cpuCmd = strings.Replace(cpuCmd, "web", "cmd", 1)
 		memCmd = strings.Replace(memCmd, "web", "cmd", 1)
@@ -35,11 +38,13 @@ func limitsSetTest(t *testing.T, cfg *utils.DeisTestConfig, ver int) {
 	if _, err := regexp.MatchString(output1, out); err != nil {
 		t.Fatal(err)
 	}
+	utils.Execute(t, limitsListCmd, cfg, false, "512")
 	utils.Execute(t, memCmd, cfg, false, "256M")
 	out = dockerInspect(t, cfg, ver+1)
 	if _, err := regexp.MatchString(output2, out); err != nil {
 		t.Fatal(err)
 	}
+	utils.Execute(t, limitsListCmd, cfg, false, "256M")
 }
 
 func limitsUnsetTest(t *testing.T, cfg *utils.DeisTestConfig, ver int) {
@@ -53,11 +58,13 @@ func limitsUnsetTest(t *testing.T, cfg *utils.DeisTestConfig, ver int) {
 	if _, err := regexp.MatchString(output3, out); err != nil {
 		t.Fatal(err)
 	}
+	utils.Execute(t, limitsListCmd, cfg, false, "Unlimited")
 	utils.Execute(t, memCmd, cfg, false, "Unlimited")
 	out = dockerInspect(t, cfg, ver+1)
 	if _, err := regexp.MatchString(output4, out); err != nil {
 		t.Fatal(err)
 	}
+	utils.Execute(t, limitsListCmd, cfg, false, "Unlimited")
 }
 
 // dockerInspect creates an SSH session to the Deis controller