Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pass without exception if jinja parameter isn't found #6101

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions st2api/tests/unit/controllers/v1/test_executions.py
Original file line number Diff line number Diff line change
Expand Up @@ -774,10 +774,8 @@ def test_post_parameter_render_failed(self):
# Runner type does not expects additional properties.
execution["parameters"]["hosts"] = "{{ABSENT}}"
post_resp = self._do_post(execution, expect_errors=True)
self.assertEqual(post_resp.status_int, 400)
self.assertEqual(
post_resp.json["faultstring"], 'Dependency unsatisfied in variable "ABSENT"'
)
# we no longer fail if parameter is not found
self.assertEqual(post_resp.status_int, 201)

def test_post_parameter_validation_explicit_none(self):
execution = copy.deepcopy(LIVE_ACTION_1)
Expand Down
82 changes: 72 additions & 10 deletions st2common/st2common/util/param.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,17 +170,80 @@ def _process_defaults(G, schemas):
_process(G, name, value.get("default"))


def _check_any_bad(G, nodes, check_any_bad=None, tracked_parents=None):
"""
:param G: nx.DiGraph
:param nodes: list[dict]
:param check_any_bad: list[boolean]
:param tracked_parents: list[str]
"""
if tracked_parents is None:
tracked_parents = []
if check_any_bad is None:
check_any_bad = []
for name in nodes:
if "value" not in G.nodes[name] and "template" not in G.nodes[name]:
# this is a string not a jinja template; embedded {{sometext}}
check_any_bad.append(False)
else:
check_any_bad.append(True)
children = [i for i in G.predecessors(name)]
if name not in tracked_parents:
tracked_parents.extend(nodes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
tracked_parents.extend(nodes)
tracked_parents.append(name)

With extend, only the first child gets checked, not any of the others. Is that intentional? If so, it's not clear why we want to do that.

_check_any_bad(G, children, check_any_bad, tracked_parents)
guzzijones marked this conversation as resolved.
Show resolved Hide resolved
return check_any_bad


def _remove_bad(g_copy, parent, nodes, tracked_parents=None):
"""
:param G: nx.DiGraph
:param nodes: str
:param check_any_bad: list[str]
:param tracked_parents: list[str]
"""

if tracked_parents is None:
tracked_parents = [parent]
else:
tracked_parents.append(parent)
for i in nodes:
g_copy.nodes[parent]["value"] = g_copy.nodes[parent].pop("template")
guzzijones marked this conversation as resolved.
Show resolved Hide resolved
if i in g_copy.nodes:
Copy link
Member

@cognifloyd cognifloyd Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i is reused here a couple of times. Could you rename this one to something like node or name to make its purpose clearer?

children = [i for i in g_copy.predecessors(i)]
if children:
if i not in tracked_parents:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see anywhere that adds the children to tracked_parents while walking the tree which means the if i not in tracked_parents: condition will always be true.

_remove_bad(g_copy, i, children)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this line need to pass in tracked_parents? If not, why not?

# remove template for neighbors; this isn't actually a variable
# it is a value
# remove template attr if it exists on parent
# remove edges
g_copy.remove_edge(i, parent)
# remove node from graph
g_copy.remove_node(i)


def _validate(G):
"""
Validates dependency graph to ensure it has no missing or cyclic dependencies
"""
g_copy = G.copy()
for name in G.nodes:
guzzijones marked this conversation as resolved.
Show resolved Hide resolved
if "value" not in G.nodes[name] and "template" not in G.nodes[name]:
msg = 'Dependency unsatisfied in variable "%s"' % name
raise ParamException(msg)

if not nx.is_directed_acyclic_graph(G):
graph_cycles = nx.simple_cycles(G)
children = [i for i in G.predecessors(name)]
if len(children) > 0:
# has children
check_all = _check_any_bad(G, children)
# check if all are FALSE
if not any(check_all):
# this is a string or object with embedded jinja string that
# doesn't exist as a parameter and not a jinja template; embedded {{sometext}}
_remove_bad(g_copy, name, children)
elif True in check_all and False in check_all:
msg = 'Dependency unsatisfied in variable "%s"' % name
# one of the parameters exists but not all
raise ParamException(msg)

if not nx.is_directed_acyclic_graph(g_copy):
graph_cycles = nx.simple_cycles(g_copy)

variable_names = []
for cycle in graph_cycles:
Expand All @@ -197,6 +260,7 @@ def _validate(G):
"referencing itself" % (variable_names)
)
raise ParamException(msg)
return g_copy


