Fix: validate automation config keys

documentation/changelog.rst

@@ -82,7 +82,7 @@ Automations arrived over several pull requests. This is what each of them contri
 * Schedules as well as forecasts: a schedule automation stores what the schedule trigger endpoint accepts, and schedules from each run's own time [see `PR #2293 <https://www.github.com/FlexMeasures/flexmeasures/pull/2293>`_]
 * A scheduler's data source now also records the flex config the scheduler computed under, so a schedule can be traced back to the configuration that produced it, and a schedule automation points at such a data source, the way a forecast automation points at its forecaster's [see `PR #2444 <https://www.github.com/FlexMeasures/flexmeasures/pull/2444>`_]
 * A single automation can now be run on demand, from the CLI (``flexmeasures jobs run-automation``), the API (``POST /assets/<id>/automations/<automation-id>/trigger``) and the asset's *Automations* page (a *Run now* button), which is useful to try out a new automation, to re-run one after fixing what made it fail, or to refresh its results after late input data arrived [see `PR #2460 <https://www.github.com/FlexMeasures/flexmeasures/pull/2460>`_]
-* Automations can be created, edited and deleted in the UI and through new API endpoints (``[POST|PATCH|DELETE] /assets/(id)/automations``), by whoever may add data under the asset, with their recurrence expressed in a selectable IANA timezone, and only involving sensors they can access themselves (read access to the sensors an automation reads, and permission to record data on the sensors it writes to). The *Automations* page shows when each active automation is due to run next, as a clock time in the automation's own timezone, so a recurrence no longer has to be read back from its cron string; the same time is available as ``next_run`` on the automations API endpoints, and is null while an automation is inactive. It is the next scheduled clock time, so it excludes catch-up work still pending, and it follows the dispatcher's daylight-saving rules, taking the first fold of a repeated local time and the first valid minute after a skipped one. Each row's *Run now*, *Edit*, *Activate*/*Deactivate* and *Delete* controls are collected into a single *Actions* menu, leaving *Details* beside it, so a long listing carries two controls per row instead of five [see `PR #2294 <https://www.github.com/FlexMeasures/flexmeasures/pull/2294>`_]
+* Automations can be created, edited and deleted in the UI and through new API endpoints (``[POST|PATCH|DELETE] /assets/(id)/automations``), by whoever may add data under the asset, with their recurrence expressed in a selectable IANA timezone, and only involving sensors they can access themselves (read access to the sensors an automation reads, and permission to record data on the sensors it writes to). The *Automations* page shows when each active automation is due to run next, as a clock time in the automation's own timezone, so a recurrence no longer has to be read back from its cron string; the same time is available as ``next_run`` on the automations API endpoints, and is null while an automation is inactive. It is the next scheduled clock time, so it excludes catch-up work still pending, and it follows the dispatcher's daylight-saving rules, taking the first fold of a repeated local time and the first valid minute after a skipped one. Each row's *Run now*, *Edit*, *Activate*/*Deactivate* and *Delete* controls are collected into a single *Actions* menu, leaving *Details* beside it, so a long listing carries two controls per row instead of five [see `PR #2294 <https://www.github.com/FlexMeasures/flexmeasures/pull/2294>`_ and `PR #2563 <https://www.github.com/FlexMeasures/flexmeasures/pull/2563>`_]
 * An automation's output sensors are checked against its creator's permissions when the automation is created, and the schedules it computes are held to exactly those sensors: a scheduler that returns results for any other sensor is refused, rather than recording on a sensor that was only ever checked for read access [see `PR #2536 <https://www.github.com/FlexMeasures/flexmeasures/pull/2536>`_]
 * Reports as well as forecasts and schedules: a report automation stores report parameters, with its reporter and the reporter's configuration on a data source, and reports on a period resolved afresh on each run, either from ``start-offset`` and ``end-offset`` applied to the run time in the automation's timezone, or since the last successful report ended, while a fixed ``start`` or ``end`` is refused; a report job records only on the sensors the automation was checked against [see `PR #2297 <https://www.github.com/FlexMeasures/flexmeasures/pull/2297>`_]
 * Every automation times its runs the same way: a fixed ``start``, ``end`` or ``prior`` in its parameters is refused, as every run would share that moment, and two of ``start-offset``, ``end-offset`` and ``duration`` describe the period each run covers instead, with the offsets applied to the time the run was due on the automation's own clock, so that, for instance, a schedule automation can plan the whole of the next day [see `PR #2551 <https://www.github.com/FlexMeasures/flexmeasures/pull/2551>`_]

