diff --git a/dc_measurements/plugins/measurements/json/thermal.json b/dc_measurements/plugins/measurements/json/thermal.json index 1d4a5335c..c0d228523 100644 --- a/dc_measurements/plugins/measurements/json/thermal.json +++ b/dc_measurements/plugins/measurements/json/thermal.json @@ -1,10 +1,16 @@ { "$schema": "http://json-schema.org/draft-07/schema#", "title": "Thermal", - "description": "Temperature readings from /sys/class/thermal, keyed by each zone's type string", + "description": "One entry per thermal zone read this cycle, keyed by the zone's type string", + "propertyNames": { + "description": "The zone's type string, e.g. x86_pkg_temp or cpu-thermal", + "minLength": 1 + }, "additionalProperties": { "description": "Temperature of the zone in degrees Celsius", - "type": "number" + "type": "number", + "minimum": -273.15 }, + "minProperties": 1, "type": "object" } diff --git a/dc_measurements/test/test_measurement_thermal.cpp b/dc_measurements/test/test_measurement_thermal.cpp index 456401349..ff51e7481 100644 --- a/dc_measurements/test/test_measurement_thermal.cpp +++ b/dc_measurements/test/test_measurement_thermal.cpp @@ -4,13 +4,44 @@ #include #include #include +#include #include #include +#include "ament_index_cpp/get_package_share_directory.hpp" #include "dc_interfaces/msg/string_stamped.hpp" #include "dc_measurements/measurement_server.hpp" #include "dc_util/json_utils.hpp" +// Runs a Record through the Measurement's own installed schema, the same file and validator +// `Measurement::validateJSON()` loads when `enable_validator` is on. +class ThermalSchema +{ +public: + ThermalSchema() + { + std::ifstream schema_file(ament_index_cpp::get_package_share_directory("dc_measurements") + + "/plugins/measurements/json/thermal.json"); + validator_.set_root_schema(nlohmann::json::parse(schema_file)); + } + + bool accepts(const nlohmann::json& record) + { + try + { + validator_.validate(record); + return true; + } + catch (const std::exception&) + { + return false; + } + } + +private: + nlohmann::json_schema::json_validator validator_; +}; + // Builds a fake /sys/class/thermal-shaped directory tree under /tmp so the plugin can be // exercised for real (auto-discovery, zone `type` as Record key, ARM-style non-numeric-suffix // naming) without depending on whatever thermal zones (if any) the test host/container exposes. @@ -172,6 +203,66 @@ TEST_F(MeasurementThermalTest, ActivatesSuccessfullyWithMissingBasePath) SUCCEED(); } +TEST(ThermalSchemaTest, AcceptsARepresentativeRecord) +{ + ThermalSchema schema; + EXPECT_TRUE(schema.accepts(nlohmann::json{ { "x86_pkg_temp", 52.0 }, { "gpu-thermal", 61.5 } })); +} + +TEST(ThermalSchemaTest, RejectsARecordWithNoZoneEntry) +{ + // Zone type strings *are* the field names, so the entry itself is what's required: a Record + // with none carries no reading at all. The Measurement never emits one (it publishes nothing + // that cycle instead), so an empty Record reaching a Destination means something went wrong. + ThermalSchema schema; + EXPECT_FALSE(schema.accepts(nlohmann::json::object())); +} + +TEST(ThermalSchemaTest, RejectsMalformedZoneEntries) +{ + ThermalSchema schema; + EXPECT_FALSE(schema.accepts(nlohmann::json{ { "cpu-thermal", "45.1" } })); + EXPECT_FALSE(schema.accepts(nlohmann::json{ { "cpu-thermal", -400.0 } })); + EXPECT_FALSE(schema.accepts(nlohmann::json{ { "", 45.1 } })); +} + +TEST_F(MeasurementThermalTest, PublishedRecordValidatesAgainstTheSchema) +{ + FakeThermalTree tree; + tree.addZone("thermal_zone0", "x86_pkg_temp", 52000); + + declareCommonParameters(); + ms_node_->declare_parameter("thermal.base_path", tree.path()); + + startLifecycleNode(); + spinUntilCallback(); + + // publish() enriches the Record *after* validateJSON() has run, so strip what the framework + // added to get back the Record the validator actually saw. + nlohmann::json record = data_json_; + for (const char* enrichment_key : { "name", "plugin", "nested", "flattened", "run_id", "tags" }) + { + record.erase(enrichment_key); + } + + EXPECT_TRUE(ThermalSchema().accepts(record)); +} + +TEST_F(MeasurementThermalTest, PublishesTheSameRecordWithTheValidatorOff) +{ + FakeThermalTree tree; + tree.addZone("thermal_zone0", "cpu-thermal", 45123); + + declareCommonParameters(); + ms_node_->declare_parameter("thermal.base_path", tree.path()); + ms_node_->declare_parameter("thermal.enable_validator", false); + + startLifecycleNode(); + spinUntilCallback(); + + EXPECT_DOUBLE_EQ(data_json_["cpu-thermal"].get(), 45.123); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); diff --git a/doc/src/dc/measurements/thermal.md b/doc/src/dc/measurements/thermal.md index 07283bded..ff0a78040 100644 --- a/doc/src/dc/measurements/thermal.md +++ b/doc/src/dc/measurements/thermal.md @@ -21,15 +21,26 @@ empty Record. ## Schema +Zone type strings are the field names, so they can't be listed ahead of time — the schema +constrains their shape instead: at least one entry (the Measurement publishes nothing rather +than an empty Record), a non-empty type string as key, and a temperature in degrees Celsius +above absolute zero as value. + ```json { "$schema": "http://json-schema.org/draft-07/schema#", "title": "Thermal", - "description": "Temperature readings from /sys/class/thermal, keyed by each zone's type string", + "description": "One entry per thermal zone read this cycle, keyed by the zone's type string", + "propertyNames": { + "description": "The zone's type string, e.g. x86_pkg_temp or cpu-thermal", + "minLength": 1 + }, "additionalProperties": { "description": "Temperature of the zone in degrees Celsius", - "type": "number" + "type": "number", + "minimum": -273.15 }, + "minProperties": 1, "type": "object" } ``` diff --git a/progress.txt b/progress.txt index 5d827fb3e..0dfce9118 100644 --- a/progress.txt +++ b/progress.txt @@ -6634,3 +6634,66 @@ package carrying all three placeholder fields: each is reported, and both checks the workspace agrees (single-quoted `version='…'` in setup.py included, which upstream's `grep` did not accept). Not run locally: the `build-doc` hook, unchanged by this work and covered by `doc.yaml` in CI. + +## #300 - Give the Thermal Measurement a schema that actually constrains its Record + +`thermal.json` declared a `type: object` with an `additionalProperties: {type: number}` and +nothing else — no floor on how few entries a Record may carry, no constraint on the keys, and +no bound on the values. `{}` validated, and so did a Record whose only "zone" was named `""`. +Every other Measurement's schema says something about what it emits; this one didn't. + +**The shape stays as it is; only the schema changed.** Zone `type` strings *are* the field +names (`x86_pkg_temp`, `cpu-thermal`, …), platform-specific and unknowable ahead of time, so +there is no fixed `properties`/`required` list to write — which is why the original schema +punted. Draft-07 has the vocabulary for exactly this case, and the new schema uses it: + +- `minProperties: 1` — a Record must carry at least one zone entry. This is the "missing a + required field" case for a map-shaped Record: the entry *is* the field. `collect()` already + returns an empty `StringStamped` (publishing nothing that cycle) rather than a content-free + `{}` when no zone can be read, so this can't fire on the plugin's own output — an empty + Thermal Record reaching a Destination means something went wrong upstream. +- `propertyNames: {minLength: 1}` — a zone whose `type` file is empty can't become a nameless + key. `readZone()` already rejects an empty `type`, so again the schema documents and enforces + an invariant the plugin holds rather than inventing a new one. +- `additionalProperties: {type: number, minimum: -273.15}` — degrees Celsius, above absolute + zero. A garbage `temp` read (or a units mix-up putting millidegrees through unconverted in + the negative direction) is caught instead of shipped. + +Nothing about collection changed: `base_path`, `zones`, auto-discovery, the graceful-degradation +contract and the emitted `{"": }` map are all untouched, and with +`enable_validator` off the plugin never loads the schema at all (`setValidationSchema()` is +already guarded on the flag) so its behaviour is unchanged either way. + +**Tests** (`test_measurement_thermal.cpp`, 3 existing → 8). A `ThermalSchema` helper loads the +*installed* `thermal.json` through `ament_index_cpp::get_package_share_directory()` — the same +file and the same `nlohmann::json_schema::json_validator` `Measurement::validateJSON()` uses, so +the test exercises the shipped artifact rather than a copy of it: + +- `AcceptsARepresentativeRecord` — a two-zone Record validates. +- `RejectsARecordWithNoZoneEntry` — `{}` fails (`minProperties`). +- `RejectsMalformedZoneEntries` — a string temperature, a sub-absolute-zero temperature, and an + empty zone-type key each fail. +- `PublishedRecordValidatesAgainstTheSchema` — end-to-end through the `MeasurementServer` + harness with the validator on (its default), against a `FakeThermalTree`. +- `PublishesTheSameRecordWithTheValidatorOff` — `enable_validator: false`, same Record. + +Worth recording, because the first draft of the end-to-end test failed on it: `publish()` calls +`enrichMsg()`, which runs `validateJSON()` **first** and only then adds `name`, `nested`, +`flattened`, `run_id` and `tags`. So the Record a subscriber sees is not the one the validator +saw, and a schema with a closed `additionalProperties` would reject it — the test strips those +framework keys before validating. This is why the schema can constrain values as tightly as it +does without breaking the enrichment path. + +**Verified for real**, not just linted: no colcon/ROS 2 in this sandbox (same recurring +constraint as the rest of this log), so the cached `localhost/dc-workspace:latest` Podman image +was reused with this worktree bind-mounted over `/root/ws/src/ros2_data_collection` — the image +was five days stale and missing `dc_common/file_scratch_ring.hpp`, so the whole tree had to be +mounted and `colcon build --packages-up-to dc_measurements` re-run, not just `dc_measurements`. +`colcon test --packages-select dc_measurements`: 8/8 thermal tests pass, and the whole package +suite is green at 163 tests / 0 failures. The +`INFO … schema:` line in the test log confirms the new schema is what the running plugin loads, +and that `nlohmann_json_schema_validator` in Jazzy implements `propertyNames`/`minProperties` +(it does — the three negative cases all fail as intended, which was the one real unknown here). +`prek run --files …` passes on the three changed files. `doc/src/dc/measurements/thermal.md`'s +Schema section carries the new schema plus a sentence on why the constraints are shaped this +way instead of a `required` list.