Merge pull request #707 from bennettoxford/aw/command-verification

Validate that a parameterised Slack command matches the corresponding `run_args_template`

bennettbot/job_configs.py

@@ -630,10 +630,15 @@ def build_config(raw_config):
     config["slack"] = sorted(config["slack"], key=itemgetter("command"))
 
     for slack_config in config["slack"]:
-        if slack_config["job_type"] not in config["jobs"]:
-            msg = f"Slack command {slack_config['command']} references unknown job type {slack_config['job_type']}"
+        command = slack_config["command"]
+        job_type = config["jobs"].get(slack_config["job_type"])
+        if not job_type:
+            msg = f"Slack command {command} references unknown job type {slack_config['job_type']}"
             raise RuntimeError(msg)
-
+        if slack_config["action"] == "schedule_job":
+            validate_job_and_slack_params_match(
+                command, slack_config["template_params"], job_type["run_args_template"]
+            )
     return config
 
 
@@ -648,14 +653,17 @@ def build_regex_from_command(command):
     return re.compile(pattern)
 
 
-def get_template_params(command):
-    """Extract parameters from Slack command.
+def get_template_params(command, wrapper="[]"):
+    """Extract parameters from a Slack command or arguments template.
 
     >>> get_template_params("say [greeting] to [name]")
     ["greeting", "name"]
