From 48269aac77efce9f05770c55986462c5c941eece Mon Sep 17 00:00:00 2001 From: Max Lobur Date: Wed, 22 Jan 2014 08:53:26 -0500 Subject: [PATCH] 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 | 10 --- ironic/db/sqlalchemy/models.py | 32 +++++++-- ironic/tests/db/sqlalchemy/test_types.py | 82 ++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 17 deletions(-) create mode 100644 ironic/tests/db/sqlalchemy/test_types.py diff --git a/ironic/db/sqlalchemy/api.py b/ironic/db/sqlalchemy/api.py index 7eddac76a6..44389e112a 100644 --- a/ironic/db/sqlalchemy/api.py +++ b/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() diff --git a/ironic/db/sqlalchemy/models.py b/ironic/db/sqlalchemy/models.py index 5cb2e57d98..55b2e61bca 100644 --- a/ironic/db/sqlalchemy/models.py +++ b/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): diff --git a/ironic/tests/db/sqlalchemy/test_types.py b/ironic/tests/db/sqlalchemy/test_types.py new file mode 100644 index 0000000000..4858aafc09 --- /dev/null +++ b/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'}})