From 815d5b6ba5da0a384e32a45bba5d6588d424fd88 Mon Sep 17 00:00:00 2001 From: Sergey Kraynev Date: Mon, 25 Aug 2014 09:39:44 -0400 Subject: [PATCH] Deprecate using required=True with default value The current schema of constraints allow to use default values for required=True. However we never use it except in unittests, because user should set it always, which looks like not required property. This patch adds checking for constraint schema class with warning message. Change-Id: Ic6ace30742e887069f119a14ffb091f6bac9e682 --- heat/engine/constraints.py | 5 ++++ heat/tests/test_constraints.py | 46 ++++++++++++++++++++++++---------- heat/tests/test_properties.py | 22 +++++++--------- 3 files changed, 47 insertions(+), 26 deletions(-) diff --git a/heat/engine/constraints.py b/heat/engine/constraints.py index 4da9ae31b2..4c522f4b59 100644 --- a/heat/engine/constraints.py +++ b/heat/engine/constraints.py @@ -14,6 +14,7 @@ import collections import numbers import re +import warnings from oslo_utils import strutils import six @@ -84,6 +85,10 @@ class Schema(collections.Mapping): raise exception.InvalidSchemaError( message=_('Invalid type (%s)') % self.type) + if required and default is not None: + warnings.warn("Option 'required=True' should not be used with " + "any 'default' value ({0})".format(default)) + self.description = description self.required = required diff --git a/heat/tests/test_constraints.py b/heat/tests/test_constraints.py index 2a4023b887..fd468cfde2 100644 --- a/heat/tests/test_constraints.py +++ b/heat/tests/test_constraints.py @@ -11,7 +11,7 @@ # License for the specific language governing permissions and limitations # under the License. - +import mock import six from heat.common import exception @@ -21,6 +21,26 @@ from heat.tests import common class SchemaTest(common.HeatTestCase): + @mock.patch('warnings.warn') + def test_warn_required_with_default(self, mock_warn): + constraints.Schema(constraints.Schema.STRING, 'A string', + default='wibble', required=True) + msg = ("Option 'required=True' should not be used with any 'default' " + "value (wibble)") + mock_warn.assert_called_once_with(msg) + + @mock.patch('warnings.warn') + def test_without_warn_only_default(self, mock_warn): + constraints.Schema(constraints.Schema.STRING, 'A string', + default='wibble') + self.assertEqual(0, mock_warn.call_count) + + @mock.patch('warnings.warn') + def test_without_warn_only_required(self, mock_warn): + constraints.Schema(constraints.Schema.STRING, 'A string', + required=True) + self.assertEqual(0, mock_warn.call_count) + def test_range_schema(self): d = {'range': {'min': 5, 'max': 10}, 'description': 'a range'} r = constraints.Range(5, 10, description='a range') @@ -92,13 +112,13 @@ class SchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'constraints': [ {'length': {'min': 4, 'max': 8}}, ] } s = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) self.assertEqual(d, dict(s)) @@ -111,7 +131,7 @@ class SchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'constraints': [ {'length': {'min': 4, 'max': 8}}, ] @@ -120,7 +140,7 @@ class SchemaTest(common.HeatTestCase): 'required': False, } s = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) l = constraints.Schema(constraints.Schema.LIST, 'A list', schema=s) self.assertEqual(d, dict(l)) @@ -134,7 +154,7 @@ class SchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'constraints': [ {'length': {'min': 4, 'max': 8}}, ] @@ -143,7 +163,7 @@ class SchemaTest(common.HeatTestCase): 'required': False, } s = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) m = constraints.Schema(constraints.Schema.MAP, 'A map', schema={'Foo': s}) @@ -162,7 +182,7 @@ class SchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'constraints': [ {'length': {'min': 4, 'max': 8}}, ] @@ -174,7 +194,7 @@ class SchemaTest(common.HeatTestCase): 'required': False, } s = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) m = constraints.Schema(constraints.Schema.MAP, 'A map', schema={'Foo': s}) @@ -231,13 +251,13 @@ class SchemaTest(common.HeatTestCase): def test_schema_validate_good(self): s = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) self.assertIsNone(s.validate()) def test_schema_validate_fail(self): s = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Range(max=4)]) err = self.assertRaises(exception.InvalidSchemaError, s.validate) self.assertIn('Range constraint invalid for String', @@ -245,7 +265,7 @@ class SchemaTest(common.HeatTestCase): def test_schema_nested_validate_good(self): nested = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) s = constraints.Schema(constraints.Schema.MAP, 'A map', schema={'Foo': nested}) @@ -253,7 +273,7 @@ class SchemaTest(common.HeatTestCase): def test_schema_nested_validate_fail(self): nested = constraints.Schema(constraints.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Range(max=4)]) s = constraints.Schema(constraints.Schema.MAP, 'A map', schema={'Foo': nested}) diff --git a/heat/tests/test_properties.py b/heat/tests/test_properties.py index 41caa0447f..b90b360b0b 100644 --- a/heat/tests/test_properties.py +++ b/heat/tests/test_properties.py @@ -31,7 +31,7 @@ class PropertySchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'update_allowed': False, 'immutable': False, 'constraints': [ @@ -39,7 +39,7 @@ class PropertySchemaTest(common.HeatTestCase): ] } s = properties.Schema(properties.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) self.assertEqual(d, dict(s)) @@ -52,7 +52,7 @@ class PropertySchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'update_allowed': False, 'immutable': False, 'constraints': [ @@ -65,7 +65,7 @@ class PropertySchemaTest(common.HeatTestCase): 'immutable': False, } s = properties.Schema(properties.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) l = properties.Schema(properties.Schema.LIST, 'A list', schema=s) self.assertEqual(d, dict(l)) @@ -79,7 +79,7 @@ class PropertySchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'update_allowed': False, 'immutable': False, 'constraints': [ @@ -92,7 +92,7 @@ class PropertySchemaTest(common.HeatTestCase): 'immutable': False, } s = properties.Schema(properties.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) m = properties.Schema(properties.Schema.MAP, 'A map', schema={'Foo': s}) @@ -111,7 +111,7 @@ class PropertySchemaTest(common.HeatTestCase): 'type': 'string', 'description': 'A string', 'default': 'wibble', - 'required': True, + 'required': False, 'update_allowed': False, 'immutable': False, 'constraints': [ @@ -129,7 +129,7 @@ class PropertySchemaTest(common.HeatTestCase): 'immutable': False, } s = properties.Schema(properties.Schema.STRING, 'A string', - default='wibble', required=True, + default='wibble', constraints=[constraints.Length(4, 8)]) m = properties.Schema(properties.Schema.MAP, 'A map', schema={'Foo': s}) @@ -162,7 +162,6 @@ class PropertySchemaTest(common.HeatTestCase): 'Type': 'String', 'Description': 'a string', 'Default': 'wibble', - 'Required': True, 'Implemented': False, 'MinLength': 4, 'MaxLength': 8, @@ -172,7 +171,7 @@ class PropertySchemaTest(common.HeatTestCase): self.assertEqual(properties.Schema.STRING, s.type) self.assertEqual('a string', s.description) self.assertEqual('wibble', s.default) - self.assertTrue(s.required) + self.assertFalse(s.required) self.assertEqual(3, len(s.constraints)) self.assertFalse(s.immutable) @@ -1618,7 +1617,6 @@ class PropertiesValidationTest(common.HeatTestCase): nested_schema = {'Key': {'Type': 'String', 'Required': True}, 'Value': {'Type': 'String', - 'Required': True, 'Default': 'fewaf'}} schema = {'foo': {'Type': 'Map', 'Schema': nested_schema}} @@ -1707,7 +1705,6 @@ class PropertiesValidationTest(common.HeatTestCase): child_schema = {'Key': {'Type': 'String', 'Required': True}, 'Value': {'Type': 'Boolean', - 'Required': True, 'Default': True}} list_schema = {'Type': 'Map', 'Schema': child_schema} schema = {'foo': {'Type': 'List', 'Schema': list_schema}} @@ -1727,7 +1724,6 @@ class PropertiesValidationTest(common.HeatTestCase): child_schema = {'Key': {'Type': 'String', 'Required': True}, 'Value': {'Type': 'Boolean', - 'Required': True, 'Default': True}} map_schema = {'boo': {'Type': 'Map', 'Schema': child_schema}} schema = {'foo': {'Type': 'Map', 'Schema': map_schema}}