Repository navigation
Fix: validate automation config keys - #2563
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
32 files changed ·
|
Context: - Main moved 8 commits ahead, among them PR #2554, which this branch waited on: it gates the "New automation" form's data generator and config fields by type. Change: - Resolved three add/add conflicts by keeping both sides. - documentation/api/change_log.rst: this branch's v3.0-39 above #2554's v3.0-38, which is the numbering this branch had already assumed. - documentation/changelog.rst: both entries in "Automations, in detail", #2554's first, following the order they merged in. - flexmeasures/api/v3_0/tests/test_automations_api.py: both sides appended their own tests, with no overlapping names, so both sets are kept. - No production code conflicted. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
|
Tested this locally, Each automation type now names the part of the request at fault. A bad key in a forecaster's or a reporter's config comes back under A schedule automation refuses a The CLI reports the same keys, and Permissions still come first: a bad config on an asset I may not write to answers 403, with nothing in the response about the asset. And a refusal leaves nothing behind the automation, data source and audit log counts are unchanged across the failing requests. |
Flix6x
left a comment
There was a problem hiding this comment.
I only request to keep changelog verbosity at bay.
Automations are unreleased, so drop this PR's own bullet from "Automations, in detail", append its number to the bullet that already covers creating automations, and drop the v3.0-39 API change log section. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com>
Main's automation config validation (#2563) names the part of a request each error came from, which the plugin payload loader does too, so the API passes those messages on as they are. A plugin-defined type is not in the static mapping of result nouns, so the refusal of a fixed moment, and the CLI's message about an invalid automation, ask the handler what its automation produces. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl>
The merge of main brought in the entry PR #2563 had extended, beside this branch's own edit of the same line. Neither conflicted with the other, so both survived: the section carried the same 900-character bullet twice, differing only in whether it mentioned Copy and which pull requests it cited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl>
* api/v3_0/sources: search the data sources listing, and narrow it to one type Context: - The listing returned every data source the caller may read, with no way to search it or to ask for one kind. A picker offering a choice of forecasters therefore had to fetch them all and sift through them in the browser. Change: - Add a `filter` of space-separated search terms, matched against a source's name, its model and its id prefix, the way the sensors listing already searches, and a `type`, which narrows the listing to one source type. - Move the rule about which sources a user may read into the data-sources service, as `get_readable_source_account_ids` and `user_may_read_source`, so that it can be applied outside this endpoint without being restated. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * api/v3_0/assets: let an automation name the data source it computes under Context: - A data source stores a data generator and the configuration it runs under, and the results computed under it are recorded on it. `flexmeasures add automation` has taken `--source` since the command was written, so several automations can share one generator and one lineage of data, but the API could only be handed a class and a config, which reaches an existing source by accident, when what it builds happens to hash to the same attributes. Change: - Accept a `source` on the creation endpoint, deserialized to the DataSource the service already knows how to take. It cannot be combined with `data-generator` or `config`, which the source itself determines, and a schedule automation cannot name one, since it re-resolves its source from the asset and the flex config on every run and would move off the named one at the next run. - Require that the caller may read the source, by the same rule the sources listing applies. A source is otherwise a way to compute under, and record on, a generator configuration belonging to another organisation. The CLI runs without a user and stays trusted, as it already is for sensors. - Let DataSourceIdField dump None, so that the field's absent default serializes rather than failing the OpenAPI generation. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * ui/assets: pick a data source for a new automation, and see what it stores Context: - The creation form asked for a data generator class and a config as free text, with no way to say "the one that automation already uses", and nothing showed what a generator would be configured with until after the automation existed. Change: - Add a search box above those two fields, which lists the data sources of the automation's own type, matched by id, by name and by generator class, and lists them on focus so they can be browsed without guessing a term. - Show the picked source's description and the configuration it stores, and disable the class and config fields while it is picked, since the source already determines both. Clearing it hands them back. - Drop the whole section for a schedule automation, which has no generator to name, and clear a selection when the type changes, as the source type does. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * ui/assets: copy an automation into the creation form The Automations page could create an automation from scratch or edit one's recurrence, but a variation on an existing automation, such as the same forecast on a different schedule, had to be typed out again from its details. Each row's Actions menu now offers Copy, which opens the creation form with the original's type, recurrence, timezone and parameters filled in. Nothing is created until the form is submitted, so the copy is a starting point that can still be changed, rather than a command of its own. The copy is filled in inactive, so that a variation being worked on does not start running while it is still being edited, and so that two automations writing the same results never appear from a single click. Its name carries a (copy) suffix, cut to the 80 characters the field and the API allow, so that a long name loses its tail rather than the suffix that marks it. A forecast or report copy reuses the data source the original computes under, which already stores the data generator and its configuration, rather than naming that class and config again. A schedule automation resolves its source afresh on every run and cannot name one, so a copy of one names none. The parameters are carried over exactly as stored, which is safe here because the copy stays on the same asset, so the sensors they name stay valid and the creation endpoint checks them against the user's permissions as it would for any new automation. The creation form is now shared between New automation and Copy, so opening it blank clears what a copy left in it, rather than inheriting the last copy's settings. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs/changelog: point the data source picker entries at PR #2584 Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs/changelog: fold the copy button into the automations UI entry Automations have not shipped, so the changelog says what the feature is rather than how it got there: the entry that already covers creating and managing automations in the UI names Copy among the row's controls and carries this PR's number, instead of a bullet of its own. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs/changelog: carry this work on the automations entry that already covers it The automations feature has not shipped yet, so the changelog describes where it ends up rather than the steps it took to get there. Someone reading the v1.1.0 notes never saw a version of the *New automation* form without a data source to pick, and a bullet announcing that one arrived tells them about a past they have no memory of. The per-PR list under *Automations, in detail* is the place where a contribution is still worth naming, so this PR's number joins the entry that already describes the creation form and the data source it records under. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * docs/api: leave the automations endpoint to the release that introduces it ``POST /assets/<id>/automations`` arrived at v3.0-37, inside this same unreleased cycle, so no API consumer has ever called it without a ``source``. The release will simply introduce the endpoint, with that field on it, and an entry saying it "now accepts" one describes a revision nobody integrated against. The ``GET /sources`` entry stays. That listing predates this cycle and is already in users' hands, so a new way to search and narrow it is a change they need told about. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/schemas/automations: say what the source field is, not what it implies The field's description had grown into a paragraph, restating what a data source stores, how the results are attributed to it, and why a schedule automation may not name one. All three are already in the automations documentation, where they have room to be explained, and none of them help someone reading the OpenAPI reference to decide what to put in the field. One sentence is left: it is the ID of a data source to reuse, in place of naming a generator and a config. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * data/schemas: let the source field be plainly optional, and leave DataSourceIdField alone Giving the field a ``load_default`` of None put a default into the OpenAPI spec, which the generator then pushed back through the field's own serializer. A field that turns an ID into a DataSource has nothing to do with None, so it raised, and the way out taken at the time was to teach it to swallow None -- changing the field for every schema that uses it, to accommodate one field's setup. The field is simply not required, so say that instead. Nothing then asks for a default, the serializer goes back to doing one thing, and the published schema improves as well: ``source`` is now an integer that may be omitted, rather than a nullable integer defaulting to null. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * ui/assets: move the automation form's reference text onto info icons The creation form carried nine blocks of help text, several of them three to five lines, so the fields it was meant to explain were spread thin between paragraphs and the modal read as a page of prose with inputs in it. Each field's label now carries the info icon the sensor creation form already uses, with the same text behind it on hover. Two hints stay on the page, because they are the ones you act on while typing rather than read once: that the generator config can be left empty, and the offset example worth copying into the parameters. Bootstrap only turns those icons into tooltips when asked, so the page now initialises them, as the chart pages do. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * ui/assets: raise the form's tooltips above the modal they belong to Bootstrap appends a tooltip to the document body, where it sits at a z-index of 1080. The theme gives a modal 999993. A tooltip opened from a field inside the creation form was therefore built, shown and positioned correctly, and painted behind the modal, so hovering an info icon appeared to do nothing at all. Anchoring each tooltip to the modal that holds its icon puts the two in one stacking context, and the tooltip comes out on top. Icons outside a modal keep the body as their container. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * ui/assets: say that the job counts reach back only as far as Redis keeps jobs The jobs column counts what the job cache returns, and that cache holds only the jobs Redis still has: FLEXMEASURES_JOB_TTL expires them, and the cache drops the ids it can no longer fetch. A daily automation two weeks old therefore shows a week of jobs on an instance that keeps them for a week, which reads as runs that never happened rather than as records that have been cleaned up. The column and the info panel now say so, naming the window this instance is configured for rather than pointing at the setting. The phrasing works for either shape the value takes, since a one-day retention reads as "a day" and not "1 day". The browser checks render the page's script through a bare Jinja environment, so that environment now carries the filter and the configuration value the page reads, keeping its promise that nothing is left unrendered. Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> * Order the data source listing, let a client limit it, and keep its types unnarrowed A search from the automation form asked for every source a user may read, in whatever order Postgres returned them, so an instance with many sources sent its whole source table to the browser, and the sources shown could differ between two searches for the same term. The listing is now ordered with the most recently created source first, and takes a limit. The source types in the response are read from every source the user may read, rather than from the sources the call returns, so that narrowing the listing no longer narrows the types a client can offer to narrow it by. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Bound the source search, and open the automation form on a clean slate The search now asks for one source more than the list shows, which is what tells the list that there are more to narrow down to, and which keeps an instance with many sources from sending all of them to the browser on every pause in typing. The list therefore says that it leaves sources out rather than how many, which is no longer a number it has. A source picked and then left behind by closing the form used to still disable the data generator fields when the form was opened again, with nothing on screen to say why, so closing the form now clears the picked source. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Read an empty source on an automation as no source at all A client which fills in the whole creation form and leaves the source field empty sends a null source, which was refused with 'Field may not be null' rather than taken as the automation setting up its own data generator. Our own form works around this by leaving the field out altogether. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Changelog: the source an automation reuses, and searching the data sources Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Drop a local import of ValidationError which the module already has Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Decide who may read a data source per source, rather than by its organisation alone Almost every data generator's source belongs to no organisation, because nothing on the creation path passes one, and a source naming neither an organisation nor a user was readable by every authenticated user. The read check this pull request added therefore hardly ever refused anything. A source is now readable when it belongs to an organisation the user may read, when an automation they may read computes under it, or when it has recorded data on a sensor they may read, which is what makes it possible to ask what produced a number one is allowed to see. The first two are what makes a source one to work with: those are the sources the listing holds, because reusing a source means recording under its lineage, which is more than reading what it computed. The stored configuration follows the stricter rule as well, since a data generator's configuration names the sensors it runs on, which can be sensors of an organisation whose data the user cannot see at all. Two tests created sources belonging to no organisation to exercise version collapsing rather than visibility; their fixtures name an organisation now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Changelog and docs: which data sources are yours to read, and which to work with Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Naming a source for an automation takes more than being allowed to read it The listing a source is picked from holds the sources the user may work with, while reading one source is allowed more widely, for a source which recorded data on a sensor they may read. Creating the automation checked the wider of the two, so a source that had recorded on one of the user's own sensors could be named for an automation of theirs, which records under that source and so writes into another organisation's lineage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Address review: say which sources the types come from, and unwrap a comment Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> * Changelog: one entry for the automations page's actions, not two The merge of main brought in the entry PR #2563 had extended, beside this branch's own edit of the same line. Neither conflicted with the other, so both survived: the section carried the same 900-character bullet twice, differing only in whether it mentioned Copy and which pull requests it cited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0129WrXeJ5gia2pctFH93BqC Signed-off-by: F.N. Claessen <claessen@seita.nl> --------- Signed-off-by: Mohamed Belhsan Hmida <mohamedbelhsanhmida@gmail.com> Signed-off-by: F.N. Claessen <claessen@seita.nl> Signed-off-by: Mohamed Belhsan Hmida <149331360+BelhsanHmida@users.noreply.github.com> Co-authored-by: F.N. Claessen <claessen@seita.nl> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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