Config param placeholders

airflow_src/dags/impl/processor_impl.py

@@ -42,6 +42,11 @@
 from metrics.metrics_calculator import calc_metrics
 from mongoengine import DoesNotExist
 
+from shared.config_params import (
+    ConfigParamPlaceholders,
+    substitute_dummy_values,
+    substitute_placeholders,
+)
 from shared.db.interface import (
     add_metrics_to_raw_file,
     get_project_settings,
@@ -192,7 +197,7 @@ def _create_quanting_env(
         raw_file.project_id,
     )
 
-    custom_command = (  # TODO: remove in favour of software_path and params
+    custom_command = (
         _prepare_custom_command(settings, substituted_params)
         # all non-alphadia softwares are treated as 'custom command'
         if settings.software_type not in [SoftwareTypes.ALPHADIA]
@@ -204,9 +209,9 @@ def _create_quanting_env(
         QuantingEnv.SETTINGS_PATH: str(settings_path),
         QuantingEnv.OUTPUT_PATH: str(output_path),
         QuantingEnv.RELATIVE_OUTPUT_PATH: str(relative_output_path),
-        QuantingEnv.SPECLIB_FILE_NAME: settings.speclib_file_name,  # TODO: construct path here
-        QuantingEnv.FASTA_FILE_NAME: settings.fasta_file_name,  # TODO: construct path here
-        QuantingEnv.CONFIG_FILE_NAME: settings.config_file_name,  # TODO: construct path here
+        QuantingEnv.SPECLIB_FILE_NAME: settings.speclib_file_name,
+        QuantingEnv.FASTA_FILE_NAME: settings.fasta_file_name,
+        QuantingEnv.CONFIG_FILE_NAME: settings.config_file_name,
         QuantingEnv.SOFTWARE: settings.software,
         QuantingEnv.SOFTWARE_TYPE: settings.software_type,
         QuantingEnv.METRICS_TYPE: settings.metrics_type,
@@ -247,22 +252,19 @@ def _substitute_config_params(  # noqa: PLR0913 Too many arguments
     if settings.config_params is None:
         return ""
 
-    substituted_params = settings.config_params
-    replacements = {
-        # mind the order of replacements here (LONGER placeholders first, e.g. RAW_FILE_PATH before RELATIVE_RAW_FILE_PATH)
-        "RELATIVE_RAW_FILE_PATH": relative_raw_file_path,
-        "RAW_FILE_PATH": raw_file_path,
-        "RAW_FILE_ID": raw_file_id,
-        "SETTINGS_PATH": settings_path,
-        "RELATIVE_OUTPUT_PATH": relative_output_path,
-        "OUTPUT_PATH": output_path,
-        "NUM_THREADS": num_threads,
-        "PROJECT_ID": project_id,
-    }
-    for placeholder, new_value in replacements.items():
-        substituted_params = substituted_params.replace(placeholder, str(new_value))
-
-    return substituted_params
+    return substitute_placeholders(
+        settings.config_params,
+        {
+            ConfigParamPlaceholders.PROJECT_ID: project_id,
+            ConfigParamPlaceholders.RAW_FILE_ID: raw_file_id,
+            ConfigParamPlaceholders.RAW_FILE_PATH: str(raw_file_path),
+            ConfigParamPlaceholders.RELATIVE_RAW_FILE_PATH: str(relative_raw_file_path),
+            ConfigParamPlaceholders.SETTINGS_PATH: str(settings_path),
+            ConfigParamPlaceholders.OUTPUT_PATH: str(output_path),
+            ConfigParamPlaceholders.RELATIVE_OUTPUT_PATH: str(relative_output_path),
+            ConfigParamPlaceholders.NUM_THREADS: str(num_threads),
+        },
+    )
 
 
 def _prepare_custom_command(settings: Settings, substituted_params: str) -> str:
@@ -319,7 +321,9 @@ def _check_content(
             )
     if settings.config_params:
         errors.extend(
-            check_for_malicious_content(settings.config_params, allow_spaces=True)
+            check_for_malicious_content(
+                substitute_dummy_values(settings.config_params), allow_spaces=True
+            )
         )
 
     return errors

airflow_src/tests/dags/impl/test_processor_impl.py

@@ -137,7 +137,7 @@ def test_create_quanting_env_custom_software(
     mock_settings.speclib_file_name = "some_speclib_file_name"
     mock_settings.fasta_file_name = "some_fasta_file_name"
     mock_settings.config_file_name = ""
-    mock_settings.config_params = "--qvalue 0.01 --f RAW_FILE_PATH --lib SETTINGS_PATH/some_speclib_file_name --out OUTPUT_PATH --fasta SETTINGS_PATH/some_fasta_file_name --threads NUM_THREADS --some_param RELATIVE_RAW_FILE_PATH --some_param2 RELATIVE_OUTPUT_PATH"
+    mock_settings.config_params = "--qvalue 0.01 --f {RAW_FILE_PATH} --lib {SETTINGS_PATH}/some_speclib_file_name --out {OUTPUT_PATH} --fasta {SETTINGS_PATH}/some_fasta_file_name --threads {NUM_THREADS} --some_param {RELATIVE_RAW_FILE_PATH} --some_param2 {RELATIVE_OUTPUT_PATH}"
     mock_settings.software = "custom1.2.3"
     mock_settings.software_type = "custom"
     mock_settings.metrics_type = "custom"
@@ -352,6 +352,33 @@ def test_check_content_rejects_malicious_resolved_config_params() -> None:
     assert len(errors) == 1
 
 
+def test_check_content_allows_placeholders_in_unresolved_config_params() -> None:
+    """Test that the placeholder braces of the unresolved config params pass validation."""
+    quanting_env = {
+        QuantingEnv.CONFIG_PARAMS: "--f /pool/backup/f.raw --threads 8",
+    }
+
+    errors = _check_content(
+        quanting_env,
+        MagicMock(config_params="--f {RAW_FILE_PATH} --threads {NUM_THREADS}"),
+    )
+
+    assert errors == []
+
+
+def test_check_content_rejects_unknown_placeholder() -> None:
+    """Test that a misspelled placeholder in the unresolved config params is rejected."""
+    quanting_env = {
+        QuantingEnv.CONFIG_PARAMS: "--f /pool/backup/f.raw",
+    }
+
+    errors = _check_content(
+        quanting_env, MagicMock(config_params="--f {RAW_FILE_PAHT}")
+    )
+
+    assert len(errors) == 1
+
+
 def test_check_content_allows_image_name_in_software_field() -> None:
     """Test that a docker image name in the software field is accepted."""
     quanting_env = {
@@ -670,7 +697,7 @@ def test_create_quanting_env_with_suffix(
     mock_internal_output_path.return_value = Path("/opt/airflow/mounts/output")
 
     mock_settings = MagicMock(
-        software_type="custom", config_params="--out OUTPUT_PATH", num_threads=8
+        software_type="custom", config_params="--out {OUTPUT_PATH}", num_threads=8
     )
     mock_settings.name = "test_settings"
 

airflow_src/tests/plugins/jobs/test_docker_job_handler.py

@@ -122,7 +122,7 @@ def test_start_job_should_sanitize_container_name(
         sample_environment[QuantingEnv.RAW_FILE_ID] = "raw+file+1.raw"
 
         # when
-        handler.start_job("ignored.sh", sample_environment, "2024_07")
+        handler.start_job(sample_environment)
 
         # then
         _, kwargs = handler._client.containers.run.call_args

docs/deployment.md

@@ -355,7 +355,7 @@ monitoring tasks for all engines, not only for Slurm.
     - execution engine `docker`,
     - `software` set to the image name, e.g. `alphakraken-msqc`,
     - `config_params` set to the arguments for the image, with the usual placeholders, e.g.
-      `RAW_FILE_PATH OUTPUT_PATH NUM_THREADS` for the msqc image. They may be left empty if the
+      `{RAW_FILE_PATH} {OUTPUT_PATH} {NUM_THREADS}` for the msqc image. They may be left empty if the
       image's entrypoint reads the environment variables instead (the msqc image supports both).
    - memory and cpus are taken from the slurm settings
 

shared/config_params.py

@@ -0,0 +1,72 @@
+"""Placeholders that can be used in the `config_params` of a settings entry."""
+
+import re
+
+from shared.keys import ConstantsClass
+
+# stand-in for a placeholder value, used where the real values are not known yet
+PLACEHOLDER_DUMMY_VALUE = "dummy"
+
+UNKNOWN_PLACEHOLDER_ERROR = "Unknown placeholder"
+
+
+class ConfigParamPlaceholders(metaclass=ConstantsClass):
+    """Names of the placeholders, without the surrounding braces."""
+
+    PROJECT_ID: str = "PROJECT_ID"
+    RAW_FILE_ID: str = "RAW_FILE_ID"
+    RAW_FILE_PATH: str = "RAW_FILE_PATH"
+    RELATIVE_RAW_FILE_PATH: str = "RELATIVE_RAW_FILE_PATH"
+    SETTINGS_PATH: str = "SETTINGS_PATH"
+    OUTPUT_PATH: str = "OUTPUT_PATH"
+    RELATIVE_OUTPUT_PATH: str = "RELATIVE_OUTPUT_PATH"
+    NUM_THREADS: str = "NUM_THREADS"
+
+
+PLACEHOLDER_DESCRIPTIONS: dict[str, str] = {
+    ConfigParamPlaceholders.PROJECT_ID: "project id",
+    ConfigParamPlaceholders.RAW_FILE_ID: "name of the raw file",
+    ConfigParamPlaceholders.RAW_FILE_PATH: "absolute path of the raw file",
+    ConfigParamPlaceholders.RELATIVE_RAW_FILE_PATH: "path of the raw file relative to `locations.backup.absolute_path` in alphakraken.yaml",
+    ConfigParamPlaceholders.SETTINGS_PATH: "absolute path of the settings directory",
+    ConfigParamPlaceholders.OUTPUT_PATH: "absolute path of the output directory",
+    ConfigParamPlaceholders.RELATIVE_OUTPUT_PATH: "path of the output directory relative to `locations.output.absolute_path` in alphakraken.yaml",
+    ConfigParamPlaceholders.NUM_THREADS: "number of threads",
+}
+
+
+_KNOWN_PLACEHOLDER_PATTERN = re.compile(
+    rf"\{{({'|'.join(ConfigParamPlaceholders.get_values())})\}}"
+)
+_BRACED_TOKEN_PATTERN = re.compile(r"\{([^{}]*)\}")
+
+
+def substitute_placeholders(config_params: str, values: dict[str, str]) -> str:
+    """Replace each `{PLACEHOLDER}` in `config_params` by the given value."""
+    # single pass, so that a substituted value containing a placeholder is not expanded again
+    return _KNOWN_PLACEHOLDER_PATTERN.sub(
+        lambda match: values.get(match.group(1), match.group()), config_params
+    )
+
+
+def substitute_dummy_values(config_params: str) -> str:
+    """Replace all known placeholders by a dummy value, to enable validating unresolved `config_params`."""
+    return substitute_placeholders(
+        config_params,
+        dict.fromkeys(ConfigParamPlaceholders.get_values(), PLACEHOLDER_DUMMY_VALUE),
+    )
+
+
+def check_for_unknown_placeholders(config_params: str) -> list[str]:
+    """Validate that `config_params` contains no braced token besides the known placeholders.
+
+    Returns:
+        list[str]: List of validation error messages (empty if valid)
+
+    """
+    known_placeholders = ConfigParamPlaceholders.get_values()
+    return [
+        f"{UNKNOWN_PLACEHOLDER_ERROR}: {match.group()}"
+        for match in _BRACED_TOKEN_PATTERN.finditer(config_params)
+        if match.group(1) not in known_placeholders
+    ]

shared/tests/test_config_params.py

@@ -0,0 +1,111 @@
+"""Unit tests for the config_params placeholder substitution."""
+
+from shared.config_params import (
+    PLACEHOLDER_DESCRIPTIONS,
+    PLACEHOLDER_DUMMY_VALUE,
+    ConfigParamPlaceholders,
+    check_for_unknown_placeholders,
+    substitute_dummy_values,
+    substitute_placeholders,
+)
+from shared.validation import check_for_malicious_content
+
+
+def test_all_placeholders_have_a_description() -> None:
+    """Test that the webapp help text covers every placeholder."""
+    assert set(PLACEHOLDER_DESCRIPTIONS) == set(ConfigParamPlaceholders.get_values())
+
+
+def test_substitute_placeholders() -> None:
+    """Test that braced placeholders are replaced."""
+    result = substitute_placeholders(
+        "--f {RAW_FILE_PATH} --threads {NUM_THREADS}",
+        {
+            ConfigParamPlaceholders.RAW_FILE_PATH: "/backup/f.raw",
+            ConfigParamPlaceholders.NUM_THREADS: "8",
+        },
+    )
+
+    assert result == "--f /backup/f.raw --threads 8"
+
+
+def test_substitute_placeholders_ignores_unbraced_names() -> None:
+    """Test that a bare placeholder name is left untouched."""
+    result = substitute_placeholders(
+        "--f RAW_FILE_PATH",
+        {ConfigParamPlaceholders.RAW_FILE_PATH: "/backup/f.raw"},
+    )
+
+    assert result == "--f RAW_FILE_PATH"
+
+
+def test_substitute_placeholders_is_order_independent() -> None:
+    """Test that a placeholder that is the suffix of another one is not substituted into it."""
+    values = {
+        ConfigParamPlaceholders.RAW_FILE_PATH: "/backup/f.raw",
+        ConfigParamPlaceholders.RELATIVE_RAW_FILE_PATH: "instrument1/f.raw",
+    }
+
+    result = substitute_placeholders("{RAW_FILE_PATH} {RELATIVE_RAW_FILE_PATH}", values)
+    result_reversed = substitute_placeholders(
+        "{RAW_FILE_PATH} {RELATIVE_RAW_FILE_PATH}", dict(reversed(values.items()))
+    )
+
+    assert result == "/backup/f.raw instrument1/f.raw"
+    assert result_reversed == result
+
+
+def test_substitute_placeholders_does_not_expand_substituted_values() -> None:
+    """Test that a placeholder contained in a substituted value is not expanded again."""
+    result = substitute_placeholders(
+        "{OUTPUT_PATH} {NUM_THREADS}",
+        {
+            ConfigParamPlaceholders.OUTPUT_PATH: "/out/{NUM_THREADS}",
+            ConfigParamPlaceholders.NUM_THREADS: "8",
+        },
+    )
+
+    assert result == "/out/{NUM_THREADS} 8"
+
+
+def test_substitute_dummy_values() -> None:
+    """Test that all known placeholders are replaced by the dummy value."""
+    config_params = " ".join(
+        f"{{{placeholder}}}" for placeholder in ConfigParamPlaceholders.get_values()
+    )
+
+    result = substitute_dummy_values(config_params)
+
+    assert result == " ".join(
+        [PLACEHOLDER_DUMMY_VALUE] * len(ConfigParamPlaceholders.get_values())
+    )
+
+
+def test_dummy_substituted_params_pass_validation() -> None:
+    """Test that config params using placeholders are accepted by the validation."""
+    config_params = "--f {RAW_FILE_PATH} --lib {SETTINGS_PATH}/library.speclib --threads {NUM_THREADS}"
+
+    errors = check_for_malicious_content(
+        substitute_dummy_values(config_params), allow_spaces=True
+    )
+
+    assert errors == []
+
+
+def test_check_for_unknown_placeholders_accepts_known_ones() -> None:
+    """Test that all known placeholders pass the check."""
+    config_params = " ".join(
+        f"{{{placeholder}}}" for placeholder in ConfigParamPlaceholders.get_values()
+    )
+
+    assert check_for_unknown_placeholders(config_params) == []
+
+
+def test_check_for_unknown_placeholders_rejects_misspelled_one() -> None:
+    """Test that a misspelled placeholder is reported by name."""
+    errors = check_for_unknown_placeholders(
+        "--f {RAW_FILE_PAHT} --threads {NUM_THREADS}"
+    )
+
+    assert len(errors) == 1
+    assert "{RAW_FILE_PAHT}" in errors[0]