flexmeasures/api/v3_0/assets.py

@@ -1753,7 +1753,9 @@ def post_automation(self, automation_data: dict, id: int, asset: GenericAsset):
                 asset, origin="API", check_permissions=True, **automation_data
             )
         except ValidationError as e:
-            return unprocessable_entity({"parameters": e.messages})
+            # The service names the part of the request each error came from,
+            # so that an error in the config is not reported against the parameters.
+            return unprocessable_entity(e.messages)
         except AutomationSensorsUnknown as e:
             return unprocessable_entity(str(e))
         except ValueError as e:

flexmeasures/api/v3_0/tests/test_automations_api.py

@@ -2,18 +2,27 @@
 
 from __future__ import annotations
 
+import json
 from datetime import datetime, timedelta, timezone
 
 import pytest
 from flask import url_for
-from sqlalchemy import select
+from sqlalchemy import func, select
 
 from flexmeasures.data.models.automations import Automation
 from flexmeasures.data.models.data_sources import DataSource
 from flexmeasures.data.models.generic_assets import GenericAsset
 from flexmeasures.data.models.time_series import Sensor
 
 
+def _with_sensor(parameters: dict, sensor_id: int) -> dict:
+    """Fill in the sensor id that a parametrised payload leaves as "SENSOR".
+
+    The id only exists once the fixtures have run, which is after the parameters are written.
+    """
+    return json.loads(json.dumps(parameters).replace('"SENSOR"', str(sensor_id)))
+
+
 @pytest.fixture(scope="function")
 def add_automations(fresh_db, add_battery_assets_fresh_db):
     battery = add_battery_assets_fresh_db["Test battery"]
