diff --git a/documentation/changelog.rst b/documentation/changelog.rst index b73682eece..936a1c0225 100644 --- a/documentation/changelog.rst +++ b/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 `_] * 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 `_] * A single automation can now be run on demand, from the CLI (``flexmeasures jobs run-automation``), the API (``POST /assets//automations//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 `_] -* 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 `_] +* 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 `_ and `PR #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 `_] * 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 `_] * 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 `_] diff --git a/flexmeasures/api/v3_0/assets.py b/flexmeasures/api/v3_0/assets.py index 77e0c53487..c5bce14e9d 100644 --- a/flexmeasures/api/v3_0/assets.py +++ b/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: diff --git a/flexmeasures/api/v3_0/tests/test_automations_api.py b/flexmeasures/api/v3_0/tests/test_automations_api.py index 3e63b8bb87..96b67672d6 100644 --- a/flexmeasures/api/v3_0/tests/test_automations_api.py +++ b/flexmeasures/api/v3_0/tests/test_automations_api.py @@ -2,11 +2,12 @@ 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 @@ -14,6 +15,14 @@ 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.""" diff --git a/flexmeasures/cli/data_add.py b/flexmeasures/cli/data_add.py index 84bd333831..726df1f9b8 100755 --- a/flexmeasures/cli/data_add.py +++ b/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() diff --git a/flexmeasures/cli/tests/test_automations.py b/flexmeasures/cli/tests/test_automations.py index de6b05d706..bd5ddea0ba 100644 --- a/flexmeasures/cli/tests/test_automations.py +++ b/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)) diff --git a/flexmeasures/data/services/automations.py b/flexmeasures/data/services/automations.py index 120e2fcf5d..84a3065203 100644 --- a/flexmeasures/data/services/automations.py +++ b/flexmeasures/data/services/automations.py @@ -4,6 +4,7 @@ from __future__ import annotations +from contextlib import contextmanager from copy import copy, deepcopy from dataclasses import dataclass from datetime import datetime, timedelta, timezone @@ -1116,6 +1117,45 @@ def get_automation_job_stats(automation: Automation) -> dict[str, int]: return counts +def refuse_fields_a_schedule_automation_cannot_use( + config: dict | None, generator_class: str | None +) -> None: + """Refuse a schedule automation's config or data generator, which it has no use for. + + A schedule automation's scheduler, and the flex config it runs under, follow from the asset and its flex context. + Accepting either field here would record a choice that nothing goes on to read, + leaving the caller to believe it applied. + + :raises marshmallow.ValidationError: keyed by the field(s) given, if either was. + """ + unsupported = {} + if config: + unsupported["config"] = [ + "A schedule automation configures no data generator of its own:" + " its scheduler and flex config follow from the asset and its flex context." + ] + if generator_class: + unsupported["data-generator"] = [ + "A schedule automation does not choose a data generator:" + " its scheduler follows from the asset." + ] + if unsupported: + raise ValidationError(unsupported) + + +@contextmanager +def errors_reported_for(section: str): + """Name the part of the request a validation error came from, so the caller can say which one to fix. + + Both `config` and `parameters` are validated against schemas of the data generator's choosing, + and either can raise. Without this, one is indistinguishable from the other by the time it surfaces. + """ + try: + yield + except ValidationError as error: + raise ValidationError({section: error.messages}) from error + + def _stored_sensor_id(sensor_reference: Any) -> int | None: """Return the sensor ID from a stored automation parameter naming a sensor. @@ -1143,7 +1183,8 @@ def _prepare_forecast_automation( from flexmeasures.data.services.data_sources import get_data_generator warnings = [] - deserialized_parameters = ForecasterParametersSchema().load(parameters) + with errors_reported_for("parameters"): + deserialized_parameters = ForecasterParametersSchema().load(parameters) sensor = deserialized_parameters.get("sensor") # A target may be given as a source-filtered reference, whose filters say which beliefs to train on, and not which sensor is meant. if isinstance(sensor, SensorReference): @@ -1153,13 +1194,14 @@ def _prepare_forecast_automation( f"The sensor to forecast ({sensor.id}) does not belong to asset {asset.id}." ) model = generator_class or "TrainPredictPipeline" - forecaster = get_data_generator( - source=source, - model=model, - config=config or {}, - save_config=True, - data_generator_type=Forecaster, - ) + with errors_reported_for("config"): + forecaster = get_data_generator( + source=source, + model=model, + config=config or {}, + save_config=True, + data_generator_type=Forecaster, + ) if forecaster is None: # With a source, the class and its config come from the source, so the source is what failed. if source is not None: @@ -1177,22 +1219,25 @@ def _prepare_report_automation( source, ) -> tuple[Reporter, dict, list[str]]: """Validate report automation parameters without creating a data source.""" - from marshmallow import ValidationError - from flexmeasures.data.services.data_sources import get_data_generator warnings: list[str] = [] if generator_class is None and source is None: raise ValidationError( - "A reporter is required for report automations (e.g. PandasReporter)." + { + "data-generator": [ + "A reporter is required for report automations (e.g. PandasReporter)." + ] + } + ) + with errors_reported_for("config"): + reporter = get_data_generator( + source=source, + model=generator_class, + config=config or {}, + save_config=True, + data_generator_type=Reporter, ) - reporter = get_data_generator( - source=source, - model=generator_class, - config=config or {}, - save_config=True, - data_generator_type=Reporter, - ) if reporter is None: # With a source, the class and its config come from the source, so the source is what failed. if source is not None: @@ -1200,9 +1245,10 @@ def _prepare_report_automation( raise ValueError(f"Could not set up reporter '{generator_class}'.") # Validate with the chosen reporter's own parameters schema, # which may extend the base ReporterParametersSchema. - deserialized_parameters = reporter._parameters_schema.load( - prepare_report_parameters(parameters, cronstr, automation_timezone) - ) + with errors_reported_for("parameters"): + deserialized_parameters = reporter._parameters_schema.load( + prepare_report_parameters(parameters, cronstr, automation_timezone) + ) return reporter, deserialized_parameters, warnings @@ -1234,8 +1280,6 @@ def create_automation( :raises werkzeug.exceptions.Forbidden: if a sensor is not accessible to the user. :returns: the automation and a list of warnings. """ - from marshmallow import ValidationError - from flexmeasures.data.models.audit_log import AssetAuditLog parameters = parameters or {} @@ -1248,12 +1292,17 @@ def create_automation( if automation_type in Automation.SUPPORTED_TYPES: # An automation runs again and again, so a moment fixed in its parameters would be shared by every run. refuse_fixed_moments(parameters, automation_type) - validate_automation_window(parameters, automation_type) + with errors_reported_for("parameters"): + validate_automation_window(parameters, automation_type) if automation_type == "forecasting": + with errors_reported_for("parameters"): + forecast_window = resolve_automation_window( + parameters, automation_type, timezone + ) forecaster, deserialized_parameters, forecast_warnings = ( _prepare_forecast_automation( asset, - resolve_automation_window(parameters, automation_type, timezone), + forecast_window, generator_class, config, source, @@ -1276,12 +1325,16 @@ def create_automation( find_momentary_flex_config_fields, ) + refuse_fields_a_schedule_automation_cannot_use(config, generator_class) + # The flex config has to describe the site and its devices, rather than one moment: # the automation computes a fresh schedule on every run, # so a value tied to a fixed moment would be stale on the next one. - momentary_fields = find_momentary_flex_config_fields( - prepare_schedule_trigger_message(dict(parameters), asset.id, timezone) - ) + with errors_reported_for("parameters"): + trigger_message = prepare_schedule_trigger_message( + dict(parameters), asset.id, timezone + ) + momentary_fields = find_momentary_flex_config_fields(trigger_message) if momentary_fields: raise RecurringAutomationFixesAMoment( f"{flexmeasures_inflection.join_words_into_a_list(momentary_fields)} fixes a moment in time," @@ -1292,9 +1345,10 @@ def create_automation( # A schedule is recorded on the sensors that the scheduler returns its results for, # and reads whatever other sensors the flex-model and flex-context refer to, # such as price sensors and the sensors of inflexible devices. - schedule_sensors = resolve_schedule_automation_sensors( - parameters, asset.id, timezone - ) + with errors_reported_for("parameters"): + schedule_sensors = resolve_schedule_automation_sensors( + parameters, asset.id, timezone + ) input_sensors = schedule_sensors["input_sensors"] output_sensors = schedule_sensors["output_sensors"] elif automation_type == "reporting": @@ -1310,7 +1364,11 @@ def create_automation( output_sensors = report_sensors["output_sensors"] else: raise ValidationError( - f"Automation type '{automation_type}' is not supported (supported types: {Automation.SUPPORTED_TYPES})." + { + "type": [ + f"Automation type '{automation_type}' is not supported (supported types: {Automation.SUPPORTED_TYPES})." + ] + } ) if check_permissions: