bug 1310188: Use custom column types for JSON columns (#184). r=nthom…

…as,jlorenzo

auslib/admin/views/base.py

@@ -104,6 +104,11 @@ def annotateRevisionDifferences(self, revisions):
                 # prep the value for being shown in revision_row.html
                 if value is None:
                     value = 'NULL'
+                elif isinstance(value, dict):
+                    try:
+                        value = json.dumps(value, indent=2, sort_keys=True)
+                    except ValueError:
+                        pass
                 elif isinstance(value, int):
                     value = unicode(str(value), 'utf8')
                 elif not isinstance(value, basestring):

auslib/admin/views/history.py

@@ -29,9 +29,8 @@ def get_value(self, type_, change_id, field):
         return revision[field]
 
     def format_value(self, value):
-        if isinstance(value, basestring):
+        if isinstance(value, dict):
             try:
-                value = json.loads(value)
                 value = json.dumps(value, indent=2, sort_keys=True)
             except ValueError:
                 pass
@@ -60,7 +59,7 @@ class DiffView(FieldView):
 
     def get_prev_id(self, value, change_id):
 
-        release_name = json.loads(value)['name']
+        release_name = value['name']
 
         table = dbo.releases.history
         old_revision = table.select(
@@ -77,17 +76,21 @@ def get_prev_id(self, value, change_id):
 
     def get(self, type_, change_id, field):
         value = self.get_value(type_, change_id, field)
+        data_version = self.get_value(type_, change_id, "data_version")
 
         prev_id = self.get_prev_id(value, change_id)
         previous = self.get_value(type_, prev_id, field)
+        prev_data_version = self.get_value(type_, prev_id, "data_version")
 
         value = self.format_value(value)
         previous = self.format_value(previous)
 
-        differ = difflib.Differ()
-        result = differ.compare(
+        result = difflib.unified_diff(
             previous.splitlines(),
-            value.splitlines()
+            value.splitlines(),
+            fromfile="Data Version {}".format(prev_data_version),
+            tofile="Data Version {}".format(data_version),
+            lineterm=""
         )
 
         return Response('\n'.join(result), content_type='text/plain')

auslib/admin/views/releases.py

@@ -17,7 +17,7 @@
 
 
 def createRelease(release, product, changed_by, transaction, releaseData):
-    blob = createBlob(json.dumps(releaseData))
+    blob = createBlob(releaseData)
     dbo.releases.insert(changed_by=changed_by, transaction=transaction, name=release,
                         product=product, data=blob)
     return dbo.releases.getReleases(name=release, transaction=transaction)[0]
@@ -220,7 +220,7 @@ def get(self, release):
             indent = 4
         else:
             indent = None
-        return Response(response=json.dumps(release[0]['data'], indent=indent), mimetype='application/json', headers=headers)
+        return Response(response=json.dumps(release[0]['data'], indent=indent, sort_keys=True), mimetype='application/json', headers=headers)
 
     @requirelogin
     def _put(self, release, changed_by, transaction):

auslib/db.py

@@ -83,6 +83,43 @@ class ChangeScheduledError(SQLAlchemyError):
     for data consistency reasons."""
 
 
+class JSONColumn(sqlalchemy.types.TypeDecorator):
+    """JSONColumns are used for types that are deserialized JSON (usually
+    dicts) in memory, but need to be serialized to text before storage.
+    JSONColumn handles the conversion both ways, serialized just before
+    storage, and deserialized just after retrieval."""
+
+    impl = Text
+
+    def process_bind_param(self, value, dialect):
+        if value:
+            value = json.dumps(value)
+        return value
+
+    def process_result_value(self, value, dialect):
+        if value:
+            value = json.loads(value)
+        return value
+
+
+def BlobColumn(impl=Text):
+    """BlobColumns are used to store Release Blobs, which are ultimately dicts.
+    Release Blobs must be serialized before storage, and deserialized upon
+    retrevial. This type handles both conversions. Some database engines
+    (eg: mysql) may require a different underlying type than Text. The
+    desired type may be passed in as an argument."""
+    class cls(sqlalchemy.types.TypeDecorator):
+
+        def process_bind_param(self, value, dialect):
+            return value.getJSON()
+
+        def process_result_value(self, value, dialect):
+            return createBlob(value)
+
+    cls.impl = impl
+    return cls
+
+
 class AUSTransaction(object):
     """Manages a single transaction. Requires a connection object.
 
@@ -1438,7 +1475,7 @@ def __init__(self, db, metadata, dialect):
             dataType = LONGTEXT
         else:
             dataType = Text
-        self.table.append_column(Column('data', dataType, nullable=False))
+        self.table.append_column(Column('data', BlobColumn(dataType), nullable=False))
         AUSTable.__init__(self, db, dialect)
 
     def setDomainWhitelist(self, domainWhitelist):
@@ -1515,7 +1552,7 @@ def getDataVersion():
         def getBlob():
             try:
                 row = self.select(where=[self.name == name], columns=[self.data], limit=1, transaction=transaction)[0]
-                blob = createBlob(row['data'])
+                blob = row['data']
                 return {"data_version": data_version, "blob": blob}
             except IndexError:
                 raise KeyError("Couldn't find release with name '%s'" % name)
@@ -1558,7 +1595,6 @@ def insert(self, changed_by, transaction=None, dryrun=False, **columns):
         blob.validate(columns["product"], self.domainWhitelist)
         if columns["name"] != blob["name"]:
             raise ValueError("name in database (%s) does not match name in blob (%s)" % (columns["name"], blob["name"]))
-        columns["data"] = blob.getJSON()
 
         ret = super(Releases, self).insert(changed_by=changed_by, transaction=transaction, dryrun=dryrun, **columns)
         if not dryrun:
@@ -1608,7 +1644,6 @@ def update(self, where, what, changed_by, old_data_version, transaction=None, dr
                 name = what.get("name", name)
                 if name != blob["name"]:
                     raise ValueError("name in database (%s) does not match name in blob (%s)" % (name, blob.get("name")))
-                what['data'] = blob.getJSON()
 
         for release in current_releases:
             name = current_release["name"]
@@ -1626,7 +1661,7 @@ def update(self, where, what, changed_by, old_data_version, transaction=None, dr
                     if ancestor_change is None:
                         self.log.debug("history for data_version %s for release %s absent" % (old_data_version, name))
                         raise
-                    ancestor_blob = createBlob(ancestor_change.get('data'))
+                    ancestor_blob = ancestor_change.get('data')
                     tip_release = self.getReleases(name=name, transaction=transaction)[0]
                     tip_blob = tip_release.get('data')
                     m = dictdiffer.merge.Merger(ancestor_blob, tip_blob, blob, {})
@@ -1638,7 +1673,7 @@ def update(self, where, what, changed_by, old_data_version, transaction=None, dr
                         unified_blob = dictdiffer.patch(m.unified_patches, ancestor_blob)
                         # converting the resultant dict into a blob and then
                         # converting it to JSON
-                        what['data'] = createBlob(unified_blob).getJSON()
+                        what['data'] = unified_blob
                         # we want the data_version for the dictdiffer.merged blob to be one
                         # more than that of the latest blob
                         tip_data_version = tip_release['data_version']
@@ -1692,7 +1727,7 @@ def addLocaleToRelease(self, name, product, platform, locale, data, old_data_ver
                     releaseBlob['platforms'][a] = {'alias': platform}
 
         releaseBlob.validate(product, self.domainWhitelist)
-        what = dict(data=releaseBlob.getJSON())
+        what = dict(data=releaseBlob)
 
         super(Releases, self).update(where=where, what=what, changed_by=changed_by, old_data_version=old_data_version,
                                      transaction=transaction)
@@ -1782,7 +1817,7 @@ def __init__(self, db, metadata, dialect):
         self.table = Table('permissions', metadata,
                            Column('permission', String(50), primary_key=True),
                            Column('username', String(100), primary_key=True),
-                           Column('options', Text)
+                           Column('options', JSONColumn)
                            )
         self.user_roles = UserRoles(db, metadata, dialect)
         AUSTable.__init__(self, db, dialect)
@@ -1817,7 +1852,6 @@ def insert(self, changed_by, transaction=None, dryrun=False, **columns):
         self.assertPermissionExists(columns["permission"])
         if columns.get("options"):
             self.assertOptionsExist(columns["permission"], columns["options"])
-            columns["options"] = json.dumps(columns["options"])
 
         if not self.db.hasPermission(changed_by, "permission", "create", transaction=transaction):
             raise PermissionDeniedError("%s is not allowed to grant permissions" % changed_by)
@@ -1849,11 +1883,6 @@ def update(self, where, what, changed_by, old_data_version, transaction=None, dr
             if what.get("options"):
                 self.assertOptionsExist(what.get("permission", current_permission["permission"]), what["options"])
 
-        if what.get("options"):
-            what["options"] = json.dumps(what["options"])
-        else:
-            what["options"] = None
-
         super(Permissions, self).update(where=where, what=what, changed_by=changed_by, old_data_version=old_data_version,
                                         transaction=transaction, dryrun=dryrun)
 
@@ -1878,34 +1907,24 @@ def revokeRole(self, username, role, changed_by=None, old_data_version=None, tra
 
     def getPermission(self, username, permission, transaction=None):
         try:
-            row = self.select(where=[self.username == username, self.permission == permission], transaction=transaction)[0]
-            if row['options']:
-                row['options'] = json.loads(row['options'])
-            return row
+            return self.select(where=[self.username == username, self.permission == permission], transaction=transaction)[0]
         except IndexError:
             return {}
 
     def getUserPermissions(self, username, transaction=None):
         rows = self.select(columns=[self.permission, self.options, self.data_version], where=[self.username == username], transaction=transaction)
         ret = dict()
         for row in rows:
-            perm = row['permission']
-            opt = row['options']
-            ret[perm] = dict()
-            ret[perm]['data_version'] = row['data_version']
-            if opt:
-                ret[perm]['options'] = json.loads(opt)
-            else:
-                ret[perm]['options'] = None
+            ret[row["permission"]] = {
+                "options": row["options"],
+                "data_version": row["data_version"]
+            }
         return ret
 
     def getOptions(self, username, permission, transaction=None):
         ret = self.select(columns=[self.options], where=[self.username == username, self.permission == permission], transaction=transaction)
         if ret:
-            if ret[0]['options']:
-                return json.loads(ret[0]['options'])
-            else:
-                return {}
+            return ret[0]["options"]
         else:
             raise ValueError('Permission "%s" doesn\'t exist' % permission)
 
@@ -1917,30 +1936,31 @@ def isAdmin(self, username, transaction=None):
         return bool(self.getPermission(username, "admin", transaction))
 
     def hasPermission(self, username, thing, action, product=None, transaction=None):
-        # Supporting product-wise admin permissions. If there are no options
-        # with admin, we assume that the user has admin access over all
-        # products.
-        if self.select(where=[self.username == username, self.permission == 'admin'], transaction=transaction):
-            options = self.getOptions(username, 'admin', transaction=transaction)
-            if options.get("products") and product not in options["products"]:
+        perm = self.getPermission(username, "admin", transaction=transaction)
+        if perm:
+            options = perm["options"]
+            if options and options.get("products") and product not in options["products"]:
+                # Supporting product-wise admin permissions. If there are no options
+                # with admin, we assume that the user has admin access over all
+                # products.
                 return False
             return True
 
-        try:
-            options = self.getOptions(username, thing, transaction=transaction)
-        except ValueError:
-            return False
-
-        # If a user has a permission that doesn't explicitly limit the type of
-        # actions they can perform, they are allowed to do any type of action.
-        if options.get("actions") and action not in options["actions"]:
-            return False
-        # Similarly, permissions without products specified grant that
-        # that permission without any limitation on the product.
-        if options.get("products") and product not in options["products"]:
-            return False
+        perm = self.getPermission(username, thing, transaction=transaction)
+        if perm:
+            options = perm["options"]
+            if options:
+                # If a user has a permission that doesn't explicitly limit the type of
+                # actions they can perform, they are allowed to do any type of action.
+                if options.get("actions") and action not in options["actions"]:
+                    return False
+                # Similarly, permissions without products specified grant that
+                # that permission without any limitation on the product.
+                if options.get("products") and product not in options["products"]:
+                    return False
+            return True
 
-        return True
+        return False
 
     def hasRole(self, username, role, transaction=None):
         return role in self.getUserRoles(username, transaction)

auslib/test/admin/views/base.py

@@ -5,6 +5,7 @@
 
 from auslib.global_state import dbo, cache
 from auslib.admin.base import app
+from auslib.blobs.base import createBlob
 
 
 class ViewTest(unittest.TestCase):
@@ -34,27 +35,27 @@ def setUp(self):
         dbo.permissions.t.insert().execute(permission='admin', username='bill', data_version=1)
         dbo.permissions.t.insert().execute(permission='permission', username='bob', data_version=1)
         dbo.permissions.t.insert().execute(permission='release', username='bob',
-                                           options=json.dumps(dict(products=['fake', 'b'], actions=["create", "modify"])), data_version=1)
-        dbo.permissions.t.insert().execute(permission='release_read_only', username='bob', options=json.dumps(dict(actions=["set"])), data_version=1)
-        dbo.permissions.t.insert().execute(permission='rule', username='bob', options=json.dumps(dict(actions=["modify"], products=['fake'])), data_version=1)
-        dbo.permissions.t.insert().execute(permission='build', username='ashanti', options=json.dumps(dict(actions=["modify"], products=['a'])), data_version=1)
-        dbo.permissions.t.insert().execute(permission="scheduled_change", username="mary", options=json.dumps(dict(actions=["enact"])), data_version=1)
+                                           options=dict(products=['fake', 'b'], actions=["create", "modify"]), data_version=1)
+        dbo.permissions.t.insert().execute(permission='release_read_only', username='bob', options=dict(actions=["set"]), data_version=1)
+        dbo.permissions.t.insert().execute(permission='rule', username='bob', options=dict(actions=["modify"], products=['fake']), data_version=1)
+        dbo.permissions.t.insert().execute(permission='build', username='ashanti', options=dict(actions=["modify"], products=['a']), data_version=1)
+        dbo.permissions.t.insert().execute(permission="scheduled_change", username="mary", options=dict(actions=["enact"]), data_version=1)
         dbo.permissions.t.insert().execute(permission='release_locale', username='ashanti',
-                                           options=json.dumps(dict(actions=["modify"], products=['a'])), data_version=1)
+                                           options=dict(actions=["modify"], products=['a']), data_version=1)
         dbo.permissions.t.insert().execute(permission='admin', username='billy',
-                                           options=json.dumps(dict(products=['a'])), data_version=1)
+                                           options=dict(products=['a']), data_version=1)
         dbo.permissions.user_roles.t.insert().execute(username="bill", role="releng", data_version=1)
         dbo.permissions.user_roles.t.insert().execute(username="bill", role="qa", data_version=1)
         dbo.permissions.user_roles.t.insert().execute(username="bob", role="relman", data_version=1)
         dbo.releases.t.insert().execute(
-            name='a', product='a', data=json.dumps(dict(name='a', hashFunction="sha512", schema_version=1)), data_version=1)
+            name='a', product='a', data=createBlob(dict(name='a', hashFunction="sha512", schema_version=1)), data_version=1)
         dbo.releases.t.insert().execute(
-            name='ab', product='a', data=json.dumps(dict(name='ab', hashFunction="sha512", schema_version=1)), data_version=1)
+            name='ab', product='a', data=createBlob(dict(name='ab', hashFunction="sha512", schema_version=1)), data_version=1)
         dbo.releases.t.insert().execute(
-            name='b', product='b', data=json.dumps(dict(name='b', hashFunction="sha512", schema_version=1)), data_version=1)
+            name='b', product='b', data=createBlob(dict(name='b', hashFunction="sha512", schema_version=1)), data_version=1)
         dbo.releases.t.insert().execute(
-            name='c', product='c', data=json.dumps(dict(name='c', hashFunction="sha512", schema_version=1)), data_version=1)
-        dbo.releases.t.insert().execute(name='d', product='d', data_version=1, data="""
+            name='c', product='c', data=createBlob(dict(name='c', hashFunction="sha512", schema_version=1)), data_version=1)
+        dbo.releases.t.insert().execute(name='d', product='d', data_version=1, data=createBlob("""
 {
     "name": "d",
     "schema_version": 1,
@@ -73,7 +74,7 @@ def setUp(self):
         }
     }
 }
-""")
+"""))
         dbo.rules.t.insert().execute(
             rule_id=1, priority=100, version='3.5', buildTarget='d', backgroundRate=100, mapping='c', update_type='minor', data_version=1
         )

auslib/test/admin/views/test_permissions.py

@@ -114,7 +114,7 @@ def testPermissionPutWithOption(self):
         query = dbo.permissions.t.select()
         query = query.where(dbo.permissions.username == 'bob')
         query = query.where(dbo.permissions.permission == 'release_locale')
-        self.assertEqual(query.execute().fetchone(), ('release_locale', 'bob', json.dumps(dict(products=['fake'])), 1))
+        self.assertEqual(query.execute().fetchone(), ('release_locale', 'bob', dict(products=['fake']), 1))
 
     def testPermissionModify(self):
         ret = self._put('/users/bob/permissions/release',
@@ -124,7 +124,7 @@ def testPermissionModify(self):
         query = dbo.permissions.t.select()
         query = query.where(dbo.permissions.username == 'bob')
         query = query.where(dbo.permissions.permission == 'release')
-        self.assertEqual(query.execute().fetchone(), ('release', 'bob', json.dumps(dict(products=['different'])), 2))
+        self.assertEqual(query.execute().fetchone(), ('release', 'bob', dict(products=['different']), 2))
 
     def testPermissionModifyWithoutDataVersion(self):
         ret = self._put("/users/bob/permissions/release",