From e737ae0fcb2b3a1a396f14b57a89e0c436d9790f Mon Sep 17 00:00:00 2001 From: Rafael RL Date: Mon, 4 Nov 2024 11:15:11 +0100 Subject: [PATCH] Add missing min max restriction --- databroker-cli/src/sdv_cli.rs | 6 +- .../src/grpc/sdv_databroker_v1/collector.rs | 82 ++++++++ .../src/grpc/sdv_databroker_v1/conversions.rs | 195 +++++++++++++----- proto/sdv/databroker/v1/types.proto | 58 ++++-- 4 files changed, 276 insertions(+), 65 deletions(-) diff --git a/databroker-cli/src/sdv_cli.rs b/databroker-cli/src/sdv_cli.rs index e52dd642..9f3fa367 100644 --- a/databroker-cli/src/sdv_cli.rs +++ b/databroker-cli/src/sdv_cli.rs @@ -1262,7 +1262,7 @@ mod test { entry_type: proto::v1::EntryType::Sensor.into(), change_type: proto::v1::ChangeType::OnChange.into(), description: "".into(), - allowed: None, + value_restriction: None, }, proto::v1::Metadata { id: 2, @@ -1271,7 +1271,7 @@ mod test { entry_type: proto::v1::EntryType::Sensor.into(), change_type: proto::v1::ChangeType::OnChange.into(), description: "".into(), - allowed: None, + value_restriction: None, }, proto::v1::Metadata { id: 3, @@ -1280,7 +1280,7 @@ mod test { entry_type: proto::v1::EntryType::Sensor.into(), change_type: proto::v1::ChangeType::OnChange.into(), description: "".into(), - allowed: None, + value_restriction: None, }, ] .to_vec(); diff --git a/databroker/src/grpc/sdv_databroker_v1/collector.rs b/databroker/src/grpc/sdv_databroker_v1/collector.rs index e0c792b8..963ab632 100644 --- a/databroker/src/grpc/sdv_databroker_v1/collector.rs +++ b/databroker/src/grpc/sdv_databroker_v1/collector.rs @@ -270,3 +270,85 @@ impl proto::collector_server::Collector for broker::DataBroker { } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::{broker::DataBroker, permissions}; + use proto::collector_server::Collector; + + #[tokio::test] + async fn test_publish_value_min_max_not_fulfilled() { + let broker = DataBroker::default(); + let authorized_access = broker.authorized_access(&permissions::ALLOW_ALL); + + let entry_id_1 = authorized_access + .add_entry( + "test.datapoint1".to_owned(), + broker::DataType::Uint8, + broker::ChangeType::OnChange, + broker::EntryType::Sensor, + "Test datapoint 1".to_owned(), + Some(broker::types::DataValue::Uint32(3)), // min + Some(broker::types::DataValue::Uint32(26)), // max + None, + None, + ) + .await + .unwrap(); + + let entry_id_2 = authorized_access + .add_entry( + "test.datapoint1.Speed".to_owned(), + broker::DataType::Float, + broker::ChangeType::OnChange, + broker::EntryType::Sensor, + "Test datapoint 1".to_owned(), + Some(broker::types::DataValue::Float(1.0)), // min + Some(broker::types::DataValue::Float(100.0)), // max + None, + None, + ) + .await + .unwrap(); + + let datapoint: proto::Datapoint = proto::Datapoint { + timestamp: None, + value: Some(proto::datapoint::Value::Int32Value(50)), + }; + + let mut datapoints = HashMap::new(); + datapoints.insert(entry_id_1, datapoint.clone()); + datapoints.insert(entry_id_2, datapoint); + + let request = proto::UpdateDatapointsRequest { datapoints }; + + // Manually insert permissions + let mut publish_value_request = tonic::Request::new(request); + publish_value_request + .extensions_mut() + .insert(permissions::ALLOW_ALL.clone()); + + match broker.update_datapoints(publish_value_request).await { + Ok(response) => { + let response = response.into_inner(); + assert_eq!(response.errors.len(), 2); + + let error_entry_1 = response.errors.get(&entry_id_1); + assert_eq!( + error_entry_1.unwrap().clone(), + proto::DatapointError::OutOfBounds as i32 + ); + + let error_entry_2 = response.errors.get(&entry_id_2); + assert_eq!( + error_entry_2.unwrap().clone(), + proto::DatapointError::InvalidType as i32 + ); + } + Err(_) => { + panic!("Should not happen!"); + } + } + } +} diff --git a/databroker/src/grpc/sdv_databroker_v1/conversions.rs b/databroker/src/grpc/sdv_databroker_v1/conversions.rs index 0e696218..68964594 100644 --- a/databroker/src/grpc/sdv_databroker_v1/conversions.rs +++ b/databroker/src/grpc/sdv_databroker_v1/conversions.rs @@ -16,6 +16,7 @@ use databroker_proto::sdv::databroker::v1 as proto; use prost_types::Timestamp; use std::convert::TryInto; use std::time::SystemTime; +use tracing::debug; use crate::broker; @@ -302,6 +303,151 @@ impl From<&proto::ChangeType> for broker::ChangeType { } } +fn value_restriction_from(metadata: &broker::Metadata) -> Option { + match metadata.data_type { + broker::DataType::String | broker::DataType::StringArray => { + let allowed = match metadata.allowed.as_ref() { + Some(broker::DataValue::StringArray(vec)) => vec.clone(), + _ => Vec::new(), + }; + + if !allowed.is_empty() { + return Some(proto::ValueRestriction { + r#type: Some(proto::value_restriction::Type::String( + proto::ValueRestrictionString { + allowed_values: allowed, + }, + )), + }); + }; + } + broker::DataType::Int8 + | broker::DataType::Int16 + | broker::DataType::Int32 + | broker::DataType::Int64 + | broker::DataType::Int8Array + | broker::DataType::Int16Array + | broker::DataType::Int32Array + | broker::DataType::Int64Array => { + let min_value = match metadata.min { + Some(broker::DataValue::Int32(value)) => Some(i64::from(value)), + Some(broker::DataValue::Int64(value)) => Some(value), + _ => None, + }; + let max_value = match metadata.max { + Some(broker::DataValue::Int32(value)) => Some(i64::from(value)), + Some(broker::DataValue::Int64(value)) => Some(value), + _ => None, + }; + let allowed = match metadata.allowed.as_ref() { + Some(allowed) => match allowed { + broker::DataValue::Int32Array(vec) => { + vec.iter().cloned().map(i64::from).collect() + } + broker::DataValue::Int64Array(vec) => vec.to_vec(), + _ => Vec::new(), + }, + _ => Vec::new(), + }; + + if min_value.is_some() | max_value.is_some() | !allowed.is_empty() { + return Some(proto::ValueRestriction { + r#type: Some(proto::value_restriction::Type::Signed( + proto::ValueRestrictionInt { + allowed_values: allowed, + min: min_value, + max: max_value, + }, + )), + }); + }; + } + broker::DataType::Uint8 + | broker::DataType::Uint16 + | broker::DataType::Uint32 + | broker::DataType::Uint64 + | broker::DataType::Uint8Array + | broker::DataType::Uint16Array + | broker::DataType::Uint32Array + | broker::DataType::Uint64Array => { + let min_value = match metadata.min { + Some(broker::DataValue::Uint32(value)) => Some(u64::from(value)), + Some(broker::DataValue::Uint64(value)) => Some(value), + _ => None, + }; + let max_value = match metadata.max { + Some(broker::DataValue::Uint32(value)) => Some(u64::from(value)), + Some(broker::DataValue::Uint64(value)) => Some(value), + _ => None, + }; + let allowed = match metadata.allowed.as_ref() { + Some(allowed) => match allowed { + broker::DataValue::Uint32Array(vec) => { + vec.iter().cloned().map(u64::from).collect() + } + broker::DataValue::Uint64Array(vec) => vec.to_vec(), + _ => Vec::new(), + }, + _ => Vec::new(), + }; + + if min_value.is_some() | max_value.is_some() | !allowed.is_empty() { + return Some(proto::ValueRestriction { + r#type: Some(proto::value_restriction::Type::Unsigned( + proto::ValueRestrictionUint { + allowed_values: allowed, + min: min_value, + max: max_value, + }, + )), + }); + }; + } + broker::DataType::Float + | broker::DataType::Double + | broker::DataType::FloatArray + | broker::DataType::DoubleArray => { + let min_value = match metadata.min { + Some(broker::DataValue::Float(value)) => Some(f64::from(value)), + Some(broker::DataValue::Double(value)) => Some(value), + _ => None, + }; + let max_value = match metadata.max { + Some(broker::DataValue::Float(value)) => Some(f64::from(value)), + Some(broker::DataValue::Double(value)) => Some(value), + _ => None, + }; + let allowed = match metadata.allowed.as_ref() { + Some(allowed) => match allowed { + broker::DataValue::FloatArray(vec) => { + vec.iter().cloned().map(f64::from).collect() + } + broker::DataValue::DoubleArray(vec) => vec.to_vec(), + _ => Vec::new(), + }, + _ => Vec::new(), + }; + + if min_value.is_some() | max_value.is_some() | !allowed.is_empty() { + return Some(proto::ValueRestriction { + r#type: Some(proto::value_restriction::Type::FloatingPoint( + proto::ValueRestrictionFloat { + allowed_values: allowed, + min: min_value, + max: max_value, + }, + )), + }); + }; + } + + _ => { + debug!("Datatype {:?} not yet handled", metadata.data_type); + } + }; + None +} + impl From<&broker::Metadata> for proto::Metadata { fn from(metadata: &broker::Metadata) -> Self { proto::Metadata { @@ -311,54 +457,7 @@ impl From<&broker::Metadata> for proto::Metadata { data_type: proto::DataType::from(&metadata.data_type) as i32, change_type: proto::ChangeType::Continuous as i32, // TODO: Add to metadata description: metadata.description.to_owned(), - allowed: match metadata.allowed.as_ref() { - Some(broker::DataValue::StringArray(vec)) => Some(proto::Allowed { - values: Some(proto::allowed::Values::StringValues(proto::StringArray { - values: vec.clone(), - })), - }), - Some(broker::DataValue::Int32Array(vec)) => Some(proto::Allowed { - values: Some(proto::allowed::Values::Int32Values(proto::Int32Array { - values: vec.clone(), - })), - }), - Some(broker::DataValue::Int64Array(vec)) => Some(proto::Allowed { - values: Some(proto::allowed::Values::Int64Values(proto::Int64Array { - values: vec.clone(), - })), - }), - Some(broker::DataValue::Uint32Array(vec)) => Some(proto::Allowed { - values: Some(proto::allowed::Values::Uint32Values(proto::Uint32Array { - values: vec.clone(), - })), - }), - Some(broker::DataValue::Uint64Array(vec)) => Some(proto::Allowed { - values: Some(proto::allowed::Values::Uint64Values(proto::Uint64Array { - values: vec.clone(), - })), - }), - Some(broker::DataValue::FloatArray(vec)) => Some(proto::Allowed { - values: Some(proto::allowed::Values::FloatValues(proto::FloatArray { - values: vec.clone(), - })), - }), - Some(broker::DataValue::DoubleArray(vec)) => Some(proto::Allowed { - values: Some(proto::allowed::Values::DoubleValues(proto::DoubleArray { - values: vec.clone(), - })), - }), - Some(broker::DataValue::BoolArray(_)) - | Some(broker::DataValue::NotAvailable) - | Some(broker::DataValue::Bool(_)) - | Some(broker::DataValue::String(_)) - | Some(broker::DataValue::Int32(_)) - | Some(broker::DataValue::Int64(_)) - | Some(broker::DataValue::Uint32(_)) - | Some(broker::DataValue::Uint64(_)) - | Some(broker::DataValue::Float(_)) - | Some(broker::DataValue::Double(_)) - | None => None, - }, + value_restriction: value_restriction_from(metadata), } } } diff --git a/proto/sdv/databroker/v1/types.proto b/proto/sdv/databroker/v1/types.proto index 44988098..8bbeb093 100644 --- a/proto/sdv/databroker/v1/types.proto +++ b/proto/sdv/databroker/v1/types.proto @@ -143,18 +143,6 @@ message Datapoint { } } -message Allowed { - oneof values { - StringArray string_values = 1; - Int32Array int32_values = 3; - Int64Array int64_values = 4; - Uint32Array uint32_values = 5; - Uint64Array uint64_values = 6; - FloatArray float_values = 7; - DoubleArray double_values = 8; - } -} - message Metadata { int32 id = 1; EntryType entry_type = 2; @@ -163,7 +151,49 @@ message Metadata { ChangeType change_type = 6; // CONTINUOUS or STATIC or ON_CHANGE string description = 7; - Allowed allowed = 10; + ValueRestriction value_restriction = 10; // int32 min_update_hz = 10; // Only for CONTINUOUS // int32 max_update_hz = 11; // Only for CONTINUOUS -}; +} + +// Value restriction +// +// One ValueRestriction{type} for each type, since +// they don't make sense unless the types match +// +message ValueRestriction { + oneof type { + ValueRestrictionString string = 21; + // For signed VSS integers + ValueRestrictionInt signed = 22; + // For unsigned VSS integers + ValueRestrictionUint unsigned = 23; + // For floating point VSS values (float and double) + ValueRestrictionFloat floating_point = 24; + } +} + +message ValueRestrictionInt { + optional sint64 min = 1; + optional sint64 max = 2; + repeated sint64 allowed_values = 3; +} + +message ValueRestrictionUint { + optional uint64 min = 1; + optional uint64 max = 2; + repeated uint64 allowed_values = 3; +} + +message ValueRestrictionFloat { + optional double min = 1; + optional double max = 2; + + // allowed for doubles/floats not recommended + repeated double allowed_values = 3; +} + +// min, max doesn't make much sense for a string +message ValueRestrictionString { + repeated string allowed_values = 1; +}