From 9265e9bb26c0a9c985914d245ece425ce805f465 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Feb 2026 02:22:40 +0000 Subject: [PATCH 1/2] engine: replace protobuf patch types with native Rust types, add module support The apply_patch API previously converted JSON input to protobuf (project_io::ProjectPatch) as an intermediate format before applying operations to the datamodel. This was a leftover from when the API was protobuf-based. This change defines native Rust patch types (ProjectPatch, ModelPatch, ProjectOperation, ModelOperation) that use datamodel types directly, eliminating the protobuf round-trip. The JSON conversion in libsimlin now goes directly from JSON -> native patch types -> datamodel, which is simpler and more maintainable. Additionally, this adds: - AddModel project operation for creating new models within a patch transaction, enabling atomic "add model + add module referencing it" - Comprehensive test coverage for module upsert, delete, replace, module references, and combined AddModel+UpsertModule operations - Test for patch rollback behavior on error - Fix pre-existing TS type error in core/datamodel.ts (List) https://claude.ai/code/session_011dCiV2VEYEPkUekXvBayyh --- src/core/datamodel.ts | 4 +- src/libsimlin/src/lib.rs | 177 ++---- src/simlin-engine/src/lib.rs | 2 +- src/simlin-engine/src/patch.rs | 1016 +++++++++++++++++++++----------- 4 files changed, 727 insertions(+), 472 deletions(-) diff --git a/src/core/datamodel.ts b/src/core/datamodel.ts index 10b36e26..13f7677a 100644 --- a/src/core/datamodel.ts +++ b/src/core/datamodel.ts @@ -238,7 +238,7 @@ function stockEquationFromJson( arrayedEquation: JsonArrayedEquation | undefined, ): Equation { if (arrayedEquation) { - const dimensionNames = List(arrayedEquation.dimensions ?? []); + const dimensionNames = List(arrayedEquation.dimensions ?? []); if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { return new ArrayedEquation({ dimensionNames, @@ -265,7 +265,7 @@ function auxEquationFromJson( } if (arrayedEquation) { - const dimensionNames = List(arrayedEquation.dimensions ?? []); + const dimensionNames = List(arrayedEquation.dimensions ?? []); if (arrayedEquation.elements && arrayedEquation.elements.length > 0) { return { equation: new ArrayedEquation({ diff --git a/src/libsimlin/src/lib.rs b/src/libsimlin/src/lib.rs index 2e029a50..bdc15e30 100644 --- a/src/libsimlin/src/lib.rs +++ b/src/libsimlin/src/lib.rs @@ -3095,23 +3095,14 @@ pub unsafe extern "C" fn simlin_project_serialize_protobuf( *out_len = len; } -/// Applies a patch to the project datamodel. -/// -/// The patch is encoded as a `project_io.Patch` protobuf message. The caller can -/// request a dry run (which performs validation without committing) and control -/// whether errors are permitted. When `allow_errors` is false, any static or -/// simulation error will cause the patch to be rejected. -/// -/// On success returns `SimlinErrorCode::NoError`. On failure returns an error -/// code describing why the patch could not be applied. When `out_errors` is not /// Internal helper that applies a ProjectPatch to a project. /// -/// This is the core patch application logic shared by both protobuf and JSON entry points. -/// It handles datamodel cloning, patch application, error gathering, validation, and -/// committing changes (unless dry_run is true). +/// This is the core patch application logic. It handles datamodel cloning, +/// patch application, error gathering, validation, and committing changes +/// (unless dry_run is true). unsafe fn apply_project_patch_internal( project_ref: &SimlinProject, - patch: engine::project_io::ProjectPatch, + patch: engine::ProjectPatch, dry_run: bool, allow_errors: bool, out_collected_errors: *mut *mut SimlinError, @@ -3393,13 +3384,7 @@ pub unsafe extern "C" fn simlin_project_apply_patch( } }; - let patch_proto = match convert_json_project_patch(json_patch) { - Ok(patch) => patch, - Err(err) => { - store_ffi_error(out_error, err); - return; - } - }; + let patch = convert_json_project_patch(json_patch); let project_ref = match require_project(project) { Ok(p) => p, @@ -3411,7 +3396,7 @@ pub unsafe extern "C" fn simlin_project_apply_patch( apply_project_patch_internal( project_ref, - patch_proto, + patch, dry_run, allow_errors, out_collected_errors, @@ -3427,6 +3412,9 @@ enum JsonProjectOperation { #[serde(rename = "simSpecs")] sim_specs: engine::json::SimSpecs, }, + AddModel { + name: String, + }, } #[cfg_attr(feature = "debug-derive", derive(Debug))] @@ -3484,137 +3472,72 @@ enum JsonModelOperation { }, } -fn convert_json_project_patch( - patch: JsonProjectPatch, -) -> Result { - let mut project_ops = Vec::with_capacity(patch.project_ops.len()); - for op in patch.project_ops { - let converted = convert_json_project_operation(op)?; - project_ops.push(engine::project_io::ProjectOperation { - op: Some(converted), - }); - } +fn convert_json_project_patch(patch: JsonProjectPatch) -> engine::ProjectPatch { + let project_ops = patch + .project_ops + .into_iter() + .map(convert_json_project_operation) + .collect(); - let mut model_patches = Vec::with_capacity(patch.models.len()); - for model in patch.models { - let mut ops = Vec::with_capacity(model.ops.len()); - for op in model.ops { - let converted = convert_json_model_operation(op)?; - ops.push(engine::project_io::ModelOperation { - op: Some(converted), - }); - } - model_patches.push(engine::project_io::ModelPatch { + let models = patch + .models + .into_iter() + .map(|model| engine::ModelPatch { name: model.name, - ops, - }); - } + ops: model + .ops + .into_iter() + .map(convert_json_model_operation) + .collect(), + }) + .collect(); - Ok(engine::project_io::ProjectPatch { + engine::ProjectPatch { project_ops, - models: model_patches, - }) + models, + } } -fn convert_json_project_operation( - op: JsonProjectOperation, -) -> Result { - use engine::datamodel; - use engine::project_io; - use engine::project_io::project_operation; - - let result = match op { +fn convert_json_project_operation(op: JsonProjectOperation) -> engine::ProjectOperation { + match op { JsonProjectOperation::SetSimSpecs { sim_specs } => { - let dm_sim_specs: datamodel::SimSpecs = sim_specs.into(); - let sim_specs_pb: project_io::SimSpecs = dm_sim_specs.into(); - project_operation::Op::SetSimSpecs(project_io::SetSimSpecsOp { - sim_specs: Some(sim_specs_pb), - }) + engine::ProjectOperation::SetSimSpecs(sim_specs.into()) } - }; - - Ok(result) + JsonProjectOperation::AddModel { name } => engine::ProjectOperation::AddModel { name }, + } } -fn convert_json_model_operation( - op: JsonModelOperation, -) -> Result { - use engine::datamodel; - use engine::project_io; - use engine::project_io::model_operation; - - let result = match op { - JsonModelOperation::UpsertAux { aux } => { - let dm_aux: datamodel::Aux = aux.into(); - let variable_pb = project_io::Variable::from(datamodel::Variable::Aux(dm_aux)); - let aux_pb = match variable_pb.v { - Some(project_io::variable::V::Aux(aux)) => aux, - _ => unreachable!(), - }; - model_operation::Op::UpsertAux(project_io::UpsertAuxOp { aux: Some(aux_pb) }) - } +fn convert_json_model_operation(op: JsonModelOperation) -> engine::ModelOperation { + match op { + JsonModelOperation::UpsertAux { aux } => engine::ModelOperation::UpsertAux(aux.into()), JsonModelOperation::UpsertStock { stock } => { - let dm_stock: datamodel::Stock = stock.into(); - let variable_pb = project_io::Variable::from(datamodel::Variable::Stock(dm_stock)); - let stock_pb = match variable_pb.v { - Some(project_io::variable::V::Stock(stock)) => stock, - _ => unreachable!(), - }; - model_operation::Op::UpsertStock(project_io::UpsertStockOp { - stock: Some(stock_pb), - }) - } - JsonModelOperation::UpsertFlow { flow } => { - let dm_flow: datamodel::Flow = flow.into(); - let variable_pb = project_io::Variable::from(datamodel::Variable::Flow(dm_flow)); - let flow_pb = match variable_pb.v { - Some(project_io::variable::V::Flow(flow)) => flow, - _ => unreachable!(), - }; - model_operation::Op::UpsertFlow(project_io::UpsertFlowOp { - flow: Some(flow_pb), - }) + engine::ModelOperation::UpsertStock(stock.into()) } + JsonModelOperation::UpsertFlow { flow } => engine::ModelOperation::UpsertFlow(flow.into()), JsonModelOperation::UpsertModule { module } => { - let dm_module: datamodel::Module = module.into(); - let variable_pb = project_io::Variable::from(datamodel::Variable::Module(dm_module)); - let module_pb = match variable_pb.v { - Some(project_io::variable::V::Module(module)) => module, - _ => unreachable!(), - }; - model_operation::Op::UpsertModule(project_io::UpsertModuleOp { - module: Some(module_pb), - }) + engine::ModelOperation::UpsertModule(module.into()) } JsonModelOperation::DeleteVariable { ident } => { - model_operation::Op::DeleteVariable(project_io::DeleteVariableOp { ident }) + engine::ModelOperation::DeleteVariable { ident } } JsonModelOperation::RenameVariable { from, to } => { - model_operation::Op::RenameVariable(project_io::RenameVariableOp { from, to }) - } - JsonModelOperation::UpsertView { index, view } => { - let dm_view: datamodel::View = view.into(); - let view_pb = project_io::View::from(dm_view); - model_operation::Op::UpsertView(project_io::UpsertViewOp { - index, - view: Some(view_pb), - }) - } - JsonModelOperation::DeleteView { index } => { - model_operation::Op::DeleteView(project_io::DeleteViewOp { index }) + engine::ModelOperation::RenameVariable { from, to } } + JsonModelOperation::UpsertView { index, view } => engine::ModelOperation::UpsertView { + index, + view: view.into(), + }, + JsonModelOperation::DeleteView { index } => engine::ModelOperation::DeleteView { index }, JsonModelOperation::UpdateStockFlows { ident, inflows, outflows, - } => model_operation::Op::UpdateStockFlows(project_io::UpdateStockFlowsOp { + } => engine::ModelOperation::UpdateStockFlows { ident, inflows, outflows, - }), - }; - - Ok(result) + }, + } } // Builder for error details used to populate SimlinError instances diff --git a/src/simlin-engine/src/lib.rs b/src/simlin-engine/src/lib.rs index 2a63f849..2dec14e5 100644 --- a/src/simlin-engine/src/lib.rs +++ b/src/simlin-engine/src/lib.rs @@ -61,7 +61,7 @@ pub mod xmile; pub use self::common::{Error, ErrorCode, ErrorKind, Result, canonicalize}; pub use self::interpreter::Simulation; pub use self::model::{ModelStage1, resolve_non_private_dependencies}; -pub use self::patch::apply_patch; +pub use self::patch::{ModelOperation, ModelPatch, ProjectOperation, ProjectPatch, apply_patch}; pub use self::project::Project; pub use self::results::{Method, Results, Specs as SimSpecs}; pub use self::variable::{Variable, identifier_set}; diff --git a/src/simlin-engine/src/patch.rs b/src/simlin-engine/src/patch.rs index 92254ae4..ac0086b1 100644 --- a/src/simlin-engine/src/patch.rs +++ b/src/simlin-engine/src/patch.rs @@ -10,76 +10,125 @@ use crate::common::{ }; use crate::datamodel::{self, Variable}; use crate::project::Project as CompiledProject; -use crate::project_io::{self, model_operation, project_operation}; -use crate::serde; use std::collections::HashMap; -macro_rules! require_field { - ($field:expr, $msg:expr) => {{ - let Some(value) = $field else { - return Err(Error::new( - ErrorKind::Model, - ErrorCode::ProtobufDecode, - Some($msg.to_string()), - )); - }; - value - }}; +/// A patch to apply to a project. Contains project-level operations +/// (like changing sim specs or adding models) and per-model patches +/// (like upserting variables or views). +#[derive(Clone)] +pub struct ProjectPatch { + pub project_ops: Vec, + pub models: Vec, } -pub fn apply_patch( - project: &mut datamodel::Project, - patch: &project_io::ProjectPatch, -) -> Result<()> { +/// A project-level operation. +#[derive(Clone)] +pub enum ProjectOperation { + SetSimSpecs(datamodel::SimSpecs), + SetSource(datamodel::Source), + AddModel { name: String }, +} + +/// A patch targeting a specific model within the project. +#[derive(Clone)] +pub struct ModelPatch { + pub name: String, + pub ops: Vec, +} + +/// An operation on a single model. +#[derive(Clone)] +pub enum ModelOperation { + UpsertStock(datamodel::Stock), + UpsertFlow(datamodel::Flow), + UpsertAux(datamodel::Aux), + UpsertModule(datamodel::Module), + DeleteVariable { + ident: String, + }, + RenameVariable { + from: String, + to: String, + }, + UpsertView { + index: u32, + view: datamodel::View, + }, + DeleteView { + index: u32, + }, + UpdateStockFlows { + ident: String, + inflows: Vec, + outflows: Vec, + }, +} + +pub fn apply_patch(project: &mut datamodel::Project, patch: &ProjectPatch) -> Result<()> { let mut staged = project.clone(); // Apply project-level operations first for project_op in &patch.project_ops { - let Some(kind) = &project_op.op else { - return Err(Error::new( - ErrorKind::Model, - ErrorCode::Generic, - Some("missing project operation".to_string()), - )); - }; - - match kind { - project_operation::Op::SetSimSpecs(specs) => apply_set_sim_specs(&mut staged, specs)?, - project_operation::Op::SetSource(op) => apply_set_source(&mut staged, op)?, + match project_op { + ProjectOperation::SetSimSpecs(sim_specs) => { + staged.sim_specs = sim_specs.clone(); + } + ProjectOperation::SetSource(source) => { + staged.source = Some(source.clone()); + } + ProjectOperation::AddModel { name } => { + apply_add_model(&mut staged, name)?; + } } } // Then apply model-level operations for model_patch in &patch.models { for op in &model_patch.ops { - let Some(kind) = &op.op else { - return Err(Error::new( - ErrorKind::Model, - ErrorCode::Generic, - Some("missing model operation".to_string()), - )); - }; - - match kind { - model_operation::Op::RenameVariable(op) => { - apply_rename_variable(&mut staged, &model_patch.name, op)? + match op { + ModelOperation::RenameVariable { from, to } => { + apply_rename_variable(&mut staged, &model_patch.name, from, to)?; } _ => { let model = get_model_mut(&mut staged, &model_patch.name)?; - match kind { - model_operation::Op::UpsertStock(op) => apply_upsert_stock(model, op)?, - model_operation::Op::UpsertFlow(op) => apply_upsert_flow(model, op)?, - model_operation::Op::UpsertAux(op) => apply_upsert_aux(model, op)?, - model_operation::Op::UpsertModule(op) => apply_upsert_module(model, op)?, - model_operation::Op::DeleteVariable(op) => { - apply_delete_variable(model, op)? + match op { + ModelOperation::UpsertStock(stock) => { + let mut stock = stock.clone(); + canonicalize_stock(&mut stock); + upsert_variable(model, Variable::Stock(stock)); + } + ModelOperation::UpsertFlow(flow) => { + let mut flow = flow.clone(); + canonicalize_flow(&mut flow); + upsert_variable(model, Variable::Flow(flow)); + } + ModelOperation::UpsertAux(aux) => { + let mut aux = aux.clone(); + canonicalize_aux(&mut aux); + upsert_variable(model, Variable::Aux(aux)); + } + ModelOperation::UpsertModule(module) => { + let mut module = module.clone(); + canonicalize_module(&mut module); + upsert_variable(model, Variable::Module(module)); + } + ModelOperation::DeleteVariable { ident } => { + apply_delete_variable(model, ident)?; + } + ModelOperation::UpsertView { index, view } => { + apply_upsert_view(model, *index, view)?; + } + ModelOperation::DeleteView { index } => { + apply_delete_view(model, *index)?; } - model_operation::Op::UpsertView(op) => apply_upsert_view(model, op)?, - model_operation::Op::DeleteView(op) => apply_delete_view(model, op)?, - model_operation::Op::UpdateStockFlows(op) => { - apply_update_stock_flows(model, op)? + ModelOperation::UpdateStockFlows { + ident, + inflows, + outflows, + } => { + apply_update_stock_flows(model, ident, inflows, outflows)?; } - model_operation::Op::RenameVariable(_) => unreachable!(), + ModelOperation::RenameVariable { .. } => unreachable!(), } } } @@ -141,81 +190,34 @@ fn get_model_mut<'a>( }) } -fn apply_set_sim_specs( - project: &mut datamodel::Project, - op: &project_io::SetSimSpecsOp, -) -> Result<()> { - let sim_specs = require_field!(&op.sim_specs, "missing sim_specs payload"); - - project.sim_specs.start = sim_specs.start; - project.sim_specs.stop = sim_specs.stop; - - if let Some(dt) = &sim_specs.dt { - project.sim_specs.dt = datamodel::Dt::from(*dt); - } - - if let Some(save_step) = &sim_specs.save_step { - project.sim_specs.save_step = Some(datamodel::Dt::from(*save_step)); - } else { - project.sim_specs.save_step = None; - } - - let sim_method = project_io::SimMethod::try_from(sim_specs.sim_method).unwrap_or_default(); - project.sim_specs.sim_method = datamodel::SimMethod::from(sim_method); - - if let Some(units) = &sim_specs.time_units { - if units.is_empty() { - project.sim_specs.time_units = None; - } else { - project.sim_specs.time_units = Some(units.clone()); - } - } else { - project.sim_specs.time_units = None; +fn apply_add_model(project: &mut datamodel::Project, name: &str) -> Result<()> { + let canonical_name = canonicalize(name); + // Check if a model with this name already exists + if project.get_model(canonical_name.as_str()).is_some() { + return Err(Error::new( + ErrorKind::Model, + ErrorCode::DuplicateVariable, + Some(format!("model '{}' already exists", name)), + )); } - - Ok(()) -} - -fn apply_upsert_stock(model: &mut datamodel::Model, op: &project_io::UpsertStockOp) -> Result<()> { - let stock_pb = require_field!(&op.stock, "missing stock payload"); - let mut stock = datamodel::Stock::from(stock_pb.clone()); - canonicalize_stock(&mut stock); - upsert_variable(model, Variable::Stock(stock)); - Ok(()) -} - -fn apply_upsert_flow(model: &mut datamodel::Model, op: &project_io::UpsertFlowOp) -> Result<()> { - let flow_pb = require_field!(&op.flow, "missing flow payload"); - let mut flow = datamodel::Flow::from(flow_pb.clone()); - canonicalize_flow(&mut flow); - upsert_variable(model, Variable::Flow(flow)); - Ok(()) -} - -fn apply_upsert_aux(model: &mut datamodel::Model, op: &project_io::UpsertAuxOp) -> Result<()> { - let aux_pb = require_field!(&op.aux, "missing auxiliary payload"); - let mut aux = datamodel::Aux::from(aux_pb.clone()); - canonicalize_aux(&mut aux); - upsert_variable(model, Variable::Aux(aux)); - Ok(()) -} - -fn apply_upsert_module( - model: &mut datamodel::Model, - op: &project_io::UpsertModuleOp, -) -> Result<()> { - let module_pb = require_field!(&op.module, "missing module payload"); - let mut module = datamodel::Module::from(module_pb.clone()); - canonicalize_module(&mut module); - upsert_variable(model, Variable::Module(module)); + project.models.push(datamodel::Model { + name: canonical_name.as_str().to_string(), + sim_specs: None, + variables: vec![], + views: vec![], + loop_metadata: vec![], + groups: vec![], + }); Ok(()) } fn apply_update_stock_flows( model: &mut datamodel::Model, - op: &project_io::UpdateStockFlowsOp, + ident_str: &str, + inflows: &[String], + outflows: &[String], ) -> Result<()> { - let ident = canonicalize(op.ident.as_str()); + let ident = canonicalize(ident_str); let stock = model .variables @@ -232,17 +234,15 @@ fn apply_update_stock_flows( Error::new( ErrorKind::Model, ErrorCode::DoesNotExist, - Some(format!("stock '{}' not found", op.ident)), + Some(format!("stock '{}' not found", ident_str)), ) })?; - stock.inflows = op - .inflows + stock.inflows = inflows .iter() .map(|s| canonicalize(s).into_string()) .collect(); - stock.outflows = op - .outflows + stock.outflows = outflows .iter() .map(|s| canonicalize(s).into_string()) .collect(); @@ -252,11 +252,8 @@ fn apply_update_stock_flows( Ok(()) } -fn apply_delete_variable( - model: &mut datamodel::Model, - op: &project_io::DeleteVariableOp, -) -> Result<()> { - let ident = canonicalize(op.ident.as_str()); +fn apply_delete_variable(model: &mut datamodel::Model, ident_str: &str) -> Result<()> { + let ident = canonicalize(ident_str); let Some(pos) = model .variables .iter() @@ -292,10 +289,11 @@ fn apply_delete_variable( fn apply_rename_variable( project: &mut datamodel::Project, model_name: &str, - op: &project_io::RenameVariableOp, + from: &str, + to: &str, ) -> Result<()> { - let old_ident = canonicalize(op.from.as_str()); - let new_ident = canonicalize(op.to.as_str()); + let old_ident = canonicalize(from); + let new_ident = canonicalize(to); if old_ident == new_ident { return Ok(()); @@ -896,17 +894,19 @@ fn update_stock_flow_references( } } -fn apply_upsert_view(model: &mut datamodel::Model, op: &project_io::UpsertViewOp) -> Result<()> { - let view_pb = require_field!(&op.view, "missing view payload"); - let view = serde::deserialize_view(view_pb.clone()); - let index = op.index as usize; +fn apply_upsert_view( + model: &mut datamodel::Model, + index: u32, + view: &datamodel::View, +) -> Result<()> { + let index = index as usize; if index < model.views.len() { - model.views[index] = view; + model.views[index] = view.clone(); Ok(()) } else if index == model.views.len() { // Allow appending at the end - model.views.push(view); + model.views.push(view.clone()); Ok(()) } else { Err(Error::new( @@ -917,8 +917,8 @@ fn apply_upsert_view(model: &mut datamodel::Model, op: &project_io::UpsertViewOp } } -fn apply_delete_view(model: &mut datamodel::Model, op: &project_io::DeleteViewOp) -> Result<()> { - let index = op.index as usize; +fn apply_delete_view(model: &mut datamodel::Model, index: u32) -> Result<()> { + let index = index as usize; if index < model.views.len() { model.views.remove(index); Ok(()) @@ -931,39 +931,12 @@ fn apply_delete_view(model: &mut datamodel::Model, op: &project_io::DeleteViewOp } } -fn apply_set_source(project: &mut datamodel::Project, op: &project_io::SetSourceOp) -> Result<()> { - let source = require_field!(&op.source, "missing source payload"); - project.source = Some(datamodel::Source::from(source.clone())); - Ok(()) -} - #[cfg(test)] mod tests { use super::*; use crate::datamodel::{self, Equation, Visibility}; - use crate::project_io::variable::V; - use crate::project_io::{ - self, ModelOperation, ModelPatch, ProjectOperation, ProjectPatch, model_operation, - project_operation, - }; use crate::test_common::TestProject; - fn stock_proto(stock: datamodel::Stock) -> project_io::variable::Stock { - let variable = Variable::Stock(stock); - match project_io::Variable::from(variable).v.unwrap() { - V::Stock(stock) => stock, - _ => unreachable!(), - } - } - - fn aux_proto(aux: datamodel::Aux) -> project_io::variable::Aux { - let variable = Variable::Aux(aux); - match project_io::Variable::from(variable).v.unwrap() { - V::Aux(aux) => aux, - _ => unreachable!(), - } - } - #[test] fn upsert_aux_adds_variable() { let mut project = TestProject::new("test").build_datamodel(); @@ -982,11 +955,7 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::UpsertAux(project_io::UpsertAuxOp { - aux: Some(aux_proto(aux.clone())), - })), - }], + ops: vec![ModelOperation::UpsertAux(aux.clone())], }], }; @@ -1004,7 +973,7 @@ mod tests { let mut project = TestProject::new("test") .stock("stock", "1", &[], &[], None) .build_datamodel(); - let mut stock = datamodel::Stock { + let stock = datamodel::Stock { ident: "stock".to_string(), equation: Equation::Scalar("5".to_string(), None), documentation: "docs".to_string(), @@ -1017,18 +986,11 @@ mod tests { ai_state: None, uid: Some(10), }; - stock.inflows.sort(); let patch = ProjectPatch { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::UpsertStock( - project_io::UpsertStockOp { - stock: Some(stock_proto(stock.clone())), - }, - )), - }], + ops: vec![ModelOperation::UpsertStock(stock.clone())], }], }; @@ -1056,12 +1018,8 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::DeleteVariable( - project_io::DeleteVariableOp { - ident: "flow".to_string(), - }, - )), + ops: vec![ModelOperation::DeleteVariable { + ident: "flow".to_string(), }], }], }; @@ -1088,13 +1046,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "flow".to_string(), - to: "new_flow".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "flow".to_string(), + to: "new_flow".to_string(), }], }], }; @@ -1116,26 +1070,18 @@ mod tests { } #[test] - fn set_sim_specs_partial_update() { + fn set_sim_specs() { let mut project = TestProject::new("test").build_datamodel(); + let new_specs = datamodel::SimSpecs { + start: 5.0, + stop: project.sim_specs.stop, + dt: datamodel::Dt::Dt(0.5), + save_step: None, + sim_method: datamodel::SimMethod::RungeKutta4, + time_units: Some("days".to_string()), + }; let patch = ProjectPatch { - project_ops: vec![ProjectOperation { - op: Some(project_operation::Op::SetSimSpecs( - project_io::SetSimSpecsOp { - sim_specs: Some(project_io::SimSpecs { - start: 5.0, - stop: project.sim_specs.stop, - dt: Some(project_io::Dt { - value: 0.5, - is_reciprocal: false, - }), - save_step: None, - sim_method: project_io::SimMethod::RungeKutta4 as i32, - time_units: Some("days".to_string()), - }), - }, - )), - }], + project_ops: vec![ProjectOperation::SetSimSpecs(new_specs)], models: vec![], }; @@ -1153,22 +1099,19 @@ mod tests { #[test] fn upsert_view_and_delete() { let mut project = TestProject::new("test").build_datamodel(); - let view = project_io::View { - kind: project_io::view::ViewType::StockFlow as i32, + let view = datamodel::View::StockFlow(datamodel::StockFlow { elements: vec![], - view_box: None, + view_box: datamodel::Rect::default(), zoom: 1.0, use_lettered_polarity: false, - }; + }); let patch = ProjectPatch { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::UpsertView(project_io::UpsertViewOp { - index: 0, - view: Some(view.clone()), - })), + ops: vec![ModelOperation::UpsertView { + index: 0, + view: view.clone(), }], }], }; @@ -1181,11 +1124,7 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::DeleteView(project_io::DeleteViewOp { - index: 0, - })), - }], + ops: vec![ModelOperation::DeleteView { index: 0 }], }], }; @@ -1198,14 +1137,10 @@ mod tests { fn set_source() { let mut project = TestProject::new("test").build_datamodel(); let patch = ProjectPatch { - project_ops: vec![ProjectOperation { - op: Some(project_operation::Op::SetSource(project_io::SetSourceOp { - source: Some(project_io::Source { - extension: project_io::source::Extension::Xmile as i32, - content: "hello".to_string(), - }), - })), - }], + project_ops: vec![ProjectOperation::SetSource(datamodel::Source { + extension: datamodel::Extension::Xmile, + content: "hello".to_string(), + })], models: vec![], }; @@ -1224,13 +1159,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "flow".to_string(), - to: "flow2".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "flow".to_string(), + to: "flow2".to_string(), }], }], }; @@ -1251,13 +1182,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "bar".to_string(), - to: "baz".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "bar".to_string(), + to: "baz".to_string(), }], }], }; @@ -1337,13 +1264,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "input".to_string(), - to: "new_input".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "input".to_string(), + to: "new_input".to_string(), }], }], }; @@ -1427,13 +1350,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "foo".to_string(), - to: "renamed_foo".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "foo".to_string(), + to: "renamed_foo".to_string(), }], }], }; @@ -1473,13 +1392,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "foo".to_string(), - to: "bar".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "foo".to_string(), + to: "bar".to_string(), }], }], }; @@ -1563,13 +1478,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "base_value".to_string(), - to: "initial_value".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "base_value".to_string(), + to: "initial_value".to_string(), }], }], }; @@ -1656,13 +1567,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "price".to_string(), - to: "unit_price".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "price".to_string(), + to: "unit_price".to_string(), }], }], }; @@ -1693,13 +1600,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "initial_stock".to_string(), - to: "starting_inventory".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "initial_stock".to_string(), + to: "starting_inventory".to_string(), }], }], }; @@ -1756,13 +1659,7 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::UpsertStock( - project_io::UpsertStockOp { - stock: Some(stock_proto(stock.clone())), - }, - )), - }], + ops: vec![ModelOperation::UpsertStock(stock.clone())], }], }; @@ -1793,19 +1690,14 @@ mod tests { _ => panic!("expected stock"), } - // Apply updateStockFlows to remove the inflow let patch = ProjectPatch { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::UpdateStockFlows( - project_io::UpdateStockFlowsOp { - ident: "inventory".to_string(), - inflows: vec![], - outflows: vec![], - }, - )), + ops: vec![ModelOperation::UpdateStockFlows { + ident: "inventory".to_string(), + inflows: vec![], + outflows: vec![], }], }], }; @@ -1815,10 +1707,8 @@ mod tests { let model = project.get_model("main").unwrap(); match model.get_variable("inventory").unwrap() { Variable::Stock(stock) => { - // Flows should be updated assert!(stock.inflows.is_empty()); assert!(stock.outflows.is_empty()); - // Equation should be preserved assert_eq!(stock.equation, Equation::Scalar("100".to_string(), None)); } _ => panic!("expected stock"), @@ -1836,26 +1726,21 @@ mod tests { &[], Some("people"), "Total population", - true, // non_negative - true, // can_be_module_input + true, + true, Visibility::Public, Some(42), ) .build_datamodel(); - // Disconnect the flow let patch = ProjectPatch { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::UpdateStockFlows( - project_io::UpdateStockFlowsOp { - ident: "population".to_string(), - inflows: vec![], - outflows: vec![], - }, - )), + ops: vec![ModelOperation::UpdateStockFlows { + ident: "population".to_string(), + inflows: vec![], + outflows: vec![], }], }], }; @@ -1865,10 +1750,8 @@ mod tests { let model = project.get_model("main").unwrap(); match model.get_variable("population").unwrap() { Variable::Stock(stock) => { - // Flows updated assert!(stock.inflows.is_empty()); assert!(stock.outflows.is_empty()); - // All other fields preserved assert_eq!(stock.equation, Equation::Scalar("1000".to_string(), None)); assert_eq!(stock.documentation, "Total population"); assert_eq!(stock.units, Some("people".to_string())); @@ -1889,14 +1772,10 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::UpdateStockFlows( - project_io::UpdateStockFlowsOp { - ident: "nonexistent".to_string(), - inflows: vec![], - outflows: vec![], - }, - )), + ops: vec![ModelOperation::UpdateStockFlows { + ident: "nonexistent".to_string(), + inflows: vec![], + outflows: vec![], }], }], }; @@ -1930,13 +1809,9 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::RenameVariable( - project_io::RenameVariableOp { - from: "alpha".to_string(), - to: "gamma".to_string(), - }, - )), + ops: vec![ModelOperation::RenameVariable { + from: "alpha".to_string(), + to: "gamma".to_string(), }], }], }; @@ -1973,12 +1848,8 @@ mod tests { project_ops: vec![], models: vec![ModelPatch { name: "main".to_string(), - ops: vec![ModelOperation { - op: Some(model_operation::Op::DeleteVariable( - project_io::DeleteVariableOp { - ident: "alpha".to_string(), - }, - )), + ops: vec![ModelOperation::DeleteVariable { + ident: "alpha".to_string(), }], }], }; @@ -1989,4 +1860,465 @@ mod tests { assert_eq!(model.groups.len(), 1); assert_eq!(model.groups[0].members, vec!["beta"]); } + + // --- New tests for module support and AddModel --- + + #[test] + fn add_model_creates_empty_model() { + let mut project = TestProject::new("test").build_datamodel(); + assert_eq!(project.models.len(), 1); + + let patch = ProjectPatch { + project_ops: vec![ProjectOperation::AddModel { + name: "submodel".to_string(), + }], + models: vec![], + }; + + apply_patch(&mut project, &patch).unwrap(); + assert_eq!(project.models.len(), 2); + let submodel = project.get_model("submodel").unwrap(); + assert!(submodel.variables.is_empty()); + assert!(submodel.views.is_empty()); + } + + #[test] + fn add_model_duplicate_returns_error() { + let mut project = TestProject::new("test").build_datamodel(); + + let patch = ProjectPatch { + project_ops: vec![ProjectOperation::AddModel { + name: "main".to_string(), + }], + models: vec![], + }; + + let err = apply_patch(&mut project, &patch).unwrap_err(); + assert_eq!(err.code, ErrorCode::DuplicateVariable); + } + + #[test] + fn upsert_module_adds_module_variable() { + let mut project = TestProject::new("test").build_datamodel(); + + // First add the submodel + project.models.push(datamodel::Model { + name: "submodel".to_string(), + sim_specs: None, + variables: vec![datamodel::Variable::Aux(datamodel::Aux { + ident: "output".to_string(), + equation: Equation::Scalar("42".to_string(), None), + documentation: String::new(), + units: None, + gf: None, + can_be_module_input: false, + visibility: Visibility::Public, + ai_state: None, + uid: None, + })], + views: vec![], + loop_metadata: vec![], + groups: vec![], + }); + + let module = datamodel::Module { + ident: "my_module".to_string(), + model_name: "submodel".to_string(), + documentation: "A test module".to_string(), + units: None, + references: vec![], + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: Some(100), + }; + + let patch = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertModule(module.clone())], + }], + }; + + apply_patch(&mut project, &patch).unwrap(); + let model = project.get_model("main").unwrap(); + match model.get_variable("my_module").unwrap() { + Variable::Module(m) => { + assert_eq!(m.model_name, "submodel"); + assert_eq!(m.documentation, "A test module"); + assert_eq!(m.uid, Some(100)); + } + _ => panic!("expected module"), + } + } + + #[test] + fn upsert_module_with_references() { + let mut project = TestProject::new("test") + .aux("local_input", "10", None) + .build_datamodel(); + + project.models.push(datamodel::Model { + name: "submodel".to_string(), + sim_specs: None, + variables: vec![datamodel::Variable::Aux(datamodel::Aux { + ident: "input_var".to_string(), + equation: Equation::Scalar("0".to_string(), None), + documentation: String::new(), + units: None, + gf: None, + can_be_module_input: true, + visibility: Visibility::Public, + ai_state: None, + uid: None, + })], + views: vec![], + loop_metadata: vec![], + groups: vec![], + }); + + let module = datamodel::Module { + ident: "my_module".to_string(), + model_name: "submodel".to_string(), + documentation: String::new(), + units: None, + references: vec![datamodel::ModuleReference { + src: "local_input".to_string(), + dst: "input_var".to_string(), + }], + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: None, + }; + + let patch = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertModule(module)], + }], + }; + + apply_patch(&mut project, &patch).unwrap(); + let model = project.get_model("main").unwrap(); + match model.get_variable("my_module").unwrap() { + Variable::Module(m) => { + assert_eq!(m.references.len(), 1); + assert_eq!(m.references[0].src, "local_input"); + assert_eq!(m.references[0].dst, "input_var"); + } + _ => panic!("expected module"), + } + } + + #[test] + fn upsert_module_replaces_existing() { + let mut project = TestProject::new("test").build_datamodel(); + project.models.push(datamodel::Model { + name: "submodel".to_string(), + sim_specs: None, + variables: vec![], + views: vec![], + loop_metadata: vec![], + groups: vec![], + }); + + // Add initial module + let initial_module = datamodel::Module { + ident: "my_module".to_string(), + model_name: "submodel".to_string(), + documentation: "initial".to_string(), + units: None, + references: vec![], + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: Some(1), + }; + let patch1 = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertModule(initial_module)], + }], + }; + apply_patch(&mut project, &patch1).unwrap(); + + // Now upsert with updated data + let updated_module = datamodel::Module { + ident: "my_module".to_string(), + model_name: "submodel".to_string(), + documentation: "updated".to_string(), + units: Some("widgets".to_string()), + references: vec![], + can_be_module_input: true, + visibility: Visibility::Public, + ai_state: None, + uid: Some(1), + }; + let patch2 = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertModule(updated_module)], + }], + }; + apply_patch(&mut project, &patch2).unwrap(); + + let model = project.get_model("main").unwrap(); + match model.get_variable("my_module").unwrap() { + Variable::Module(m) => { + assert_eq!(m.documentation, "updated"); + assert_eq!(m.units, Some("widgets".to_string())); + assert!(m.can_be_module_input); + assert_eq!(m.visibility, Visibility::Public); + } + _ => panic!("expected module"), + } + } + + #[test] + fn delete_module_variable() { + let mut project = TestProject::new("test").build_datamodel(); + project.models.push(datamodel::Model { + name: "submodel".to_string(), + sim_specs: None, + variables: vec![], + views: vec![], + loop_metadata: vec![], + groups: vec![], + }); + + let module = datamodel::Module { + ident: "my_module".to_string(), + model_name: "submodel".to_string(), + documentation: String::new(), + units: None, + references: vec![], + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: None, + }; + let add_patch = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertModule(module)], + }], + }; + apply_patch(&mut project, &add_patch).unwrap(); + assert!( + project + .get_model("main") + .unwrap() + .get_variable("my_module") + .is_some() + ); + + let delete_patch = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::DeleteVariable { + ident: "my_module".to_string(), + }], + }], + }; + apply_patch(&mut project, &delete_patch).unwrap(); + assert!( + project + .get_model("main") + .unwrap() + .get_variable("my_module") + .is_none() + ); + } + + #[test] + fn add_model_and_module_in_same_patch() { + let mut project = TestProject::new("test") + .aux("driver", "100", None) + .build_datamodel(); + + let module = datamodel::Module { + ident: "sub".to_string(), + model_name: "new_submodel".to_string(), + documentation: String::new(), + units: None, + references: vec![datamodel::ModuleReference { + src: "driver".to_string(), + dst: "input".to_string(), + }], + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: None, + }; + + let patch = ProjectPatch { + project_ops: vec![ProjectOperation::AddModel { + name: "new_submodel".to_string(), + }], + models: vec![ + // Add a variable to the new submodel + ModelPatch { + name: "new_submodel".to_string(), + ops: vec![ModelOperation::UpsertAux(datamodel::Aux { + ident: "input".to_string(), + equation: Equation::Scalar("0".to_string(), None), + documentation: String::new(), + units: None, + gf: None, + can_be_module_input: true, + visibility: Visibility::Public, + ai_state: None, + uid: None, + })], + }, + // Add the module reference to main + ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertModule(module)], + }, + ], + }; + + apply_patch(&mut project, &patch).unwrap(); + + // Verify submodel was created with variable + let submodel = project.get_model("new_submodel").unwrap(); + assert!(submodel.get_variable("input").is_some()); + + // Verify module was added to main + let main = project.get_model("main").unwrap(); + match main.get_variable("sub").unwrap() { + Variable::Module(m) => { + assert_eq!(m.model_name, "new_submodel"); + assert_eq!(m.references.len(), 1); + } + _ => panic!("expected module"), + } + } + + #[test] + fn rename_variable_updates_module_references_in_same_model() { + let mut project = TestProject::new("test") + .aux("old_name", "42", None) + .build_datamodel(); + + project.models.push(datamodel::Model { + name: "submodel".to_string(), + sim_specs: None, + variables: vec![datamodel::Variable::Aux(datamodel::Aux { + ident: "sub_input".to_string(), + equation: Equation::Scalar("0".to_string(), None), + documentation: String::new(), + units: None, + gf: None, + can_be_module_input: true, + visibility: Visibility::Public, + ai_state: None, + uid: None, + })], + views: vec![], + loop_metadata: vec![], + groups: vec![], + }); + + // Add a module that references old_name + let module = datamodel::Module { + ident: "child".to_string(), + model_name: "submodel".to_string(), + documentation: String::new(), + units: None, + references: vec![datamodel::ModuleReference { + src: "old_name".to_string(), + dst: "self.sub_input".to_string(), + }], + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: None, + }; + let add_module_patch = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertModule(module)], + }], + }; + apply_patch(&mut project, &add_module_patch).unwrap(); + + // Now rename old_name to new_name + let rename_patch = ProjectPatch { + project_ops: vec![], + models: vec![ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::RenameVariable { + from: "old_name".to_string(), + to: "new_name".to_string(), + }], + }], + }; + apply_patch(&mut project, &rename_patch).unwrap(); + + let model = project.get_model("main").unwrap(); + let module = model + .variables + .iter() + .find_map(|v| match v { + Variable::Module(m) => Some(m), + _ => None, + }) + .unwrap(); + assert_eq!(module.references[0].src, "new_name"); + } + + #[test] + fn patch_rollback_on_error() { + let mut project = TestProject::new("test") + .aux("x", "1", None) + .build_datamodel(); + + // Try a patch that adds a variable then operates on a nonexistent model + let patch = ProjectPatch { + project_ops: vec![], + models: vec![ + ModelPatch { + name: "main".to_string(), + ops: vec![ModelOperation::UpsertAux(datamodel::Aux { + ident: "y".to_string(), + equation: Equation::Scalar("2".to_string(), None), + documentation: String::new(), + units: None, + gf: None, + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: None, + })], + }, + ModelPatch { + name: "nonexistent_model".to_string(), + ops: vec![ModelOperation::DeleteVariable { + ident: "z".to_string(), + }], + }, + ], + }; + + let result = apply_patch(&mut project, &patch); + assert!(result.is_err()); + + // Project should be unchanged (rollback) + let model = project.get_model("main").unwrap(); + assert!( + model.get_variable("y").is_none(), + "y should not have been added on error" + ); + assert!(model.get_variable("x").is_some(), "x should still exist"); + } } From 0ebd249d9553960dbbce4bc98437b7e115eef5da Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 7 Feb 2026 03:05:16 +0000 Subject: [PATCH 2/2] engine: fix model name handling, take patch by value, restore fallible conversion Three issues addressed: 1. apply_add_model was the only code path that canonicalized model names before storing. XMILE import, JSON import, and the C FFI simlin_project_add_model all store names as-provided. Since get_model/get_model_mut use literal string equality, canonicalizing on add broke same-patch references using the original display name (e.g. "Customer Growth" would become "customer_growth" and then a ModelPatch targeting "Customer Growth" would fail with BadModelName). Now stores names as-provided, matching all other code paths. 2. apply_patch now takes ProjectPatch by value instead of by reference. This eliminates all .clone() calls on the operation payloads (Stock, Flow, Aux, Module, View, SimSpecs, Source) since we can move data directly out of the consumed patch. The Clone derive is removed from the patch types. 3. convert_json_project_patch in libsimlin returns Result again. While the current conversion arms are all infallible, returning Result preserves the ability to add validation without changing the call site, and matches the established pattern in the FFI layer. https://claude.ai/code/session_011dCiV2VEYEPkUekXvBayyh --- src/libsimlin/src/lib.rs | 66 +++++++------ src/simlin-engine/src/patch.rs | 171 ++++++++++++++++++++------------- 2 files changed, 145 insertions(+), 92 deletions(-) diff --git a/src/libsimlin/src/lib.rs b/src/libsimlin/src/lib.rs index bdc15e30..2ba4ea55 100644 --- a/src/libsimlin/src/lib.rs +++ b/src/libsimlin/src/lib.rs @@ -3120,7 +3120,7 @@ unsafe fn apply_project_patch_internal( project_locked.datamodel.clone() }; - if let Err(err) = engine::apply_patch(&mut staged_datamodel, &patch) { + if let Err(err) = engine::apply_patch(&mut staged_datamodel, patch) { store_error( out_error, SimlinError::new(SimlinErrorCode::from(err.code)) @@ -3384,7 +3384,13 @@ pub unsafe extern "C" fn simlin_project_apply_patch( } }; - let patch = convert_json_project_patch(json_patch); + let patch = match convert_json_project_patch(json_patch) { + Ok(patch) => patch, + Err(err) => { + store_ffi_error(out_error, err); + return; + } + }; let project_ref = match require_project(project) { Ok(p) => p, @@ -3472,43 +3478,48 @@ enum JsonModelOperation { }, } -fn convert_json_project_patch(patch: JsonProjectPatch) -> engine::ProjectPatch { - let project_ops = patch - .project_ops - .into_iter() - .map(convert_json_project_operation) - .collect(); +fn convert_json_project_patch( + patch: JsonProjectPatch, +) -> std::result::Result { + let mut project_ops = Vec::with_capacity(patch.project_ops.len()); + for op in patch.project_ops { + project_ops.push(convert_json_project_operation(op)?); + } - let models = patch - .models - .into_iter() - .map(|model| engine::ModelPatch { + let mut models = Vec::with_capacity(patch.models.len()); + for model in patch.models { + let mut ops = Vec::with_capacity(model.ops.len()); + for op in model.ops { + ops.push(convert_json_model_operation(op)?); + } + models.push(engine::ModelPatch { name: model.name, - ops: model - .ops - .into_iter() - .map(convert_json_model_operation) - .collect(), - }) - .collect(); + ops, + }); + } - engine::ProjectPatch { + Ok(engine::ProjectPatch { project_ops, models, - } + }) } -fn convert_json_project_operation(op: JsonProjectOperation) -> engine::ProjectOperation { - match op { +fn convert_json_project_operation( + op: JsonProjectOperation, +) -> std::result::Result { + let result = match op { JsonProjectOperation::SetSimSpecs { sim_specs } => { engine::ProjectOperation::SetSimSpecs(sim_specs.into()) } JsonProjectOperation::AddModel { name } => engine::ProjectOperation::AddModel { name }, - } + }; + Ok(result) } -fn convert_json_model_operation(op: JsonModelOperation) -> engine::ModelOperation { - match op { +fn convert_json_model_operation( + op: JsonModelOperation, +) -> std::result::Result { + let result = match op { JsonModelOperation::UpsertAux { aux } => engine::ModelOperation::UpsertAux(aux.into()), JsonModelOperation::UpsertStock { stock } => { engine::ModelOperation::UpsertStock(stock.into()) @@ -3537,7 +3548,8 @@ fn convert_json_model_operation(op: JsonModelOperation) -> engine::ModelOperatio inflows, outflows, }, - } + }; + Ok(result) } // Builder for error details used to populate SimlinError instances diff --git a/src/simlin-engine/src/patch.rs b/src/simlin-engine/src/patch.rs index ac0086b1..a1ba73df 100644 --- a/src/simlin-engine/src/patch.rs +++ b/src/simlin-engine/src/patch.rs @@ -15,14 +15,12 @@ use std::collections::HashMap; /// A patch to apply to a project. Contains project-level operations /// (like changing sim specs or adding models) and per-model patches /// (like upserting variables or views). -#[derive(Clone)] pub struct ProjectPatch { pub project_ops: Vec, pub models: Vec, } /// A project-level operation. -#[derive(Clone)] pub enum ProjectOperation { SetSimSpecs(datamodel::SimSpecs), SetSource(datamodel::Source), @@ -30,14 +28,12 @@ pub enum ProjectOperation { } /// A patch targeting a specific model within the project. -#[derive(Clone)] pub struct ModelPatch { pub name: String, pub ops: Vec, } /// An operation on a single model. -#[derive(Clone)] pub enum ModelOperation { UpsertStock(datamodel::Stock), UpsertFlow(datamodel::Flow), @@ -64,17 +60,17 @@ pub enum ModelOperation { }, } -pub fn apply_patch(project: &mut datamodel::Project, patch: &ProjectPatch) -> Result<()> { +pub fn apply_patch(project: &mut datamodel::Project, patch: ProjectPatch) -> Result<()> { let mut staged = project.clone(); // Apply project-level operations first - for project_op in &patch.project_ops { + for project_op in patch.project_ops { match project_op { ProjectOperation::SetSimSpecs(sim_specs) => { - staged.sim_specs = sim_specs.clone(); + staged.sim_specs = sim_specs; } ProjectOperation::SetSource(source) => { - staged.source = Some(source.clone()); + staged.source = Some(source); } ProjectOperation::AddModel { name } => { apply_add_model(&mut staged, name)?; @@ -83,50 +79,46 @@ pub fn apply_patch(project: &mut datamodel::Project, patch: &ProjectPatch) -> Re } // Then apply model-level operations - for model_patch in &patch.models { - for op in &model_patch.ops { + for model_patch in patch.models { + for op in model_patch.ops { match op { ModelOperation::RenameVariable { from, to } => { - apply_rename_variable(&mut staged, &model_patch.name, from, to)?; + apply_rename_variable(&mut staged, &model_patch.name, &from, &to)?; } _ => { let model = get_model_mut(&mut staged, &model_patch.name)?; match op { - ModelOperation::UpsertStock(stock) => { - let mut stock = stock.clone(); + ModelOperation::UpsertStock(mut stock) => { canonicalize_stock(&mut stock); upsert_variable(model, Variable::Stock(stock)); } - ModelOperation::UpsertFlow(flow) => { - let mut flow = flow.clone(); + ModelOperation::UpsertFlow(mut flow) => { canonicalize_flow(&mut flow); upsert_variable(model, Variable::Flow(flow)); } - ModelOperation::UpsertAux(aux) => { - let mut aux = aux.clone(); + ModelOperation::UpsertAux(mut aux) => { canonicalize_aux(&mut aux); upsert_variable(model, Variable::Aux(aux)); } - ModelOperation::UpsertModule(module) => { - let mut module = module.clone(); + ModelOperation::UpsertModule(mut module) => { canonicalize_module(&mut module); upsert_variable(model, Variable::Module(module)); } ModelOperation::DeleteVariable { ident } => { - apply_delete_variable(model, ident)?; + apply_delete_variable(model, &ident)?; } ModelOperation::UpsertView { index, view } => { - apply_upsert_view(model, *index, view)?; + apply_upsert_view(model, index, view)?; } ModelOperation::DeleteView { index } => { - apply_delete_view(model, *index)?; + apply_delete_view(model, index)?; } ModelOperation::UpdateStockFlows { ident, inflows, outflows, } => { - apply_update_stock_flows(model, ident, inflows, outflows)?; + apply_update_stock_flows(model, &ident, &inflows, &outflows)?; } ModelOperation::RenameVariable { .. } => unreachable!(), } @@ -190,10 +182,11 @@ fn get_model_mut<'a>( }) } -fn apply_add_model(project: &mut datamodel::Project, name: &str) -> Result<()> { - let canonical_name = canonicalize(name); - // Check if a model with this name already exists - if project.get_model(canonical_name.as_str()).is_some() { +fn apply_add_model(project: &mut datamodel::Project, name: String) -> Result<()> { + // Check if a model with this name already exists. + // Model names are stored and looked up as-is (no canonicalization), + // consistent with XMILE/JSON import and the C FFI simlin_project_add_model. + if project.get_model(&name).is_some() { return Err(Error::new( ErrorKind::Model, ErrorCode::DuplicateVariable, @@ -201,7 +194,7 @@ fn apply_add_model(project: &mut datamodel::Project, name: &str) -> Result<()> { )); } project.models.push(datamodel::Model { - name: canonical_name.as_str().to_string(), + name, sim_specs: None, variables: vec![], views: vec![], @@ -897,16 +890,16 @@ fn update_stock_flow_references( fn apply_upsert_view( model: &mut datamodel::Model, index: u32, - view: &datamodel::View, + view: datamodel::View, ) -> Result<()> { let index = index as usize; if index < model.views.len() { - model.views[index] = view.clone(); + model.views[index] = view; Ok(()) } else if index == model.views.len() { // Allow appending at the end - model.views.push(view.clone()); + model.views.push(view); Ok(()) } else { Err(Error::new( @@ -959,7 +952,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); let var = model.get_variable("new_aux").unwrap(); match var { @@ -994,7 +987,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); let var = model.get_variable("stock").unwrap(); match var { @@ -1024,7 +1017,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); assert!(model.get_variable("flow").is_none()); match model.get_variable("stock").unwrap() { @@ -1053,7 +1046,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); assert!(model.get_variable("flow").is_none()); match model.get_variable("new_flow").unwrap() { @@ -1085,7 +1078,7 @@ mod tests { models: vec![], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); assert_eq!(project.sim_specs.start, 5.0); assert_eq!(project.sim_specs.dt, datamodel::Dt::Dt(0.5)); assert!(project.sim_specs.save_step.is_none()); @@ -1116,7 +1109,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); assert_eq!(model.views.len(), 1); @@ -1128,7 +1121,7 @@ mod tests { }], }; - apply_patch(&mut project, &delete_patch).unwrap(); + apply_patch(&mut project, delete_patch).unwrap(); let model = project.get_model("main").unwrap(); assert!(model.views.is_empty()); } @@ -1144,7 +1137,7 @@ mod tests { models: vec![], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); assert!(project.source.is_some()); assert_eq!(project.source.as_ref().unwrap().content, "hello"); } @@ -1166,7 +1159,7 @@ mod tests { }], }; - let err = apply_patch(&mut project, &patch).unwrap_err(); + let err = apply_patch(&mut project, patch).unwrap_err(); assert_eq!(err.code, ErrorCode::DuplicateVariable); assert_eq!(err.kind, ErrorKind::Model); } @@ -1189,7 +1182,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("foo").unwrap() { @@ -1271,7 +1264,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("consumer").unwrap() { @@ -1357,7 +1350,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("consumer").unwrap() { @@ -1399,7 +1392,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("consumer").unwrap() { @@ -1485,7 +1478,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("regional_growth").unwrap() { @@ -1574,7 +1567,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("revenue").unwrap() { @@ -1607,7 +1600,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("inventory").unwrap() { @@ -1663,7 +1656,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); let var = model.get_variable("inventory").unwrap(); @@ -1702,7 +1695,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("inventory").unwrap() { @@ -1745,7 +1738,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("population").unwrap() { @@ -1780,7 +1773,7 @@ mod tests { }], }; - let err = apply_patch(&mut project, &patch).unwrap_err(); + let err = apply_patch(&mut project, patch).unwrap_err(); assert_eq!(err.code, ErrorCode::DoesNotExist); } @@ -1816,7 +1809,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); assert_eq!(model.groups.len(), 1); @@ -1854,7 +1847,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); assert_eq!(model.groups.len(), 1); @@ -1875,7 +1868,7 @@ mod tests { models: vec![], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); assert_eq!(project.models.len(), 2); let submodel = project.get_model("submodel").unwrap(); assert!(submodel.variables.is_empty()); @@ -1893,7 +1886,7 @@ mod tests { models: vec![], }; - let err = apply_patch(&mut project, &patch).unwrap_err(); + let err = apply_patch(&mut project, patch).unwrap_err(); assert_eq!(err.code, ErrorCode::DuplicateVariable); } @@ -1941,7 +1934,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("my_module").unwrap() { Variable::Module(m) => { @@ -2001,7 +1994,7 @@ mod tests { }], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("my_module").unwrap() { Variable::Module(m) => { @@ -2044,7 +2037,7 @@ mod tests { ops: vec![ModelOperation::UpsertModule(initial_module)], }], }; - apply_patch(&mut project, &patch1).unwrap(); + apply_patch(&mut project, patch1).unwrap(); // Now upsert with updated data let updated_module = datamodel::Module { @@ -2065,7 +2058,7 @@ mod tests { ops: vec![ModelOperation::UpsertModule(updated_module)], }], }; - apply_patch(&mut project, &patch2).unwrap(); + apply_patch(&mut project, patch2).unwrap(); let model = project.get_model("main").unwrap(); match model.get_variable("my_module").unwrap() { @@ -2109,7 +2102,7 @@ mod tests { ops: vec![ModelOperation::UpsertModule(module)], }], }; - apply_patch(&mut project, &add_patch).unwrap(); + apply_patch(&mut project, add_patch).unwrap(); assert!( project .get_model("main") @@ -2127,7 +2120,7 @@ mod tests { }], }], }; - apply_patch(&mut project, &delete_patch).unwrap(); + apply_patch(&mut project, delete_patch).unwrap(); assert!( project .get_model("main") @@ -2186,7 +2179,7 @@ mod tests { ], }; - apply_patch(&mut project, &patch).unwrap(); + apply_patch(&mut project, patch).unwrap(); // Verify submodel was created with variable let submodel = project.get_model("new_submodel").unwrap(); @@ -2250,7 +2243,7 @@ mod tests { ops: vec![ModelOperation::UpsertModule(module)], }], }; - apply_patch(&mut project, &add_module_patch).unwrap(); + apply_patch(&mut project, add_module_patch).unwrap(); // Now rename old_name to new_name let rename_patch = ProjectPatch { @@ -2263,7 +2256,7 @@ mod tests { }], }], }; - apply_patch(&mut project, &rename_patch).unwrap(); + apply_patch(&mut project, rename_patch).unwrap(); let model = project.get_model("main").unwrap(); let module = model @@ -2310,7 +2303,7 @@ mod tests { ], }; - let result = apply_patch(&mut project, &patch); + let result = apply_patch(&mut project, patch); assert!(result.is_err()); // Project should be unchanged (rollback) @@ -2321,4 +2314,52 @@ mod tests { ); assert!(model.get_variable("x").is_some(), "x should still exist"); } + + #[test] + fn add_model_preserves_display_name() { + let mut project = TestProject::new("test").build_datamodel(); + + let patch = ProjectPatch { + project_ops: vec![ProjectOperation::AddModel { + name: "Customer Growth".to_string(), + }], + models: vec![], + }; + + apply_patch(&mut project, patch).unwrap(); + assert_eq!(project.models.len(), 2); + // The model should be stored with its display name, not canonicalized + assert_eq!(project.models[1].name, "Customer Growth"); + // And we should be able to find it by its display name + assert!(project.get_model("Customer Growth").is_some()); + } + + #[test] + fn add_model_and_operate_on_it_in_same_patch_with_display_name() { + let mut project = TestProject::new("test").build_datamodel(); + + let patch = ProjectPatch { + project_ops: vec![ProjectOperation::AddModel { + name: "Customer Growth".to_string(), + }], + models: vec![ModelPatch { + name: "Customer Growth".to_string(), + ops: vec![ModelOperation::UpsertAux(datamodel::Aux { + ident: "growth_rate".to_string(), + equation: Equation::Scalar("0.05".to_string(), None), + documentation: String::new(), + units: None, + gf: None, + can_be_module_input: false, + visibility: Visibility::Private, + ai_state: None, + uid: None, + })], + }], + }; + + apply_patch(&mut project, patch).unwrap(); + let model = project.get_model("Customer Growth").unwrap(); + assert!(model.get_variable("growth_rate").is_some()); + } }