From 2350a6da3d1ce3cf582420f023fc236460e9a251 Mon Sep 17 00:00:00 2001 From: James Date: Tue, 11 Aug 2026 16:22:14 +0100 Subject: [PATCH] feat(schema): Enforce unique node names within a network --- pywr-schema/src/error.rs | 26 +++ pywr-schema/src/lib.rs | 2 +- pywr-schema/src/model.rs | 19 +- pywr-schema/src/network.rs | 164 +++++++++++++++++- pywr-schema/src/nodes/delay.rs | 2 +- pywr-schema/src/nodes/mod.rs | 6 +- .../duplicate-node-name-with-composite.json | 72 ++++++++ .../invalid/duplicate-virtual-node-name.json | 86 +++++++++ pywr-schema/tests/test_invalid.rs | 51 +++++- 9 files changed, 416 insertions(+), 12 deletions(-) create mode 100644 pywr-schema/tests/invalid/duplicate-node-name-with-composite.json create mode 100644 pywr-schema/tests/invalid/duplicate-virtual-node-name.json diff --git a/pywr-schema/src/error.rs b/pywr-schema/src/error.rs index 0b6e2413..27f68bae 100644 --- a/pywr-schema/src/error.rs +++ b/pywr-schema/src/error.rs @@ -10,12 +10,38 @@ use pyo3::prelude::*; use std::path::PathBuf; use thiserror::Error; +/// A node name that is used by more than one node in a network. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DuplicateNodeName { + /// The duplicated name. + pub name: String, + /// The number of nodes with this name. + pub nodes: usize, + /// The number of virtual nodes with this name. + pub virtual_nodes: usize, +} + +impl std::fmt::Display for DuplicateNodeName { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "`{}` ({} node(s), {} virtual node(s))", + self.name, self.nodes, self.virtual_nodes + ) + } +} + #[derive(Error, Debug)] pub enum SchemaError { // Catch infallible errors here rather than unwrapping at call site. This should be safer // in the long run if an infallible error is changed to a fallible one. #[error("Infallible error: {0}")] Infallible(#[from] std::convert::Infallible), + #[error( + "Node names must be unique. Each name may be used by only one entry of `nodes` or `virtual_nodes`. Duplicate name(s) found: {}", + .0.iter().map(|d| d.to_string()).collect::>().join("; ") + )] + DuplicateNodeNames(Vec), #[error("IO error on path `{path}`: {error}")] IO { path: PathBuf, error: std::io::Error }, // Use this error when a node is not found in the schema (i.e. while parsing the schema). diff --git a/pywr-schema/src/lib.rs b/pywr-schema/src/lib.rs index 562af9c6..35d07c1c 100644 --- a/pywr-schema/src/lib.rs +++ b/pywr-schema/src/lib.rs @@ -23,7 +23,7 @@ mod v1; mod visit; pub use digest::{Checksum, ChecksumError}; -pub use error::{ComponentConversionError, ConversionError, SchemaError}; +pub use error::{ComponentConversionError, ConversionError, DuplicateNodeName, SchemaError}; pub use model::{ModelSchema, ModelSchemaReadError, MultiNetworkModelSchema}; #[cfg(feature = "core")] pub use model::{ModelSchemaBuildError, MultiNetworkModelSchemaBuildError}; diff --git a/pywr-schema/src/model.rs b/pywr-schema/src/model.rs index f51787c2..d1095b78 100644 --- a/pywr-schema/src/model.rs +++ b/pywr-schema/src/model.rs @@ -1,8 +1,6 @@ #[cfg(feature = "core")] use crate::data_tables::LoadedTableCollection; -use crate::error::ComponentConversionError; -#[cfg(feature = "core")] -use crate::error::SchemaError; +use crate::error::{ComponentConversionError, SchemaError}; use crate::metric::Metric; #[cfg(feature = "core")] use crate::network::{LoadArgs, NetworkSchemaBuildError, NetworkSchemaReadError}; @@ -498,6 +496,11 @@ impl ModelSchema { Ok(serde_json::from_str(data.as_str())?) } + /// Validate the model's schema. See [`NetworkSchema::validate`]. + pub fn validate(&self) -> Result<(), SchemaError> { + self.network.validate() + } + /// Create a [`pywr_core::models::ModelBuilder`] from the schema. #[cfg(feature = "core")] pub fn create_model_builder( @@ -804,6 +807,16 @@ impl MultiNetworkModelSchema { Ok(serde_json::from_str(data.as_str())?) } + /// Validate the schema of each network in the model. See [`NetworkSchema::validate`]. + pub fn validate(&self) -> Result<(), SchemaError> { + for entry in &self.networks { + if let NetworkSchemaRef::Inline(network) = &entry.network { + network.validate()?; + } + } + Ok(()) + } + #[cfg(feature = "core")] pub fn create_model_builder( &self, diff --git a/pywr-schema/src/network.rs b/pywr-schema/src/network.rs index cf16f378..e993c198 100644 --- a/pywr-schema/src/network.rs +++ b/pywr-schema/src/network.rs @@ -5,9 +5,7 @@ use crate::ConversionError; use crate::data_tables::DataTable; #[cfg(feature = "core")] use crate::data_tables::{LoadedTableCollection, TableCollectionLoadError}; -use crate::error::ComponentConversionError; -#[cfg(feature = "core")] -use crate::error::SchemaError; +use crate::error::{ComponentConversionError, DuplicateNodeName, SchemaError}; use crate::metric::Metric; use crate::metric_sets::MetricSet; #[cfg(feature = "core")] @@ -27,6 +25,7 @@ use pywr_core::models::ModelDomain; use pywr_schema_macros::skip_serializing_none; use pywr_v1_schema::nodes::{CoreNode as CoreNodeV1, Node as NodeV1}; use schemars::JsonSchema; +use std::collections::HashMap; use std::path::{Path, PathBuf}; use std::str::FromStr; use strum_macros::{Display, EnumDiscriminants, EnumIter, EnumString, IntoStaticStr}; @@ -45,6 +44,11 @@ pub enum NetworkSchemaReadError { #[cfg(feature = "core")] #[derive(Error, Debug)] pub enum NetworkSchemaBuildError { + #[error("Network schema validation failed: {source}")] + Validation { + #[source] + source: Box, + }, #[error("Circular node reference(s) found.")] CircularNodeReference, #[error("Circular parameters reference(s) found. Unable to load the following parameters: {0:?}")] @@ -445,6 +449,61 @@ impl NetworkSchema { } } + /// Returns true if any node or virtual node in the network is called `name`. + pub fn node_name_exists(&self, name: &str) -> bool { + self.get_node_by_name(name).is_some() || self.get_virtual_node_by_name(name).is_some() + } + + /// Validate the network schema. + /// + /// Validation is **not** exhaustive. It checks that the schema is internally unambiguous, + /// not that it can be built, which should be done using [`NetworkSchema::add_to_network`] + /// instead. + /// + /// # Unique node names + /// + /// Every entry of [`NetworkSchema::nodes`] and [`NetworkSchema::virtual_nodes`] must have a + /// unique name. + /// + /// Duplicates must be rejected here because name resolution in this crate is + /// first-match-wins (see [`NetworkSchema::get_node_by_name`]). A duplicate therefore does + /// not fail on its own: it silently binds every reference to whichever entry appears first + /// and leaves the other unreachable. + /// + /// `pywr-core` cannot catch this for us. Its duplicate check runs over the *expanded* core + /// nodes, keyed by name **and** sub-name. One schema node may expand to several core nodes, + /// and composite types such as [`crate::nodes::DelayNode`] emit only sub-named ones. + pub fn validate(&self) -> Result<(), SchemaError> { + // Count the occurrences of each name in each of the two lists. + let mut counts: HashMap<&str, (usize, usize)> = HashMap::with_capacity(self.nodes.len()); + + for node in &self.nodes { + counts.entry(node.name()).or_default().0 += 1; + } + + for virtual_node in self.virtual_nodes.as_deref().into_iter().flatten() { + counts.entry(virtual_node.name()).or_default().1 += 1; + } + + let mut duplicates: Vec = counts + .into_iter() + .filter(|(_, (nodes, virtual_nodes))| nodes + virtual_nodes > 1) + .map(|(name, (nodes, virtual_nodes))| DuplicateNodeName { + name: name.to_string(), + nodes, + virtual_nodes, + }) + .collect(); + + if duplicates.is_empty() { + Ok(()) + } else { + // Sort for a deterministic error message. + duplicates.sort_by(|a, b| a.name.cmp(&b.name)); + Err(SchemaError::DuplicateNodeNames(duplicates)) + } + } + #[cfg(feature = "core")] pub fn add_to_network( &self, @@ -454,6 +513,11 @@ impl NetworkSchema { output_path: Option<&Path>, inter_network_transfers: &[MultiNetworkTransfer], ) -> Result<(LoadedTableCollection, LoadedTimeseriesCollection), NetworkSchemaBuildError> { + // Reject an invalid schema before doing any work to build it. + self.validate().map_err(|source| NetworkSchemaBuildError::Validation { + source: Box::new(source), + })?; + let tables = LoadedTableCollection::from_schema(self.tables.as_deref(), data_path)?; let timeseries = LoadedTimeseriesCollection::from_schema(self.timeseries.as_deref(), domain, data_path)?; @@ -557,3 +621,97 @@ pub enum NetworkSchemaRef { Path(PathBuf), Inline(NetworkSchema), } + +#[cfg(test)] +mod tests { + use super::NetworkSchema; + use crate::error::{DuplicateNodeName, SchemaError}; + use std::str::FromStr; + + /// Return the duplicates reported by [`NetworkSchema::validate`], or panic if it succeeded. + fn expect_duplicates(network: &NetworkSchema) -> Vec { + match network.validate() { + Err(SchemaError::DuplicateNodeNames(duplicates)) => duplicates, + Err(e) => panic!("Expected a duplicate name error, got: {e}"), + Ok(()) => panic!("Expected validation to fail, but it succeeded"), + } + } + + /// A network where a node and a virtual node are both called `licence`. + const NETWORK_WITH_SHARED_NODE_AND_VIRTUAL_NODE_NAME: &str = r#" + { + "nodes": [ + { "meta": { "name": "licence" }, "type": "Input" }, + { "meta": { "name": "demand1" }, "type": "Output" } + ], + "virtual_nodes": [ + { + "meta": { "name": "licence" }, + "type": "Aggregated", + "nodes": [{ "name": "demand1" }] + } + ], + "edges": [ + { "from_node": "licence", "to_node": "demand1" } + ] + } + "#; + + /// Nodes and virtual nodes are a single name-space, so a name shared between the two lists + /// is a duplicate. + #[test] + fn test_validate_rejects_name_shared_with_virtual_node() { + let network = NetworkSchema::from_str(NETWORK_WITH_SHARED_NODE_AND_VIRTUAL_NODE_NAME).unwrap(); + + assert_eq!( + expect_duplicates(&network), + vec![DuplicateNodeName { + name: "licence".to_string(), + nodes: 1, + virtual_nodes: 1, + }] + ); + } + + /// A network with two separately duplicated names, plus a unique one. + const NETWORK_WITH_SEVERAL_DUPLICATES: &str = r#" + { + "nodes": [ + { "meta": { "name": "zzz" }, "type": "Input" }, + { "meta": { "name": "zzz" }, "type": "Input" }, + { "meta": { "name": "aaa" }, "type": "Output" }, + { "meta": { "name": "unique" }, "type": "Output" } + ], + "virtual_nodes": [ + { + "meta": { "name": "aaa" }, + "type": "Aggregated", + "nodes": [{ "name": "unique" }] + } + ], + "edges": [] + } + "#; + + /// Every duplicate is reported, not just the first one found. + #[test] + fn test_validate_reports_all_duplicates() { + let network = NetworkSchema::from_str(NETWORK_WITH_SEVERAL_DUPLICATES).unwrap(); + + assert_eq!( + expect_duplicates(&network), + vec![ + DuplicateNodeName { + name: "aaa".to_string(), + nodes: 1, + virtual_nodes: 1, + }, + DuplicateNodeName { + name: "zzz".to_string(), + nodes: 2, + virtual_nodes: 0, + }, + ] + ); + } +} diff --git a/pywr-schema/src/nodes/delay.rs b/pywr-schema/src/nodes/delay.rs index b261b874..b5820eb9 100644 --- a/pywr-schema/src/nodes/delay.rs +++ b/pywr-schema/src/nodes/delay.rs @@ -1,13 +1,13 @@ use crate::error::{ComponentConversionError, ConversionError}; use crate::nodes::NodeMeta; use crate::parameters::{ConstantValue, Parameter}; +use crate::v1::try_convert_node_meta; #[cfg(feature = "core")] use crate::{ error::SchemaError, network::LoadArgs, nodes::{NodeAttribute, NodeComponent, NodeSlot}, }; -use crate::v1::try_convert_node_meta; use crate::{mermaid, node_attribute_subset_enum, node_component_subset_enum}; #[cfg(feature = "core")] use pywr_core::{metric::UnresolvedMetricF64, node::UnresolvedNode, parameters::ParameterName}; diff --git a/pywr-schema/src/nodes/mod.rs b/pywr-schema/src/nodes/mod.rs index 57a0a1e5..8cbdb06a 100644 --- a/pywr-schema/src/nodes/mod.rs +++ b/pywr-schema/src/nodes/mod.rs @@ -178,12 +178,14 @@ impl NodeBuilder { } /// Create the next default name without duplicating an existing name in the model. + /// + /// Nodes and virtual nodes share a single name-space, so this checks both. pub fn next_default_name_for_model(mut self, network: &NetworkSchema) -> Self { let mut num = 1; loop { let name = format!("{}-{}", self.ty, num); - if network.get_node_by_name(&name).is_none() { - // No node with this name found! + if !network.node_name_exists(&name) { + // No node or virtual node with this name found! self.name = Some(name); break; } else { diff --git a/pywr-schema/tests/invalid/duplicate-node-name-with-composite.json b/pywr-schema/tests/invalid/duplicate-node-name-with-composite.json new file mode 100644 index 00000000..e49af4cc --- /dev/null +++ b/pywr-schema/tests/invalid/duplicate-node-name-with-composite.json @@ -0,0 +1,72 @@ +{ + "metadata": { + "title": "Invalid model with a simple and a composite node sharing a name", + "minimum_version": "0.1" + }, + "time": { + "start": "2015-01-01", + "end": "2015-01-10", + "timestep": { + "type": "Days", + "days": 1 + } + }, + "network": { + "nodes": [ + { + "meta": { + "name": "supply1" + }, + "type": "Input", + "max_flow": { + "type": "Literal", + "value": 5 + } + }, + { + "meta": { + "name": "source1" + }, + "type": "Input" + }, + { + "meta": { + "name": "supply1" + }, + "type": "Delay", + "delay": { + "type": "Literal", + "value": 3 + }, + "initial_value": { + "type": "Literal", + "value": 0.0 + } + }, + { + "meta": { + "name": "demand1" + }, + "type": "Output", + "max_flow": { + "type": "Literal", + "value": 10 + }, + "cost": { + "type": "Literal", + "value": -10 + } + } + ], + "edges": [ + { + "from_node": "supply1", + "to_node": "demand1" + }, + { + "from_node": "source1", + "to_node": "supply1" + } + ] + } +} diff --git a/pywr-schema/tests/invalid/duplicate-virtual-node-name.json b/pywr-schema/tests/invalid/duplicate-virtual-node-name.json new file mode 100644 index 00000000..f7a5ba7c --- /dev/null +++ b/pywr-schema/tests/invalid/duplicate-virtual-node-name.json @@ -0,0 +1,86 @@ +{ + "metadata": { + "title": "Invalid model with two virtual nodes sharing a name", + "minimum_version": "0.1" + }, + "time": { + "start": "2015-01-01", + "end": "2015-01-10", + "timestep": { + "type": "Days", + "days": 1 + } + }, + "network": { + "nodes": [ + { + "meta": { + "name": "supply1" + }, + "type": "Input", + "max_flow": { + "type": "Literal", + "value": 5 + } + }, + { + "meta": { + "name": "storage1" + }, + "type": "Storage", + "max_volume": { + "type": "Literal", + "value": 100 + }, + "initial_volume": { + "type": "Proportional", + "proportion": 1.0 + } + }, + { + "meta": { + "name": "demand1" + }, + "type": "Output", + "cost": { + "type": "Literal", + "value": -10 + } + } + ], + "virtual_nodes": [ + { + "meta": { + "name": "total" + }, + "type": "Aggregated", + "nodes": [ + { + "name": "supply1" + } + ] + }, + { + "meta": { + "name": "total" + }, + "type": "AggregatedStorage", + "storage_nodes": [ + { + "name": "storage1" + } + ] + } + ], + "edges": [ + { + "from_node": "supply1", + "to_node": "storage1" + }, + { + "from_node": "storage1", + "to_node": "demand1" + } + ] + } +} diff --git a/pywr-schema/tests/test_invalid.rs b/pywr-schema/tests/test_invalid.rs index e3780f8c..477f9cb8 100644 --- a/pywr-schema/tests/test_invalid.rs +++ b/pywr-schema/tests/test_invalid.rs @@ -1,6 +1,6 @@ -use pywr_schema::ModelSchema; +use pywr_schema::{ModelSchema, SchemaError}; #[cfg(feature = "core")] -use pywr_schema::ModelSchemaBuildError; +use pywr_schema::{ModelSchemaBuildError, NetworkSchemaBuildError}; use std::fs; use std::path::Path; #[cfg(feature = "core")] @@ -40,6 +40,53 @@ invalid_tests! { agg_storage_with_flow_node: "agg-storage-with-flow-node.json", NetworkBuildError, } +/// Models that are rejected by [`ModelSchema::validate`]. +macro_rules! invalid_schema_tests { + ($($test_func:ident: $value:expr, $expected_err:ident,)*) => { + $( + #[test] + fn $test_func() { + let input: &str = $value; + let input_pth = Path::new(env!("CARGO_MANIFEST_DIR")).join("tests").join("invalid").join(input); + + let schema = deserialise_test_model(&input_pth); + + match schema.validate() { + Ok(()) => panic!("Expected validation to fail, but the schema was valid!"), + Err(e) => { + if !matches!(e, SchemaError::$expected_err { .. }) { + panic!("Expected error: SchemaError::{}, but got: {:?}", stringify!($expected_err), e); + } + } + } + + // The same error must also stop the model being built. + #[cfg(feature = "core")] + { + match build_test_model(&schema) { + ModelSchemaBuildError::NetworkBuildError { source } => { + if !matches!(*source, NetworkSchemaBuildError::Validation { .. }) { + panic!("Expected a validation error when building, but got: {:?}", source); + } + } + e => panic!("Expected ModelSchemaBuildError::NetworkBuildError, but got: {e:?}"), + } + } + } + )* + } +} + +invalid_schema_tests! { + // Two virtual nodes sharing a name. The two are built into separate pywr-core collections, + // so the core builder never sees a clash. + duplicate_virtual_node_name: "duplicate-virtual-node-name.json", DuplicateNodeNames, + // A simple and a composite node sharing a name. The composite node expands only to + // sub-named core nodes, so again the core builder never sees a clash. Validation is the + // only thing standing between this model and a silently wrong network. + duplicate_node_name_with_composite: "duplicate-node-name-with-composite.json", DuplicateNodeNames, +} + fn deserialise_test_model(model_path: &Path) -> ModelSchema { let data = fs::read_to_string(model_path).expect("Unable to read file"); serde_json::from_str(&data).expect("Failed to deserialize model")