def _render(node, render_context):
Expand Down Expand Up @@ -276,7 +340,6 @@ def _cast_params_from(params, context, schemas):
# value is a template, it is rendered and added to the live params before this validation.
for schema in schemas:
for param_name, param_details in schema.items():

# Skip if the parameter have immutable set to true in schema
if param_details.get("immutable"):
continue
Expand Down Expand Up @@ -336,8 +399,7 @@ def render_live_params(

[_process(G, name, value) for name, value in six.iteritems(params)]
_process_defaults(G, [action_parameters, runner_parameters])
_validate(G)

G = _validate(G)
context = _resolve_dependencies(G)
live_params = _cast_params_from(
params, context, [action_parameters, runner_parameters]
Expand All @@ -359,7 +421,7 @@ def render_final_params(runner_parameters, action_parameters, params, action_con
# by that point, all params should already be resolved so any template should be treated value
[G.add_node(name, value=value) for name, value in six.iteritems(params)]
_process_defaults(G, [action_parameters, runner_parameters])
_validate(G)
G = _validate(G)

context = _resolve_dependencies(G)
context = _cast_params_from(
Expand Down
42 changes: 13 additions & 29 deletions st2common/tests/unit/test_param_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ class ParamsUtilsTest(DbTestCase):
runnertype_db = FIXTURES["runners"]["testrunner1.yaml"]

def test_process_jinja_exception(self):

action_context = {"api_user": "noob"}
config = {}
G = param_utils._create_graph(action_context, config)
Expand All @@ -69,7 +68,6 @@ def test_process_jinja_exception(self):
self.assertEquals(G.nodes.get(name, {}).get("value"), value)

def test_process_jinja_template(self):

action_context = {"api_user": "noob"}
config = {}
G = param_utils._create_graph(action_context, config)
Expand Down Expand Up @@ -524,28 +522,20 @@ def test_get_finalized_params_with_missing_dependency(self):
params = {"r1": "{{r3}}", "r2": "{{r3}}"}
runner_param_info = {"r1": {}, "r2": {}}
action_param_info = {}
test_pass = True
try:
param_utils.get_finalized_params(
runner_param_info, action_param_info, params, {"user": None}
)
test_pass = False
except ParamException as e:
test_pass = six.text_type(e).find("Dependency") == 0
self.assertTrue(test_pass)
result = param_utils.get_finalized_params(
runner_param_info, action_param_info, params, {"user": None}
)
self.assertEquals(result[0]["r1"], params["r1"])
self.assertEquals(result[0]["r2"], params["r2"])

params = {}
runner_param_info = {"r1": {"default": "{{r3}}"}, "r2": {"default": "{{r3}}"}}
action_param_info = {}
test_pass = True
try:
param_utils.get_finalized_params(
runner_param_info, action_param_info, params, {"user": None}
)
test_pass = False
except ParamException as e:
test_pass = six.text_type(e).find("Dependency") == 0
self.assertTrue(test_pass)
result2 = param_utils.get_finalized_params(
runner_param_info, action_param_info, params, {"user": None}
)
self.assertEquals(result2[0]["r1"], runner_param_info["r1"]["default"])
self.assertEquals(result2[0]["r2"], runner_param_info["r2"]["default"])

def test_get_finalized_params_no_double_rendering(self):
params = {"r1": "{{ action_context.h1 }}{{ action_context.h2 }}"}
Expand Down Expand Up @@ -788,16 +778,10 @@ def test_unsatisfied_dependency_friendly_error_message(self):
}
action_context = {"user": None}

expected_msg = 'Dependency unsatisfied in variable "variable_not_defined"'
self.assertRaisesRegexp(
ParamException,
expected_msg,
param_utils.render_live_params,
runner_param_info,
action_param_info,
params,
action_context,
result = param_utils.render_live_params(
runner_param_info, action_param_info, params, action_context
)
self.assertEquals(result["r4"], params["r4"])

def test_add_default_templates_to_live_params(self):
"""Test addition of template values in defaults to live params"""
Expand Down
Loading