diff --git a/st2client/st2client/commands/action.py b/st2client/st2client/commands/action.py index 48964746a5..bfe9dccd74 100644 --- a/st2client/st2client/commands/action.py +++ b/st2client/st2client/commands/action.py @@ -1206,14 +1206,19 @@ def _get_parameter_sort_value(self, parameters, name): By default, parameters are sorted using "position" parameter attribute. If this attribute is not available, parameter is sorted based on the name. + + Returns a tuple (tier, value) so that parameters with a numeric position + sort before those without, and mixed int/str comparisons are avoided. """ parameter = parameters.get(name, None) if not parameter: - return None + return (1, 0, name) - sort_value = parameter.get("position", name) - return sort_value + position = parameter.get("position", None) + if position is not None: + return (0, int(position), "") + return (1, 0, name) def _get_inherited_env_vars(self): env_vars = os.environ.copy() diff --git a/st2client/tests/unit/test_command_actionrun.py b/st2client/tests/unit/test_command_actionrun.py index 805491f189..e567333869 100644 --- a/st2client/tests/unit/test_command_actionrun.py +++ b/st2client/tests/unit/test_command_actionrun.py @@ -393,3 +393,77 @@ def test_correctly_generate_empty_params_no_inherit_empty_parameters(self): ) self.assertDictEqual({}, param) + + def test_sort_parameters_mixed_position_types(self): + """Regression test for #5130. + + When some parameters have a numeric 'position' attribute and others do + not, sorted() previously raised TypeError because the sort key returned + either int or str depending on the parameter. Verify that sorting now + succeeds and that positioned parameters come first (by position), with + unpositioned parameters following in alphabetical order. + """ + action = Action() + action.ref = "test.action" + action.parameters = {} + + subparser = mock.Mock() + command = ActionRunCommand(action, self, subparser, name="test") + + parameters = { + "host": {"type": "string", "position": 0}, + "timeout": {"type": "integer", "position": 1}, + "verbose": {"type": "boolean"}, + "output": {"type": "string"}, + } + names = ["verbose", "output", "host", "timeout"] + + # Must not raise TypeError + result = command._sort_parameters(parameters=parameters, names=names) + + # Positioned params come first, in ascending position order + self.assertEqual(result[0], "host") + self.assertEqual(result[1], "timeout") + # Unpositioned params follow, alphabetically + self.assertEqual(result[2], "output") + self.assertEqual(result[3], "verbose") + + def test_sort_parameters_all_positioned(self): + """Parameters that all have a 'position' attribute sort by position.""" + action = Action() + action.ref = "test.action" + action.parameters = {} + + subparser = mock.Mock() + command = ActionRunCommand(action, self, subparser, name="test") + + parameters = { + "z_param": {"type": "string", "position": 2}, + "a_param": {"type": "string", "position": 0}, + "m_param": {"type": "string", "position": 1}, + } + names = ["z_param", "a_param", "m_param"] + + result = command._sort_parameters(parameters=parameters, names=names) + + self.assertEqual(result, ["a_param", "m_param", "z_param"]) + + def test_sort_parameters_none_positioned(self): + """Parameters with no 'position' attribute sort alphabetically by name.""" + action = Action() + action.ref = "test.action" + action.parameters = {} + + subparser = mock.Mock() + command = ActionRunCommand(action, self, subparser, name="test") + + parameters = { + "zebra": {"type": "string"}, + "apple": {"type": "string"}, + "mango": {"type": "string"}, + } + names = ["zebra", "apple", "mango"] + + result = command._sort_parameters(parameters=parameters, names=names) + + self.assertEqual(result, ["apple", "mango", "zebra"])