@@ -933,6 +942,247 @@ def test_a_forecast_automation_names_the_data_generator_it_runs(
     fresh_db.session.flush()
 
 
+@pytest.mark.parametrize(
+    "automation_type, data_generator, config, parameters",
+    [
+        (
+            "forecasting",
+            "TrainPredictPipeline",
+            {"not-a-config-field": 1},
+            {"sensor": "SENSOR"},
+        ),
+        (
+            "reporting",
+            "PandasReporter",
+            {
+                "required_input": [{"name": "flow"}],
+                "required_output": [{"name": "flow"}],
+                "transformations": [],
+                "not-a-config-field": 1,
+            },
+            {
+                "input": [{"name": "flow", "sensor": "SENSOR"}],
+                "output": [{"name": "flow", "sensor": "SENSOR"}],
+            },
+        ),
+    ],
+)
+@pytest.mark.parametrize(
+    "requesting_user", ["test_prosumer_user_2@seita.nl"], indirect=True
+)
+def test_post_automation_reports_a_config_error_against_the_config(
+    app,
+    fresh_db,
+    add_battery_assets_fresh_db,
+    requesting_user,
+    automation_type,
+    data_generator,
+    config,
+    parameters,
+):
+    """A fault in the data generator's config is reported against `config`, not against `parameters`.
+
+    Both are validated by schemas of the data generator's choosing, so naming the wrong one
+    sends the caller looking for a mistake in a part of the request that is fine.
+    """
+    battery = add_battery_assets_fresh_db["Test battery"]
+    sensor_id = battery.sensors[0].id
+    parameters = _with_sensor(parameters, sensor_id)
+
+    with app.test_client() as client:
+        response = client.post(
+            url_for("AssetAPI:post_automation", id=battery.id),
+            json={
+                "name": "Bad config",
+                "cron": "0 6 * * *",
+                "type": automation_type,
+                "data-generator": data_generator,
+                "config": config,
+                "parameters": parameters,
+            },
+        )
+
+    assert response.status_code == 422, response.json
+    messages = response.json["message"]["json"]
+    assert "not-a-config-field" in str(messages["config"])
+    assert "parameters" not in messages
+
+
+@pytest.mark.parametrize(
+    "automation_type, data_generator, config, parameters",
+    [
+        (
+            "forecasting",
+            "TrainPredictPipeline",
+            {},
+            {"sensor": "SENSOR", "not-a-parameter": 1},
+        ),
+        (
+            "reporting",
+            "PandasReporter",
+            {
+                "required_input": [{"name": "flow"}],
+                "required_output": [{"name": "flow"}],
+                "transformations": [],
+            },
+            {
+                "input": [{"name": "flow", "sensor": "SENSOR"}],
+                "output": [{"name": "flow", "sensor": "SENSOR"}],
+                "not-a-parameter": 1,
+            },
+        ),
+        ("scheduling", None, {}, {"duration": "PT12H", "not-a-parameter": 1}),
+    ],
+)
+@pytest.mark.parametrize(
+    "requesting_user", ["test_prosumer_user_2@seita.nl"], indirect=True
+)
+def test_post_automation_reports_a_parameter_error_against_the_parameters(
+    app,
+    fresh_db,
+    add_battery_assets_fresh_db,
+    requesting_user,
+    automation_type,
+    data_generator,
+    config,
+    parameters,
+):
+    """A fault in the parameters is reported against `parameters`, for every automation type."""
+    battery = add_battery_assets_fresh_db["Test battery"]
+    parameters = _with_sensor(parameters, battery.sensors[0].id)
+    payload = {
+        "name": "Bad parameters",
+        "cron": "0 6 * * *",
+        "type": automation_type,
+        "parameters": parameters,
+    }
+    if data_generator is not None:
+        payload["data-generator"] = data_generator
+        payload["config"] = config
+
+    with app.test_client() as client:
+        response = client.post(
+            url_for("AssetAPI:post_automation", id=battery.id), json=payload
+        )
+
+    assert response.status_code == 422, response.json
+    messages = response.json["message"]["json"]
+    assert "not-a-parameter" in str(messages["parameters"])
+    assert "config" not in messages
+
+
+@pytest.mark.parametrize(
+    "field, value",
+    [
+        ("config", {"model": "CustomLGBM"}),
+        ("data-generator", "TrainPredictPipeline"),
+    ],
+)
+@pytest.mark.parametrize(
+    "requesting_user", ["test_prosumer_user_2@seita.nl"], indirect=True
+)
+def test_post_schedule_automation_rejects_a_data_generator_and_its_config(
+    app, fresh_db, add_battery_assets_fresh_db, requesting_user, field, value
+):
+    """A schedule automation resolves its own scheduler and flex config from the asset.
+
+    Taking either field here would record a choice that nothing goes on to read,
+    so each is refused by name rather than silently ignored.
+    """
+    battery = add_battery_assets_fresh_db["Test battery"]
+    with app.test_client() as client:
+        response = client.post(
+            url_for("AssetAPI:post_automation", id=battery.id),
+            json={
+                "name": "Schedules with an unusable field",
+                "cron": "0 6 * * *",
+                "type": "scheduling",
+                "parameters": {"duration": "PT12H"},
+                field: value,
+            },
+        )
+
+    assert response.status_code == 422, response.json
+    assert field in response.json["message"]["json"]
+    assert (
+        fresh_db.session.execute(
+            select(Automation).filter_by(name="Schedules with an unusable field")
+        ).scalar_one_or_none()
+        is None
+    )
+
+
+@pytest.mark.parametrize(
+    "requesting_user", ["test_prosumer_user_2@seita.nl"], indirect=True
+)
+def test_post_report_automation_without_a_reporter_names_the_field_to_fill_in(
+    app, fresh_db, add_battery_assets_fresh_db, requesting_user
+):
+    """A report automation has to name its reporter, and the error says which field is missing."""
+    battery = add_battery_assets_fresh_db["Test battery"]
+    with app.test_client() as client:
+        response = client.post(
+            url_for("AssetAPI:post_automation", id=battery.id),
+            json={
+                "name": "Reporter-less report",
+                "cron": "0 1 * * *",
+                "type": "reporting",
+                "parameters": {"input": [{"sensor": battery.sensors[0].id}]},
+            },
+        )
+
+    assert response.status_code == 422, response.json
+    assert "A reporter is required" in str(
+        response.json["message"]["json"]["data-generator"]
+    )
+
+
+@pytest.mark.parametrize(
+    "requesting_user", ["test_prosumer_user_2@seita.nl"], indirect=True
+)
+def test_a_refused_automation_leaves_nothing_behind(
+    app, fresh_db, add_battery_assets_fresh_db, requesting_user
+):
+    """A rejected request records neither the automation, nor a data source for its generator, nor an audit log entry."""
+    from flexmeasures.data.models.audit_log import AssetAuditLog
+
+    battery = add_battery_assets_fresh_db["Test battery"]
+    sensor_id = battery.sensors[0].id
+    before = {
+        model: fresh_db.session.scalar(select(func.count()).select_from(model))
+        for model in (Automation, DataSource, AssetAuditLog)
+    }
+    refused = [
+        {
+            "type": "forecasting",
+            "config": {"not-a-config-field": 1},
+            "parameters": {"sensor": sensor_id},
+        },
+        {
+            "type": "forecasting",
+            "parameters": {"sensor": sensor_id, "not-a-parameter": 1},
+        },
+        {
+            "type": "scheduling",
+            "data-generator": "TrainPredictPipeline",
+            "parameters": {"duration": "PT12H"},
+        },
+    ]
+    with app.test_client() as client:
+        for index, payload in enumerate(refused):
+            response = client.post(
+                url_for("AssetAPI:post_automation", id=battery.id),
+                json={"name": f"Refused {index}", "cron": "0 6 * * *", **payload},
+            )
+            assert response.status_code == 422, response.json
+
+    after = {
+        model: fresh_db.session.scalar(select(func.count()).select_from(model))
+        for model in (Automation, DataSource, AssetAuditLog)
+    }
+    assert after == before
+
+
 @pytest.fixture(scope="function")
 def add_automation_on_a_child_asset(fresh_db, add_battery_assets_fresh_db):
     """Put an automation on a sub-asset of the battery, where automations usually live."""

flexmeasures/cli/data_add.py

@@ -1966,8 +1966,9 @@ def add_automation(
             origin="CLI",
         )
     except ValidationError as e:
+        # The messages name the part of the request at fault, which is not always the parameters.
         click.secho(
-            f"Invalid {Automation.RESULT_NOUNS[automation_type]} parameters: {e.messages}",
+            f"Invalid {Automation.RESULT_NOUNS[automation_type]} automation: {e.messages}",
             **MsgStyle.ERROR,
         )
         raise click.Abort()

flexmeasures/cli/tests/test_automations.py

@@ -914,7 +914,11 @@ def test_add_schedule_automation(app, fresh_db, setup_dummy_data, tmp_path):
         ],
     )  # fmt: skip
     assert result.exit_code != 0
