Re-add str_replace parameter validation, and fix test

This partially reverts https://review.openstack.org/#/c/275602/, which
removed the validation of the parameters argument completely, but a
comment added post-merge rightly points out that we can just check for
a map or function and still fix the bug.

Also, it seems that we had an existing test which failed to catch the
fact that this didn't work, so fix that to prove the expected
behavior works in the context of a stack, not just when resolving a
function snippet, and add a test which proves the reinstated
constructor validation works as expected.

Change-Id: I21c1868061e37760e1cb27f44ebb7c4ee97fefef
Partial-Bug: #1539737

heat/engine/cfn/functions.py

@@ -401,6 +401,10 @@ class Replace(function.Function):
         super(Replace, self).__init__(stack, fn_name, args)
 
         self._mapping, self._string = self._parse_args()
+        if not isinstance(self._mapping,
+                          (collections.Mapping, function.Function)):
+            raise TypeError(_('"%s" parameters must be a mapping') %
+                            self.fn_name)
 
     def _parse_args(self):
 

heat/tests/common.py

@@ -165,6 +165,8 @@ class HeatTestCase(testscenarios.WithScenarios,
             generic_rsrc.ResourceWithRequiredPropsAndEmptyAttrs)
         resource._register_class('ResourceWithPropsAndAttrs',
                                  generic_rsrc.ResourceWithPropsAndAttrs)
+        resource._register_class('ResWithStringPropAndAttr',
+                                 generic_rsrc.ResWithStringPropAndAttr),
         resource._register_class('ResWithComplexPropsAndAttrs',
                                  generic_rsrc.ResWithComplexPropsAndAttrs)
         resource._register_class('ResourceWithCustomConstraint',

heat/tests/generic_resource.py

@@ -69,7 +69,21 @@ class ResWithShowAttr(GenericResource):
                 'Another': self.name}
 
 
-class ResWithComplexPropsAndAttrs(GenericResource):
+class ResWithStringPropAndAttr(GenericResource):
+
+    properties_schema = {
+        'a_string': properties.Schema(properties.Schema.STRING)}
+
+    attributes_schema = {'string': attributes.Schema('A string')}
+
+    def _resolve_attribute(self, name):
+        try:
+            return self.properties["a_%s" % name]
+        except KeyError:
+            return None
+
+
+class ResWithComplexPropsAndAttrs(ResWithStringPropAndAttr):
 
     properties_schema = {
         'a_string': properties.Schema(properties.Schema.STRING),

heat/tests/test_hot.py

@@ -18,6 +18,7 @@ import six
 from heat.common import exception
 from heat.common import identifier
 from heat.common import template_format
+from heat.engine.cfn import functions as cfn_functions
 from heat.engine import environment
 from heat.engine import function
 from heat.engine.hot import functions as hot_functions
@@ -611,7 +612,22 @@ class HOTemplateTest(common.HeatTestCase):
         snippet = {'str_replace': {'template': 'Template var1 string var2',
                                    'params': ['var1', 'foo', 'var2', 'bar']}}
 
-        self.assertRaises(TypeError, self.resolve, snippet, tmpl)
+        ex = self.assertRaises(TypeError, self.resolve, snippet, tmpl)
+        self.assertIn('parameters must be a mapping', six.text_type(ex))
+
+    def test_str_replace_invalid_param_type_init(self):
+        """Test str_replace function parameter values.
+
+        Pass parameter values of wrong type to function and verify that we get
+        a TypeError in the constructor.
+        """
+        args = [['var1', 'foo', 'var2', 'bar'],
+                'Template var1 string var2']
+        ex = self.assertRaises(
+            TypeError,
+            cfn_functions.Replace,
+            None, 'Fn::Replace', args)
+        self.assertIn('parameters must be a mapping', six.text_type(ex))
 
     def test_str_replace_ref_get_param(self):
         """Test str_replace referencing parameters."""
@@ -625,16 +641,25 @@ class HOTemplateTest(common.HeatTestCase):
             type: json
             default:
               replaceme: success
+        resources:
+          rsrc:
+            type: ResWithStringPropAndAttr
+            properties:
+              a_string:
+                str_replace:
+                  template: {get_param: p_template}
+                  params: {get_param: p_params}
+        outputs:
+          replaced:
+            value: {get_attr: [rsrc, string]}
         ''')
-        snippet = {'str_replace':
-                   {'template': {'get_param': 'p_template'},
-                    'params': {'get_param': 'p_params'}}}
-        snippet_resolved = 'foo-success'
-
         tmpl = template.Template(hot_tpl)
-        stack = parser.Stack(utils.dummy_context(), 'test_stack', tmpl)
-
-        self.assertEqual(snippet_resolved, self.resolve(snippet, tmpl, stack))
+        self.stack = parser.Stack(utils.dummy_context(), 'test_stack', tmpl)
+        self.stack.store()
+        self.stack.create()
+        self.assertEqual((parser.Stack.CREATE, parser.Stack.COMPLETE),
+                         self.stack.state)
+        self.assertEqual('foo-success', self.stack.output('replaced'))
 
     def test_get_file(self):
         """Test get_file function."""