From 269792e9743ceb5b87156e7f2914a66ef278bcc8 Mon Sep 17 00:00:00 2001 From: dyrpsf Date: Wed, 7 Jan 2026 09:14:16 +0530 Subject: [PATCH 1/3] Avoid duplicate model elements in ModelBuilder by reusing existing IDs --- .../src/org/sbml/jsbml/util/ModelBuilder.java | 117 ++++++++++++------ 1 file changed, 78 insertions(+), 39 deletions(-) diff --git a/core/src/org/sbml/jsbml/util/ModelBuilder.java b/core/src/org/sbml/jsbml/util/ModelBuilder.java index 974fad755..dbc4d12b2 100644 --- a/core/src/org/sbml/jsbml/util/ModelBuilder.java +++ b/core/src/org/sbml/jsbml/util/ModelBuilder.java @@ -250,16 +250,22 @@ public void buildCBMunits() { * @param sizeUnits * @return */ - public Compartment buildCompartment(String id, boolean constant, String name, double spatialDimensions, double size, String sizeUnits) { + public Compartment buildCompartment(String id, boolean constant, String name, + double spatialDimensions, double size, String sizeUnits) { Model model = getModel(); - Compartment c = model.createCompartment(id); - c.setConstant(constant); - c.setName(name); - c.setSpatialDimensions(spatialDimensions); - c.setSize(size); - if (sizeUnits != null) { - c.setUnits(sizeUnits); + Compartment c = model.getCompartment(id); + + if (c == null) { + c = model.createCompartment(id); + c.setConstant(constant); + c.setName(name); + c.setSpatialDimensions(spatialDimensions); + c.setSize(size); + if (sizeUnits != null) { + c.setUnits(sizeUnits); + } } + return c; } @@ -298,8 +304,18 @@ public Compartment buildCompartment(String id, boolean constant, String name, do * @return */ public Model buildModel(String id, String name) { - Model model = doc.createModel(id); - model.setName(name); + Model model; + + if (doc.isSetModel()) { + model = doc.getModel(); + } else { + model = doc.createModel(id); + } + + if (name != null) { + model.setName(name); + } + return model; } @@ -312,12 +328,19 @@ public Model buildModel(String id, String name) { * @param units * @return */ - public Parameter buildParameter(String id, String name, double value, boolean constant, String units) { - Parameter p = getModel().createParameter(id); - p.setName(name); - p.setValue(value); - p.setConstant(constant); - p.setUnits(units); + public Parameter buildParameter(String id, String name, double value, + boolean constant, String units) { + Model model = getModel(); + Parameter p = model.getParameter(id); + + if (p == null) { + p = model.createParameter(id); + p.setName(name); + p.setValue(value); + p.setConstant(constant); + p.setUnits(units); + } + return p; } @@ -369,15 +392,21 @@ public Reaction buildReaction(String id, String name, Compartment compartment, b * @param reversible * @return */ - public Reaction buildReaction(String id, String name, String compartment, boolean fast, boolean reversible) { + public Reaction buildReaction(String id, String name, String compartment, + boolean fast, boolean reversible) { Model model = getModel(); - Reaction r = model.createReaction(id); - r.setName(name); - if (compartment != null) { - r.setCompartment(compartment); + Reaction r = model.getReaction(id); + + if (r == null) { + r = model.createReaction(id); + r.setName(name); + if (compartment != null) { + r.setCompartment(compartment); + } + r.setFast(fast); + r.setReversible(reversible); } - r.setFast(fast); - r.setReversible(reversible); + return r; } @@ -451,18 +480,23 @@ public Species buildSpecies(String id, String name, * @return */ public Species buildSpecies(String id, String name, - String compartmentId, boolean hasOnlySubstanceUnits, - boolean boundaryCondition, boolean constant, double initialConcentration, - String substanceUnits) { + String compartmentId, boolean hasOnlySubstanceUnits, + boolean boundaryCondition, boolean constant, double initialConcentration, + String substanceUnits) { Model model = getModel(); - Species s = model.createSpecies(id); - s.setName(name); - s.setCompartment(compartmentId); - s.setHasOnlySubstanceUnits(hasOnlySubstanceUnits); - s.setBoundaryCondition(boundaryCondition); - s.setConstant(constant); - s.setInitialConcentration(initialConcentration); - s.setSubstanceUnits(substanceUnits); + Species s = model.getSpecies(id); + + if (s == null) { + s = model.createSpecies(id); + s.setName(name); + s.setCompartment(compartmentId); + s.setHasOnlySubstanceUnits(hasOnlySubstanceUnits); + s.setBoundaryCondition(boundaryCondition); + s.setConstant(constant); + s.setInitialConcentration(initialConcentration); + s.setSubstanceUnits(substanceUnits); + } + return s; } @@ -483,13 +517,18 @@ public Unit buildUnit(double multiplier, int scale, Kind kind, double exponent) */ public UnitDefinition buildUnitDefinition(String id, String name, Unit... units) { Model model = getModel(); - UnitDefinition ud = model.createUnitDefinition(id); - ud.setName(name); - if (units != null) { - for (Unit unit : units) { - ud.addUnit(unit); + UnitDefinition ud = model.getUnitDefinition(id); + + if (ud == null) { + ud = model.createUnitDefinition(id); + ud.setName(name); + if (units != null) { + for (Unit unit : units) { + ud.addUnit(unit); + } } } + return ud; } From 2bad80bd246d33dde6f5cd9478e535d7f76b6716 Mon Sep 17 00:00:00 2001 From: dyrpsf Date: Thu, 12 Mar 2026 09:36:42 +0530 Subject: [PATCH 2/3] Clarify ModelBuilder semantics and guard against id clashes (#272) --- .../src/org/sbml/jsbml/util/ModelBuilder.java | 195 +++++++++++++++--- 1 file changed, 163 insertions(+), 32 deletions(-) diff --git a/core/src/org/sbml/jsbml/util/ModelBuilder.java b/core/src/org/sbml/jsbml/util/ModelBuilder.java index dbc4d12b2..8a8a44ddf 100644 --- a/core/src/org/sbml/jsbml/util/ModelBuilder.java +++ b/core/src/org/sbml/jsbml/util/ModelBuilder.java @@ -36,6 +36,7 @@ import org.sbml.jsbml.Unit.Kind; import org.sbml.jsbml.UnitDefinition; import org.sbml.jsbml.text.parser.ParseException; +import org.sbml.jsbml.SBase; /** * This class provides a collection of convenient methods to create SBML models @@ -250,22 +251,58 @@ public void buildCBMunits() { * @param sizeUnits * @return */ + /** + * Creates or returns a {@link Compartment} with the given identifier. + *

+ * If a compartment with the given {@code id} already exists in the underlying + * {@link Model}, this method returns that compartment unchanged and does not + * apply the other arguments. If no such compartment exists, a new one is created + * and the provided arguments are used to initialise its attributes. + *

+ * If an element with the same {@code id} exists in the model but is not a + * {@link Compartment}, an {@link IllegalArgumentException} is thrown. + * + * @param id + * the id of the compartment to create or return + * @param constant + * value for {@code constant} if a new compartment is created + * @param name + * value for {@code name} if a new compartment is created + * @param spatialDimensions + * value for {@code spatialDimensions} if a new compartment is created + * @param size + * value for {@code size} if a new compartment is created + * @param sizeUnits + * units for {@code size} if a new compartment is created + * @return the existing or newly created {@link Compartment} + */ public Compartment buildCompartment(String id, boolean constant, String name, double spatialDimensions, double size, String sizeUnits) { Model model = getModel(); Compartment c = model.getCompartment(id); - if (c == null) { - c = model.createCompartment(id); - c.setConstant(constant); - c.setName(name); - c.setSpatialDimensions(spatialDimensions); - c.setSize(size); - if (sizeUnits != null) { - c.setUnits(sizeUnits); + if (c != null) { + return c; + } + + if (id != null) { + SBase existing = model.findNamedSBase(id); + if (existing != null && !(existing instanceof Compartment)) { + throw new IllegalArgumentException( + "Element with id '" + id + "' already exists and is of type " + + existing.getElementName() + ", not a compartment."); } } + c = model.createCompartment(id); + c.setConstant(constant); + c.setName(name); + c.setSpatialDimensions(spatialDimensions); + c.setSize(size); + if (sizeUnits != null) { + c.setUnits(sizeUnits); + } + return c; } @@ -298,10 +335,18 @@ public Compartment buildCompartment(String id, boolean constant, String name, do } /** + * Creates or returns the {@link Model} associated with this {@link SBMLDocument}. + *

+ * If the document already contains a model, this method returns that model unchanged + * and does not alter its identifier. Otherwise, a new model is created with the + * given {@code id}. If {@code name} is non-null, it is applied as the model's + * name (only when the model is created or does not yet have a name). * * @param id + * the id to use if a new model is created * @param name - * @return + * an optional name to assign + * @return the existing or newly created {@link Model} */ public Model buildModel(String id, String name) { Model model; @@ -312,7 +357,7 @@ public Model buildModel(String id, String name) { model = doc.createModel(id); } - if (name != null) { + if (name != null && !model.isSetName()) { model.setName(name); } @@ -328,19 +373,41 @@ public Model buildModel(String id, String name) { * @param units * @return */ + /** + * Creates or returns a {@link Parameter} with the given identifier. + *

+ * If a parameter with the given {@code id} already exists in the underlying + * {@link Model}, this method returns that parameter unchanged and does not + * apply the other arguments. If no such parameter exists, a new one is + * created and the provided arguments are used to initialise its attributes. + *

+ * If an element with the same {@code id} exists in the model but is not a + * {@link Parameter}, an {@link IllegalArgumentException} is thrown. + */ public Parameter buildParameter(String id, String name, double value, boolean constant, String units) { Model model = getModel(); Parameter p = model.getParameter(id); - if (p == null) { - p = model.createParameter(id); - p.setName(name); - p.setValue(value); - p.setConstant(constant); - p.setUnits(units); + if (p != null) { + return p; + } + + if (id != null) { + SBase existing = model.findNamedSBase(id); + if (existing != null && !(existing instanceof Parameter)) { + throw new IllegalArgumentException( + "Element with id '" + id + "' already exists and is of type " + + existing.getElementName() + ", not a parameter."); + } } + p = model.createParameter(id); + p.setName(name); + p.setValue(value); + p.setConstant(constant); + p.setUnits(units); + return p; } @@ -392,21 +459,43 @@ public Reaction buildReaction(String id, String name, Compartment compartment, b * @param reversible * @return */ + /** + * Creates or returns a {@link Reaction} with the given identifier. + *

+ * If a reaction with the given {@code id} already exists in the underlying + * {@link Model}, this method returns that reaction unchanged and does not + * apply the other arguments. If no such reaction exists, a new one is created + * and the provided arguments are used to initialise its attributes. + *

+ * If an element with the same {@code id} exists in the model but is not a + * {@link Reaction}, an {@link IllegalArgumentException} is thrown. + */ public Reaction buildReaction(String id, String name, String compartment, boolean fast, boolean reversible) { Model model = getModel(); Reaction r = model.getReaction(id); - if (r == null) { - r = model.createReaction(id); - r.setName(name); - if (compartment != null) { - r.setCompartment(compartment); + if (r != null) { + return r; + } + + if (id != null) { + SBase existing = model.findNamedSBase(id); + if (existing != null && !(existing instanceof Reaction)) { + throw new IllegalArgumentException( + "Element with id '" + id + "' already exists and is of type " + + existing.getElementName() + ", not a reaction."); } - r.setFast(fast); - r.setReversible(reversible); } + r = model.createReaction(id); + r.setName(name); + if (compartment != null) { + r.setCompartment(compartment); + } + r.setFast(fast); + r.setReversible(reversible); + return r; } @@ -479,6 +568,17 @@ public Species buildSpecies(String id, String name, * @param substanceUnits * @return */ + /** + * Creates or returns a {@link Species} with the given identifier. + *

+ * If a species with the given {@code id} already exists in the underlying + * {@link Model}, this method returns that species unchanged and does not + * apply the other arguments. If no such species exists, a new one is created + * and the provided arguments are used to initialise its attributes. + *

+ * If an element with the same {@code id} exists in the model but is not a + * {@link Species}, an {@link IllegalArgumentException} is thrown. + */ public Species buildSpecies(String id, String name, String compartmentId, boolean hasOnlySubstanceUnits, boolean boundaryCondition, boolean constant, double initialConcentration, @@ -486,17 +586,28 @@ public Species buildSpecies(String id, String name, Model model = getModel(); Species s = model.getSpecies(id); - if (s == null) { - s = model.createSpecies(id); - s.setName(name); - s.setCompartment(compartmentId); - s.setHasOnlySubstanceUnits(hasOnlySubstanceUnits); - s.setBoundaryCondition(boundaryCondition); - s.setConstant(constant); - s.setInitialConcentration(initialConcentration); - s.setSubstanceUnits(substanceUnits); + if (s != null) { + return s; } + if (id != null) { + SBase existing = model.findNamedSBase(id); + if (existing != null && !(existing instanceof Species)) { + throw new IllegalArgumentException( + "Element with id '" + id + "' already exists and is of type " + + existing.getElementName() + ", not a species."); + } + } + + s = model.createSpecies(id); + s.setName(name); + s.setCompartment(compartmentId); + s.setHasOnlySubstanceUnits(hasOnlySubstanceUnits); + s.setBoundaryCondition(boundaryCondition); + s.setConstant(constant); + s.setInitialConcentration(initialConcentration); + s.setSubstanceUnits(substanceUnits); + return s; } @@ -515,6 +626,26 @@ public Unit buildUnit(double multiplier, int scale, Kind kind, double exponent) * @param name * @return */ + /** + * Creates or returns a {@link UnitDefinition} with the given identifier. + *

+ * If a unit definition with the given {@code id} already exists in the + * underlying {@link Model}, this method returns that unit definition unchanged + * and does not apply the other arguments. If no such unit definition exists, + * a new one is created and the provided arguments are used to initialise its + * attributes. + *

+ * Note that {@link UnitDefinition} objects live in their own namespace and + * may have the same id as SId-based elements without causing clashes. + * + * @param id + * the id of the unit definition + * @param name + * the optional name to assign + * @param units + * optional units to add if a new definition is created + * @return the existing or newly created {@link UnitDefinition} + */ public UnitDefinition buildUnitDefinition(String id, String name, Unit... units) { Model model = getModel(); UnitDefinition ud = model.getUnitDefinition(id); From 41d6910f4417d8af8f7bdf8047b1cecc23b6ea0a Mon Sep 17 00:00:00 2001 From: dyrpsf Date: Thu, 12 Mar 2026 09:44:28 +0530 Subject: [PATCH 3/3] Add regression tests for ModelBuilder reuse and id clash detection (#272) --- .../jsbml/util/ModelBuilderReuseTest.java | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) create mode 100644 core/test/org/sbml/jsbml/util/ModelBuilderReuseTest.java diff --git a/core/test/org/sbml/jsbml/util/ModelBuilderReuseTest.java b/core/test/org/sbml/jsbml/util/ModelBuilderReuseTest.java new file mode 100644 index 000000000..c10b6d1d9 --- /dev/null +++ b/core/test/org/sbml/jsbml/util/ModelBuilderReuseTest.java @@ -0,0 +1,91 @@ +/* + * ---------------------------------------------------------------------------- + * This file is part of JSBML. Please visit + * for the latest version of JSBML and more information about SBML. + * + * Copyright (C) 2009-2022 jointly by the following organizations: + * 1. The University of Tuebingen, Germany + * 2. EMBL European Bioinformatics Institute (EBML-EBI), Hinxton, UK + * 3. The California Institute of Technology, Pasadena, CA, USA + * 4. The University of California, San Diego, La Jolla, CA, USA + * 5. The Babraham Institute, Cambridge, UK + * + * This library is free software; you can redistribute it and/or modify it + * under the terms of the GNU Lesser General Public License as published by + * the Free Software Foundation. A copy of the license agreement is provided + * in the file named "LICENSE.txt" included with this software distribution + * and also available online as . + * ---------------------------------------------------------------------------- + */ + +package org.sbml.jsbml.util; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertSame; + +import org.junit.Test; +import org.sbml.jsbml.Compartment; +import org.sbml.jsbml.Model; +import org.sbml.jsbml.SBMLDocument; +import org.sbml.jsbml.Species; + +/** + * Tests for the reuse and clash-detection behaviour in {@link ModelBuilder}. + */ +public class ModelBuilderReuseTest { + + /** + * If a compartment with the given id already exists, buildCompartment should + * return it unchanged and not overwrite its attributes. + */ + @Test + public void reusesExistingCompartmentWithoutOverwritingAttributes() { + SBMLDocument doc = new SBMLDocument(3, 1); + Model model = doc.createModel("m"); + + Compartment original = model.createCompartment("c1"); + original.setName("original"); + original.setSpatialDimensions(3d); + original.setSize(1.23); + original.setUnits("litre"); + original.setConstant(false); + + ModelBuilder builder = new ModelBuilder(doc); + + // Call with different arguments – these should NOT be applied, + // because the compartment already exists. + Compartment result = builder.buildCompartment("c1", true, "newName", + 1d, 9.99, "otherUnits"); + + assertSame(original, result); + assertEquals("original", result.getName()); + assertEquals(3d, result.getSpatialDimensions(), 0.0); + assertEquals(1.23, result.getSize(), 0.0); + assertEquals("litre", result.getUnits()); + // remains false, not overwritten by the 'true' passed in + assertEquals(false, result.getConstant()); + } + + /** + * If an element with the same id exists but is of a different type, + * ModelBuilder should throw an IllegalArgumentException instead of + * silently creating a new element. + */ + @Test(expected = IllegalArgumentException.class) + public void throwsIfIdIsUsedByDifferentType() { + SBMLDocument doc = new SBMLDocument(3, 1); + Model model = doc.createModel("m"); + + // Create a species with id 'shared' + Species species = model.createSpecies("shared"); + species.setCompartment("c"); + species.setInitialConcentration(1.0); + + ModelBuilder builder = new ModelBuilder(doc); + + // Attempt to create a compartment with the same id. + // Our new implementation should detect the clash via findNamedSBase + // and throw an IllegalArgumentException. + builder.buildCompartment("shared", true, "compartment", 3d, 1.0, "litre"); + } +} \ No newline at end of file