feat(schema): Enforce unique node names within a network

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::<Vec<_>>().join("; ")
+    )]
+    DuplicateNodeNames(Vec<DuplicateNodeName>),
     #[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).

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};

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,

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<SchemaError>,
+    },
     #[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<DuplicateNodeName> = 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<DuplicateNodeName> {
+        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,
+                },
+            ]
+        );
+    }
+}

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};

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 {

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"
+      }
+    ]
+  }
+}