Fix: validate automation config keys - #2563
Draft
BelhsanHmida wants to merge 4 commits into
Draft
BelhsanHmida wants to merge 4 commits into
BelhsanHmida wants to merge 4 commits into
Conversation
… is at fault Context: - Issue #2498: a mistake in a data generator's config was reported against "parameters", sending the caller looking in a part of the request that was fine. - A scheduling automation accepted a config and a data-generator with 201 Created, and silently ignored both. - A report automation without a reporter raised a bare string, naming no field at all. Change: - Added an `errors_reported_for(section)` context manager, which re-raises a ValidationError keyed by the section it came from. - Wrapped each raise site with the section it belongs to: the window check, the forecast and report parameter loads, the schedule trigger load and the schedule sensor resolution report against "parameters"; the forecaster and reporter setups report against "config". - A scheduling automation now refuses a non-empty config or data-generator by name, in `refuse_fields_a_schedule_automation_cannot_use`: its scheduler and flex config follow from the asset. - A missing reporter is reported against "data-generator", and an unsupported automation type against "type". - The create endpoint returns the service's messages as they are, rather than re-wrapping every one of them as {"parameters": ...}. - The CLI reads "Invalid <noun> automation:", since the fault is not always in the parameters, and the keyed messages now say where it is. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - The create endpoint and `flexmeasures add automation` now key each validation error by the part of the request it came from (issue #2498). - Seven CLI tests asserted the old wording, "Invalid <noun> parameters", which contradicts itself once the messages can name the config instead. Change: - Added API tests: a config error is keyed "config" and a parameter error "parameters" (forecasting and reporting, plus scheduling for the parameters); a scheduling automation refuses "config" and "data-generator" by name; a report automation without a reporter is keyed "data-generator"; and three refused requests leave the automation, data source and audit log counts unchanged. - Added a CLI test for a forecaster config error keyed "config". - Updated the CLI tests that asserted the old wording to assert the new one, plus the key the message is now reported under. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
…t fault Context: - PR #2563 changes which key an automation's validation errors are reported under, which an API client reads. Change: - Added an entry to the "Automations, in detail" section of the main changelog, rather than to Bugfixes, since automations have not shipped in a release yet. - Added a v3.0-39 section to the API change log, saying explicitly that this is not a breaking change, as the automation endpoints are not part of a release. - Numbered v3.0-39 against PR #2554's v3.0-38, which merges first. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Context: - PR #2542 landed on main, and adds `_stored_sensor_id` to `flexmeasures/data/services/automations.py` in the same place this branch adds `refuse_fields_a_schedule_automation_cannot_use` and `errors_reported_for`. Change: - Resolved the add/add conflict by keeping both sides: this branch's two helpers, then main's `_stored_sensor_id`, which main placed just above `_prepare_forecast_automation`. - Everything else merged cleanly, including `_prepare_forecast_automation`, where main's SensorReference unwrap now follows this branch's wrapped parameter load. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Documentation build overview
5 files changed ·
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
configwas reported againstparameters, for forecast and report automations alike, sending the caller looking for a mistake in a part of the request that was fine. The create endpoint wrapped everyValidationErrorfrom the service as{"parameters": ...}, discarding which schema had failed.schedulingautomation acceptedconfiganddata-generatorwith201 Createdand silently ignored both. It now refuses each by name with a422: a schedule automation's scheduler, and the flex config it runs under, follow from the asset and its flex context, so accepting either would record a choice that nothing goes on to read.data-generator.type.errors_reported_for(section)context manager inflexmeasures/data/services/automations.py, which re-raises aValidationErrorkeyed by the section it came from, and wrapped each raise site with the section it belongs to: the window check, the forecast and report parameter loads, the forecast window resolution, the schedule trigger message and the schedule sensor resolution report againstparameters; the forecaster and reporter setups report againstconfig.parameters: the unknown-field error fromAssetTriggerSchema(raised insideresolve_schedule_automation_sensors, whose docstring already promised the caller would report it against the parameters), the bare messages fromvalidate_automation_window, and the offset and window errors fromresolve_automation_window, which was evaluated as an argument and so sat outside the wrapper.flexmeasures add automationnow reports "Invalid automation:" instead of "Invalid parameters:", since the fault is not always in the parameters, and the keyed messages now say where it is.refuse_fields_a_schedule_automation_cannot_use, so thatcreate_automationstays within the C901 complexity budget.This is not a breaking change. A client parsing
message.json.parameterswill find config errors undermessage.json.config, but the automation endpoints landed inv3.0-37andv3.0-38, both inside the unreleasedv1.1.0, so no released client can be parsing this shape.The issue predates #2297 and #2536 and is partly out of date: unknown
parameterskeys were already rejected with a field-specific422, with nothing persisted. Criterion 6 is covered by a new test rather than by new code.documentation/changelog.rstLook & Feel
Before, a forecast automation with a bad config key:
{"message": {"json": {"parameters": {"not-a-config-field": ["Unknown field."]}}}}After:
{"message": {"json": {"config": {"not-a-config-field": ["Unknown field."]}}}}Before, a schedule automation sent a
configand adata-generator:After:
{"message": {"json": { "config": ["A schedule automation configures no data generator of its own: its scheduler and flex config follow from the asset and its flex context."], "data-generator": ["A schedule automation does not choose a data generator: its scheduler follows from the asset."] }}}Before, a report automation with no reporter:
{"message": {"json": {"parameters": ["A reporter is required for report automations (e.g. PandasReporter)."]}}}After:
{"message": {"json": {"data-generator": ["A reporter is required for report automations (e.g. PandasReporter)."]}}}On the command line:
How to test
New tests:
test_post_automation_reports_a_config_error_against_the_config— forecasting and reporting; the error is keyedconfigand noparameterskey is present.test_post_automation_reports_a_parameter_error_against_the_parameters— forecasting, reporting and scheduling; the error is keyedparametersand noconfigkey is present.test_post_schedule_automation_rejects_a_data_generator_and_its_config— each field is refused by name, and nothing is persisted.test_post_report_automation_without_a_reporter_names_the_field_to_fill_in— keyeddata-generator.test_a_refused_automation_leaves_nothing_behind— three failing requests leave theAutomation,DataSourceandAssetAuditLogrow counts unchanged.test_add_forecast_automation_reports_a_config_error_against_the_config— the CLI equivalent.Updated: seven CLI tests asserted the old wording, "Invalid parameters", which contradicts itself once the messages can name the config instead. They now assert the new wording and the key the message is reported under.
Each new test was proven to fail before it was called done:
{"parameters": e.messages}againerrors_reported_for("parameters")renamed to"mislabelled"<noun>parameters"422test_a_refused_automation_leaves_nothing_behindEach break was restored and the suite re-run.
Full suites, run one at a time:
flexmeasures/api/v3_0/tests/518 passed;flexmeasures/cli/tests/234 passed, 1 xfailed;flexmeasures/data/tests/389 passed.Further Improvements
Merge order: #2554 should merge first. Main's "New automation" form shows the Data generator and Config fields for every automation type; #2554 gates them so that a schedule automation sends neither. Without that gating, this PR's new rejection turns leftover text in a hidden field into a
422as soon as the user picksscheduling.The API changelog section here is numbered against #2554's
v3.0-38. If this PR lands first, renumber it tov3.0-38.Related Items
Closes #2498.
Sign-off