Issue 93 - Determine payload value type based on enum (Custom deserialize())

[joaoag]

Context

• This PR outlines a proposal for further addressing issue Determine payload value type based on enum #93, which was partially addressed by PR Validate event payload values match their type #124, as per quote below:

@benjaminedwardwebb did a great job in #124 by validating if the type of the values matches what is expected from the value_type. Still, we keep this issue open as we strive for a cleaner version in the future: Instead of validating if the type is correct, we first want to parse it properly using a custom serde deserialize implementation. See the comments in #124 for the full insight.

• This PR isn’t meant to be a full implementation, just a POC with enough code to indicate the direction of travel and to understand if the approach acceptable in principle

• Before this PR: {"type":"PRICE","values":[1]} parses 1 as Value::Integer(1), so validation rejects a legitimate price because it's not formatted as a float

• After this PR: {"type":"PRICE","values":[1]} parses 1 as Value::Integer(1) and then normalises to Value::Number(1.0) so it passes validation.

Rationale for approach

• My taking from the discussions on Determine payload value type based on enum #93 and Validate event payload values match their type #124 especially the quoted comment from above, was that we wanted a way to address the class of bugs that would arise out of e.g. treating a whole number on the wire (e.g. 1) as an integer - because #[serde(untagged)] tries the Integer variant before Number - when the payload's value_type (e.g. PRICE) actually expects a float.

• While I saw that there was talk about implementing this in a way that made invalid states unrepresentable, which I agree with in principle, I wanted to see if a more conservative / intermediate-step approach would also be acceptable, so I’ve created this POC for the custom deserialization normalising after parsing.

• The approach in this PR is to accept that values may be put in the wrong types for a short time while introduces a mechanism to correct / normalize known mis-typings.

• The PR also keeps DRY the relationship between the event kinds and the value types between the new normalize function and the existing validate function. I thought it’d be preferable for them to share the same source of truth, but appreciate that introduces tighter coupling which may not be desired - happy to keep two separate lists if it’s preferred.

Summary of changes

1. Introduced custom deserialize(), which:
   1. First deserializes Value as per existing enum + serde macro
   2. Then attempts to normalise known edge case bugs into correct types using new normalize_value() function and associated helper methods
2. Centralised behaviour and data responsible for specifying and asserting against EventType variants and their expected Value type/kind variants via a new EventType::expected_value() method returning a ValueKind, consumed by both normalize_value() and validate_value()
3. Fixed some import formatting
4. Added basic unit tests to cover new functionality

Outstanding work to be implemented if approach gets approved

1. Decision on reverse cases e.g. should 1.0 arriving for an integer-typed field (SIMPLE, CTA2045_*) be normalized to an int?
2. benjaminedwardwebb referenced §10.2 of the OpenADR 3.0.1 Definitions and a mapping table in Validate event payload values match their type #124 is that the authoritative source for the full set of normalisation cases, or is there a newer spec version I should work from?

Misc. notes for reviewers

• I removed most of the comments against the EventTypes, only keeping ones which looked like they contained very specific domain knowledge - very happy to reinstate the others if needed

• Normalisation runs only on the deserialize() path, so a hand-constructed EventValuesMap with an Integer under PRICE wouldn't be normalised and would fail validate() - I assumed this was acceptable given the discussions, let me know if not.

• I considered having expected_value() return an Option<ValueKind> and having any EventTypes which have no constraints return None (instead of the current return of Any) e.g. inside expected_value() this arm would be ControlSetpoint | Private(_) => None, - roughly the changes outlined below. Happy to implement this way, if it’s preferred.

  enum ValueKind { Integer, Number, Boolean, Point, Text }   // no Any

  impl EventType {
      fn expected_value(&self) -> Option<ValueKind> {
          use EventType::*;
          match self {
              Price | ExportPrice | ... => Some(ValueKind::Number),
//             ..etc
              AlertGridEmergency | ... => Some(ValueKind::Text),
              ControlSetpoint | Private(_) => None,   // no constraint — reads as "anything"
          }
      }
  }  

fn validate_value(value_type: &EventType, value: &Value) -> Result<(), ValidationError> {
      match value_type.expected_value() {
          None => Ok(()), // accepts anything
          Some(expected) if value.kind() == expected => Ok(()),
          _ => Err(validate_value_error(value_type, value)),
      }
  }

[codecov]

Codecov Report

❌ Patch coverage is 95.58824% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.84%. Comparing base (6961f00) to head (0784992).

Files with missing lines	Patch %	Lines
openleadr-wire/src/event.rs	96.66%	2 Missing ⚠️
openleadr-wire/src/values_map.rs	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #462      +/-   ##
==========================================
+ Coverage   83.36%   83.84%   +0.47%     
==========================================
  Files          50       50              
  Lines        7354     7385      +31     
==========================================
+ Hits         6131     6192      +61     
+ Misses       1223     1193      -30     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[joaoag]

@stefanvi just a gentle nudge on this for when you get back from your holidays : )