+    >>> get_template_params("say {greeting} to {name}", wrapper="{}")
+    ["greeting", "name"]
     """
-
-    return re.findall(r"\[(\w+)\]", command)
+    start, end = wrapper
+    regex = f"{re.escape(start)}(\\w+){re.escape(end)}"
+    return re.findall(regex, command)
 
 
 def validate_job_config(job_type, job_config):
@@ -708,4 +716,11 @@ def validate_slack_config(slack_config):
         raise RuntimeError(msg)
 
 
+def validate_job_and_slack_params_match(command, params, run_args_template):
+    run_args = get_template_params(run_args_template, wrapper="{}")
+    if set(params) != set(run_args):
+        msg = f"Slack command {command} does not match the template {run_args_template}"
+        raise RuntimeError(msg)
+
+
 config = build_config(raw_config)

tests/job_configs.py

@@ -12,7 +12,7 @@
                 "run_args_template": "cat poem",
             },
             "parameterised_job": {
-                "run_args_template": "cat {path}",
+                "run_args_template": "cp poem poem{n}; cat poem{n}; rm poem{n}",
             },
             "parameterised_job_2": {
                 "run_args_template": "echo {thing_to_echo}",
@@ -78,12 +78,19 @@
         },
         "slack": [
             {
-                "command": "do job [n]",
-                "help": "do the job",
+                "command": "do good job",
+                "help": "do the good job",
                 "action": "schedule_job",
                 "job_type": "good_job",
                 "delay_seconds": 60,
             },
+            {
+                "command": "do job [n]",
+                "help": "do the parameterised job",
+                "action": "schedule_job",
+                "job_type": "parameterised_job",
+                "delay_seconds": 60,
+            },
             {
                 "command": "cancel job",
                 "help": "cancel the job",

tests/test_bot.py

@@ -108,14 +108,16 @@ def test_register_listeners_invalid_support_channel_settings(setting, value):
 def test_schedule_job(mock_app):
     handle_message(mock_app, "<@U1234> test do job 10")
 
-    jj = scheduler.get_jobs_of_type("test_good_job")
+    jj = scheduler.get_jobs_of_type("test_parameterised_job")
     assert len(jj) == 1
-    assert_job_matches(jj[0], "test_good_job", {"n": "10"}, "channel", T(60), None)
+    assert_job_matches(
+        jj[0], "test_parameterised_job", {"n": "10"}, "channel", T(60), None
+    )
 
 
 def test_schedule_job_with_job_already_running(mock_app):
     with patch("bennettbot.scheduler.schedule_job", return_value=True):
-        handle_message(mock_app, "<@U1234> test do job 1")
+        handle_message(mock_app, "<@U1234> test do good job")
         assert_slack_client_sends_messages(
             messages_kwargs=[{"channel": "channel", "text": "already started"}],
         )
@@ -124,9 +126,11 @@ def test_schedule_job_with_job_already_running(mock_app):
 def test_schedule_job_from_reminder(mock_app):
     handle_message(mock_app, "Reminder: <@U1234|test bot> test do job 10")
 
-    jj = scheduler.get_jobs_of_type("test_good_job")
+    jj = scheduler.get_jobs_of_type("test_parameterised_job")
     assert len(jj) == 1
-    assert_job_matches(jj[0], "test_good_job", {"n": "10"}, "channel", T(60), None)
+    assert_job_matches(
+        jj[0], "test_parameterised_job", {"n": "10"}, "channel", T(60), None
+    )
 
 
 def test_schedule_python_job(mock_app):
@@ -155,7 +159,7 @@ def test_url_formatting_removed(mock_app, message):
 
 
 def test_cancel_job(mock_app):
-    handle_message(mock_app, "<@U1234> test do job 10")
+    handle_message(mock_app, "<@U1234> test do good job")
     assert scheduler.get_jobs_of_type("test_good_job")
 
     handle_message(mock_app, "<@U1234> test cancel job", reaction_count=2)
@@ -238,7 +242,7 @@ def test_namespace_help(mock_app):
     handle_message(mock_app, "<@U1234> test help", reaction_count=0)
     assert_slack_client_sends_messages(
         messages_kwargs=[
-            {"channel": "channel", "text": "`test do job [n]`: do the job"}
+            {"channel": "channel", "text": "`test do good job`: do the good job"}
         ],
     )
 
@@ -335,7 +339,7 @@ def test_message_parsing(mock_app, pre_message, message):
     handle_message(mock_app, f"{pre_message}<@U1234>{message}", reaction_count=0)
     assert_slack_client_sends_messages(
         messages_kwargs=[
-            {"channel": "channel", "text": "`test do job [n]`: do the job"}
+            {"channel": "channel", "text": "`test do good job`: do the good job"}
         ],
     )
 
@@ -367,7 +371,7 @@ def test_direct_message(mock_app, message):
     )
     assert_slack_client_sends_messages(
         messages_kwargs=[
-            {"channel": "IM0001", "text": "`test do job [n]`: do the job"}
+            {"channel": "IM0001", "text": "`test do good job`: do the good job"}
         ],
     )
 
@@ -800,7 +804,7 @@ def test_new_channel_created(mock_app):
 
 
 def test_remove_job(mock_app):
-    handle_message(mock_app, "<@U1234> test do job 10", reaction_count=1)
+    handle_message(mock_app, "<@U1234> test do good job", reaction_count=1)
     jobs = scheduler.get_jobs_of_type("test_good_job")
     assert len(jobs) == 1
     job_id = jobs[0]["id"]

tests/test_dispatcher.py

@@ -105,7 +105,7 @@ def test_job_success():
 def test_job_success_with_parameterised_args():
     log_dir = build_log_dir("test_parameterised_job")
 
-    scheduler.schedule_job("test_parameterised_job", {"path": "poem"}, "channel", TS, 0)
+    scheduler.schedule_job("test_parameterised_job", {"n": "10"}, "channel", TS, 0)
     job = scheduler.reserve_job()
 
     do_job(slack_web_client(), job)

tests/test_job_configs.py

@@ -13,8 +13,8 @@ def test_build_config():
             "restricted": True,
             "default_channel": "#some-channel",
             "jobs": {
-                "good_job": {"run_args_template": "cat [poem]"},
-                "bad_job": {"run_args_template": "dog [poem]"},
+                "good_job": {"run_args_template": "cat {poem}"},
+                "bad_job": {"run_args_template": "dog {poem}"},
             },
             "slack": [
                 {
@@ -27,8 +27,8 @@ def test_build_config():
         },
         "ns2": {
             "jobs": {
-                "good_job": {"run_args_template": "cat [poem]", "report_stdout": True},
-                "bad_job": {"run_args_template": "dog [poem]", "report_success": False},
+                "good_job": {"run_args_template": "cat {poem}", "report_stdout": True},
+                "bad_job": {"run_args_template": "dog {poem}", "report_success": False},
             },
             "slack": [
                 {
@@ -76,25 +76,25 @@ def test_build_config():
     assert config == {
         "jobs": {
             "ns1_good_job": {
-                "run_args_template": "cat [poem]",
+                "run_args_template": "cat {poem}",
                 "report_stdout": False,
                 "report_format": "text",
                 "report_success": True,
             },
             "ns1_bad_job": {
-                "run_args_template": "dog [poem]",
+                "run_args_template": "dog {poem}",
                 "report_stdout": False,
                 "report_format": "text",
                 "report_success": True,
             },
             "ns2_good_job": {
-                "run_args_template": "cat [poem]",
+                "run_args_template": "cat {poem}",
                 "report_stdout": True,
                 "report_format": "text",
                 "report_success": True,
             },
             "ns2_bad_job": {
-                "run_args_template": "dog [poem]",
+                "run_args_template": "dog {poem}",
                 "report_stdout": False,
                 "report_format": "text",
                 "report_success": False,
@@ -303,3 +303,53 @@ def test_build_config_with_invalid_report_format():
     with pytest.raises(RuntimeError) as e:
         build_config(raw_config)
     assert "invalid report_format" in str(e)
+
+
+def test_build_config_with_missing_param_in_slack_command():
+    raw_config = {
+        "workflows": {
+            "restricted": True,
+            "description": "read a poem",
+            "jobs": {
+                "parameterised_job": {
+                    "run_args_template": "python read.py --poem {poem}",
+                },
+            },
+            "slack": [
+                {
+                    "command": "read poem",
+                    "help": "read a poem",
+                    "action": "schedule_job",
+                    "job_type": "parameterised_job",
+                },
+            ],
+        },
+    }
+    with pytest.raises(RuntimeError) as e:
+        build_config(raw_config)
+    assert "does not match the template" in str(e)
+
+
+def test_build_config_with_missing_param_in_run_args_template():
+    raw_config = {
+        "workflows": {
+            "restricted": True,
+            "description": "read a poem",
+            "jobs": {
+                "mismatched_job": {
+                    "run_args_template": "python read_poem.py",
+                },
+            },
+            "slack": [
+                {
+                    "command": "read poem [poem]",
+                    "help": "read a poem",
+                    "action": "schedule_job",
+                    "job_type": "mismatched_job",
+                },
+            ],
+        },
+    }
+    with pytest.raises(RuntimeError) as e:
+        build_config(raw_config)
+    assert "does not match the template" in str(e)