Fix OverdeterminationValidator for L3V2 rateOf csymbol

core/src/org/sbml/jsbml/validator/OverdeterminationValidator.java

@@ -38,13 +38,17 @@
 import org.sbml.jsbml.ListOf;
 import org.sbml.jsbml.LocalParameter;
 import org.sbml.jsbml.Model;
+import org.sbml.jsbml.NamedSBase;
 import org.sbml.jsbml.Parameter;
 import org.sbml.jsbml.RateRule;
 import org.sbml.jsbml.Reaction;
 import org.sbml.jsbml.Rule;
 import org.sbml.jsbml.SBase;
 import org.sbml.jsbml.Species;
 import org.sbml.jsbml.SpeciesReference;
+import org.w3c.dom.Node;
+
+import com.thoughtworks.xstream.io.binary.Token.StartNode;
 
 /**
  * This class creates a bipartite graph and a matching for the given model using
@@ -813,6 +817,42 @@ public Map<SBase, SBase> getMatching() {
     return matching;
   }
 
+  /**
+   * Resolve an identifier from a MathML {@link ASTNode} to the
+   * corresponding {@link SBase} object in the model.
+   *
+   * This is in particular needed for SBML Level&nbsp;3 Version&nbsp;2
+   * {@code rateOf} csymbols, where the {@link ASTNode#getVariable()}
+   * may not yet be initialized.
+   *
+   * Only model-level {@link Species}, {@link Compartment},
+   * {@link Parameter}, and {@link Reaction} objects are considered.
+   *
+   * @param id the SBML id of the object
+   * @return the corresponding {@link SBase}, or {@code null} if none
+   *         can be found or if the id refers to another type of
+   *         {@link NamedSBase}.
+   */
+  private SBase resolveSBaseFromId(String id) {
+    if (model == null || id == null) {
+      return null;
+    }
+
+    NamedSBase named = model.findNamedSBase(id);
+    if (named == null) {
+      return null;
+    }
+
+    if (named instanceof Species
+        || named instanceof Compartment
+        || named instanceof Parameter
+        || named instanceof Reaction) {
+      return named;
+    }
+
+    return null;
+  }
+
   /**
    * Returns the variables in a MathML object without local parameter
    * 
@@ -824,46 +864,59 @@ public Map<SBase, SBase> getMatching() {
   private void getVariables(ListOf<LocalParameter> param, ASTNode node,
     List<SBase> variables, int level) {
 
-    if (node  == null)
-    {
+    if (node == null) {
       return;
     }
 
-    // found node with species
-    if ((node.getChildCount() == 0) && (node.isString()) &&
-        (node.getType() != Type.NAME_TIME) &&
-        (node.getType() != Type.NAME_AVOGADRO)) { // TODO - deal with csymbol rateOf as well ?
+    // found node with species / compartment / parameter / reaction id
+    if ((node.getChildCount() == 0) && node.isString()
+        && (node.getType() != Type.NAME_TIME)
+        && (node.getType() != Type.NAME_AVOGADRO)) {
+
       if (!node.isConstant()) {
-        if (param == null) {
-          SBase variable=node.getVariable();
-          if (level==1) {
-            int insertingPosition = 0;
-            for (SBase element:variables) {
-              if (!(element instanceof Parameter) || (!((Parameter)element).isSetValue())) {
-                insertingPosition++;
-              }
-            }
-            variables.add(insertingPosition, variable);
-          }
-          else {
-            variables.add(variable);
+
+        // Try to get the referenced SBase. For identifiers used in
+        // csymbol rateOf (L3V2), getVariable() may not yet be set,
+        // so we fall back to resolving the id in the model.
+        SBase variable = node.getVariable();
+        if (variable == null) {
+          String id = node.getName();
+          if (id != null) {
+            variable = resolveSBaseFromId(id);
           }
-        } else {
-          if (!param.contains(node.getVariable())) {
-            SBase variable=node.getVariable();
-            if (level==1) {
-              int insertingPosition=0;
-              for (SBase element:variables) {
-                if (!(element instanceof Parameter) ||
-                    (!((Parameter) element).isSetValue())) {
+        }
+
+        if (variable != null) {
+          if (param == null) {
+            // Global identifier
+            if (level == 1) {
+              int insertingPosition = 0;
+              for (SBase element : variables) {
+                if (!(element instanceof Parameter)
+                    || (!((Parameter) element).isSetValue())) {
                   insertingPosition++;
                 }
               }
               variables.add(insertingPosition, variable);
-            }
-            else {
+            } else {
               variables.add(variable);
             }
+          } else {
+            // Exclude local parameters
+            if (!param.contains(variable)) {
+              if (level == 1) {
+                int insertingPosition = 0;
+                for (SBase element : variables) {
+                  if (!(element instanceof Parameter)
+                      || (!((Parameter) element).isSetValue())) {
+                    insertingPosition++;
+                  }
+                }
+                variables.add(insertingPosition, variable);
+              } else {
+                variables.add(variable);
+              }
+            }
           }
         }
       }
@@ -933,4 +986,4 @@ private void updateMatching(List<Node<SBase>> path) {
     }
   }
 
-}
+}

core/test/org/sbml/jsbml/validator/OverdeterminationValidatorTest.java

@@ -0,0 +1,62 @@
+package org.sbml.jsbml.validator;
+
+import static org.junit.Assert.assertTrue;
+
+import java.lang.reflect.Method;
+import java.util.ArrayList;
+import java.util.List;
+
+import org.junit.Test;
+import org.sbml.jsbml.ASTNode;
+import org.sbml.jsbml.Model;
+import org.sbml.jsbml.Reaction;
+import org.sbml.jsbml.SBase;
+
+/**
+ * Tests for {@link OverdeterminationValidator}.
+ */
+public class OverdeterminationValidatorTest {
+
+  /**
+   * In SBML Level 3 Version 2, the csymbol {@code rateOf} can target
+   * a reaction identifier. The {@link OverdeterminationValidator}
+   * must treat that identifier as a referenced variable in the MathML
+   * expression.
+   *
+   * This test checks that when an {@link ASTNode} refers to a reaction
+   * by its id, and {@code getVariable()} is not set, the validator still
+   * resolves the identifier via the model and collects the reaction as
+   * a variable.
+   */
+  @Test
+  public void testGetVariablesResolvesReactionIdWhenVariableNotSet() throws Exception {
+    // Create minimal L3V2 model with one reaction
+    Model model = new Model(3, 2);
+    model.setId("m");
+    Reaction r1 = model.createReaction();
+    r1.setId("R1");
+
+    // Create an AST node that refers to the reaction by its id.
+    // In some L3V2 rateOf constructs, getVariable() may not be set
+    // on such nodes; the validator must then resolve the id via the model.
+    ASTNode nameNode = new ASTNode("R1");
+
+    OverdeterminationValidator validator = new OverdeterminationValidator(model);
+
+    // Prepare list to receive variables
+    List<SBase> vars = new ArrayList<SBase>();
+
+    // Call the private getVariables(..) method via reflection
+    Method m = OverdeterminationValidator.class.getDeclaredMethod(
+        "getVariables",
+        org.sbml.jsbml.ListOf.class,
+        ASTNode.class,
+        List.class,
+        int.class);
+    m.setAccessible(true);
+    m.invoke(validator, null, nameNode, vars, model.getLevel());
+
+    assertTrue("The reaction referenced by id must be collected as a variable",
+               vars.contains(r1));
+  }
+}

extensions/fbc/src/org/sbml/jsbml/ext/fbc/GeneProduct.java

@@ -112,12 +112,22 @@ public GeneProduct() {
   }
 
   /**
-   * Creates a new {@link GeneProduct} instance.
+   * Creates a new {@link GeneProduct} instance by copying all fields.
    * 
    * @param nsb the instance to clone
    */
   public GeneProduct(GeneProduct nsb) {
     super(nsb);
+
+    // copy GeneProduct-specific fields
+    if (nsb.isSetLabel()) {
+      // use the setter to keep property-change semantics consistent
+      setLabel(nsb.getLabel());
+    }
+
+    if (nsb.isSetAssociatedSpecies()) {
+      setAssociatedSpecies(nsb.getAssociatedSpecies());
+    }
   }
 
   /**