Add JSONEncodedType with enforced type checking

Add abstract base class for JSONEncoded properties which: 1. Does type
checking to make sure we're serializing what we really expect
2. Ensures that default value of the field matches default value of
that type, so we have consistent interface. This patch creates two child
classes JSONEncodedDict and JSONEncodedList and updates SA models
correspondingly.

Change-Id: I528ac64e998e9a874feb16709d7617feb4b09c8a
Closes-Bug: #1270146

ironic/db/sqlalchemy/api.py

@@ -279,12 +279,6 @@ class Connection(api.Connection):
             values['power_state'] = states.NOSTATE
         if not values.get('provision_state'):
             values['provision_state'] = states.NOSTATE
-        if not values.get('properties'):
-            values['properties'] = '{}'
-        if not values.get('extra'):
-            values['extra'] = '{}'
-        if not values.get('driver_info'):
-            values['driver_info'] = '{}'
 
         node = models.Node()
         node.update(values)
@@ -400,8 +394,6 @@ class Connection(api.Connection):
     def create_port(self, values):
         if not values.get('uuid'):
             values['uuid'] = utils.generate_uuid()
-        if not values.get('extra'):
-            values['extra'] = '{}'
         port = models.Port()
         port.update(values)
         try:
@@ -460,8 +452,6 @@ class Connection(api.Connection):
     def create_chassis(self, values):
         if not values.get('uuid'):
             values['uuid'] = utils.generate_uuid()
-        if not values.get('extra'):
-            values['extra'] = '{}'
         chassis = models.Chassis()
         chassis.update(values)
         chassis.save()

ironic/db/sqlalchemy/models.py

@@ -49,15 +49,23 @@ def table_args():
     return None
 
 
-class JSONEncodedDict(TypeDecorator):
-    """Represents an immutable structure as a json-encoded string."""
-
+class JsonEncodedType(TypeDecorator):
+    """Abstract base type serialized as json-encoded string in db."""
+    type = None
     impl = VARCHAR
 
     def process_bind_param(self, value, dialect):
-        if value is not None:
-            value = json.dumps(value)
-        return value
+        if value is None:
+            # Save default value according to current type to keep the
+            # interface the consistent.
+            value = self.type()
+        elif not isinstance(value, self.type):
+            raise TypeError("%s supposes to store %s objects, but %s given"
+                            % (self.__class__.__name__,
+                               self.type.__name__,
+                               type(value).__name__))
+        serialized_value = json.dumps(value)
+        return serialized_value
 
     def process_result_value(self, value, dialect):
         if value is not None:
@@ -65,6 +73,16 @@ class JSONEncodedDict(TypeDecorator):
         return value
 
 
+class JSONEncodedDict(JsonEncodedType):
+    """Represents dict serialized as json-encoded string in db."""
+    type = dict
+
+
+class JSONEncodedList(JsonEncodedType):
+    """Represents list serialized as json-encoded string in db."""
+    type = list
+
+
 class IronicBase(models.TimestampMixin,
                  models.ModelBase):
 
@@ -102,7 +120,7 @@ class Conductor(Base):
         )
     id = Column(Integer, primary_key=True)
     hostname = Column(String(255), nullable=False)
-    drivers = Column(JSONEncodedDict)
+    drivers = Column(JSONEncodedList)
 
 
 class Node(Base):

ironic/tests/db/sqlalchemy/test_types.py

@@ -0,0 +1,82 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+"""Tests for custom SQLAlchemy types via Ironic DB."""
+
+from ironic.openstack.common.db import exception as db_exc
+
+from ironic.common import utils as ironic_utils
+from ironic.db import api as dbapi
+import ironic.db.sqlalchemy.api as sa_api
+from ironic.db.sqlalchemy import models
+
+from ironic.tests.db import base
+
+
+class SqlAlchemyCustomTypesTestCase(base.DbTestCase):
+
+    def setUp(self):
+        super(SqlAlchemyCustomTypesTestCase, self).setUp()
+        self.dbapi = dbapi.get_instance()
+
+    # NOTE(max_lobur): Since it's not straightforward to check this in
+    #                  isolation these tests use existing db models.
+
+    def test_JSONEncodedDict_default_value(self):
+        # Create chassis w/o extra specified.
+        ch1_id = ironic_utils.generate_uuid()
+        self.dbapi.create_chassis({'uuid': ch1_id})
+        # Get chassis manually to test SA types in isolation from UOM.
+        ch1 = sa_api.model_query(models.Chassis).filter_by(uuid=ch1_id).one()
+        self.assertEqual({}, ch1.extra)
+
+        # Create chassis with extra specified.
+        ch2_id = ironic_utils.generate_uuid()
+        extra = {'foo1': 'test', 'foo2': 'other extra'}
+        self.dbapi.create_chassis({'uuid': ch2_id, 'extra': extra})
+        # Get chassis manually to test SA types in isolation from UOM.
+        ch2 = sa_api.model_query(models.Chassis).filter_by(uuid=ch2_id).one()
+        self.assertEqual(extra, ch2.extra)
+
+    def test_JSONEncodedDict_type_check(self):
+        self.assertRaises(db_exc.DBError,
+                          self.dbapi.create_chassis,
+                          {'extra':
+                               ['this is not a dict']})
+
+    def test_JSONEncodedLict_default_value(self):
+        # Create conductor w/o extra specified.
+        cdr1_id = 321321
+        self.dbapi.register_conductor({'hostname': 'test_host1',
+                                       'drivers': None,
+                                       'id': cdr1_id})
+        # Get conductor manually to test SA types in isolation from UOM.
+        cdr1 = sa_api.model_query(models.Conductor).filter_by(id=cdr1_id)\
+            .one()
+        self.assertEqual([], cdr1.drivers)
+
+        # Create conductor with drivers specified.
+        cdr2_id = 623623
+        drivers = ['foo1', 'other driver']
+        self.dbapi.register_conductor({'hostname': 'test_host2',
+                                       'drivers': drivers,
+                                       'id': cdr2_id})
+        # Get conductor manually to test SA types in isolation from UOM.
+        cdr2 = sa_api.model_query(models.Conductor).filter_by(id=cdr2_id)\
+            .one()
+        self.assertEqual(drivers, cdr2.drivers)
+
+    def test_JSONEncodedList_type_check(self):
+        self.assertRaises(db_exc.DBError,
+                          self.dbapi.register_conductor,
+                          {'drivers':
+                               {'this is not a list': 'test'}})