feat: allow unit param in file upload - #221
Conversation
Signed-off-by: Nicolas Höning <nicolas@seita.nl>
Coverage Report for CI Build 31124985627Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 96.58%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Servers before v0.30.0 do not read a "unit" form field on the sensor data
upload endpoint. Their webargs location loader copies only the file and the
"belief-time-measured-instantly" field out of the form, so an unknown "unit"
never reaches marshmallow and no validation error is raised. Ingestion then
falls back to `fields.get("unit", sensor.unit)`, which makes the unit
conversion a no-op: the file's values are recorded as if they were already
in the sensor's unit, and the request still returns 200.
The result is silent data corruption (e.g. a 1000x error when uploading W
to a kW sensor) with no signal to the caller. Fail before sending anything
instead, but only when a unit is actually passed, so existing file uploads
are unaffected and incur no extra request.
Base versions are compared via _server_version_at_least so that pre-release
builds such as 0.30.0.dev5, which already expose the field, are accepted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Dropping unit from json_params is right for the mutual-exclusivity check, since a unit can now accompany a file upload. But that same list also picks the error message, so a lone unit stopped counting as "something was passed" and post_sensor_data(sensor_id=1, unit="MW") reported that neither mode was chosen -- while a unit sat right there in the call. prior was already in that position. Keep unit and prior out of the mode decision, but let them select the message, so the caller is told which parameters are missing instead. The genuinely empty call is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard leans on _server_version_at_least returning False for an unknown version, which was the one uncovered branch in that helper. A server whose /api/ response carries no flexmeasures_version key now has to be refused, since it cannot be shown to honour the unit and would otherwise record the file's values unconverted behind a 200. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BelhsanHmida
left a comment
BelhsanHmida
left a comment
There was a problem hiding this comment.
Tested this works well!
I pushed two small fixes:
1. Version guard. The unit field only landed server-side in FM 0.30.0, and older servers don't reject it they silently ignore it and store the values as if they were already in the sensor's unit. I confirmed this: the same upload without a unit stores 5000 instead of 5, and still returns 200. So passing a unit to a pre-0.30 server gives you a silent 1000x error. Now it raises before anything is uploaded. Only kicks in when a unit is actually passed, so existing uploads are unaffected, and dev builds like 0.30.0.dev5 still pass.
2. Error message. Since unit left json_params, post_sensor_data(sensor_id=1, unit="MW") was answering "you provided neither" even though you clearly did pass something. Now it tells you which parameters are missing.
Also added a test for the case where the server reports no version at all.
3 participants
The file upload (via
post_sensor_data()and_post_sensor_data_file()) should allow to specify the unit parameter. The former accepts a unit param but only uses it for JSON upload.This PR adds this missing ability.