-    assert "Invalid schedule parameters" in result.output
+    # The error names the part of the request at fault, which for a schedule automation is always the parameters.
+    assert "Invalid schedule automation" in result.output
+    assert (
+        "{'parameters': {'not-a-trigger-field': ['Unknown field.']}}" in result.output
+    )
 
     # minimal valid parameters (flex config can live on the asset)
     parameters_file.write_text('duration: "PT12H"\n')
@@ -1029,7 +1033,8 @@ def test_add_schedule_automation_rejects_unsupported_durations(
     )
 
     assert result.exit_code != 0
-    assert "Invalid schedule parameters" in result.output
+    assert "Invalid schedule automation" in result.output
+    assert "'parameters'" in result.output
 
 
 def test_add_schedule_automation_rejects_forecast_config(
@@ -1103,7 +1108,45 @@ def test_add_forecast_automation_still_requires_sensor(app, fresh_db, setup_dumm
     )
 
     assert result.exit_code != 0
-    assert "Invalid forecast parameters" in result.output
+    assert "Invalid forecast automation" in result.output
+    assert (
+        "{'parameters': {'sensor': ['Missing data for required field.']}}"
+        in result.output
+    )
+
+
+def test_add_forecast_automation_reports_a_config_error_against_the_config(
+    app, fresh_db, setup_dummy_data, tmp_path
+):
+    """A fault in the forecaster's config is reported against the config, rather than against the parameters.
+
+    Both are validated by schemas of the data generator's choosing, so naming the wrong one
+    sends the user looking for a mistake in a part of the command that is fine.
+    """
+    from flexmeasures.cli.data_add import add_automation
+
+    config_file = tmp_path / "config.yml"
+    config_file.write_text("not-a-config-field: 1\n")
+    result = app.test_cli_runner().invoke(
+        add_automation,
+        [
+            "--asset", "1",
+            "--name", "Bad forecaster config",
+            "--cron", "0 6 * * *",
+            "--sensor", str(setup_dummy_data[0]),
+            "--config", str(config_file),
+        ],
+    )  # fmt: skip
+
+    assert result.exit_code != 0
+    assert "Invalid forecast automation" in result.output
+    assert "{'config': {'not-a-config-field': ['Unknown field.']}}" in result.output
+    assert (
+        fresh_db.session.execute(
+            select(Automation).filter_by(name="Bad forecaster config")
+        ).scalar_one_or_none()
+        is None
+    )
 
 
 @pytest.mark.parametrize("is_dst", (True, False))