sbgn->sbml doesn't crash when identifiers are problematic for sbml

CHANGELOG

+minerva (14.0.3) stable; urgency=medium
+  * Bug fix: some SBGN files uploaded to minerva could not be exported to SBML
+    due to problems with identifiers used by SBGN (#1006)
+
+ -- Piotr Gawron <piotr.gawron@uni.lu>  Wed, 06 Nov 2019 12:00:00 +0200
+
 minerva (14.0.3) stable; urgency=medium
   * Bug fix: default zoom level on main map works even when x or y are
     undefined (#993)

converter-sbml/src/main/java/lcsb/mapviewer/converter/model/sbml/SbmlBioEntityExporter.java

 package lcsb.mapviewer.converter.model.sbml;
 
-import java.awt.*;
+import java.awt.Color;
 import java.util.*;
+import java.util.regex.Pattern;
 
 import javax.xml.stream.XMLStreamException;
 
@@ -10,10 +11,8 @@ import org.apache.logging.log4j.Logger;
 import org.sbml.jsbml.Model;
 import org.sbml.jsbml.ext.SBasePlugin;
 import org.sbml.jsbml.ext.layout.*;
-import org.sbml.jsbml.ext.layout.Point;
 import org.sbml.jsbml.ext.multi.MultiModelPlugin;
 import org.sbml.jsbml.ext.render.*;
-import org.sbml.jsbml.ext.render.Polygon;
 
 import lcsb.mapviewer.common.XmlParser;
 import lcsb.mapviewer.common.exception.InvalidStateException;
@@ -49,12 +48,26 @@ public abstract class SbmlBioEntityExporter<T extends BioEntity, S extends org.s
 
   private boolean provideDefaults;
 
+  private Pattern sbmlValidIdMatcher;
+
+  /**
+   * Map with elementId mapping for identifiers that cannot be used in sbml.
+   */
+  private Map<String, String> correctedElementId = new HashMap<>();
+
   public SbmlBioEntityExporter(Model sbmlModel, lcsb.mapviewer.model.map.model.Model minervaModel,
       Collection<SbmlExtension> sbmlExtensions) {
     this.sbmlModel = sbmlModel;
     this.layout = getLayout(sbmlModel);
     this.minervaModel = minervaModel;
     this.sbmlExtensions.addAll(sbmlExtensions);
+
+    String underscore = "_";
+    String letter = "a-zA-Z";
+    String digit = "0-9";
+    String idChar = '[' + letter + digit + underscore + ']';
+    sbmlValidIdMatcher = Pattern.compile("^[" + letter + underscore + "]" + idChar + '*');
+
   }
 
   public void exportElements() throws InconsistentModelException {
@@ -63,10 +76,10 @@ public abstract class SbmlBioEntityExporter<T extends BioEntity, S extends org.s
       try {
         S sbmlElement = getSbmlElement(bioEntity);
 
-        if (sbmlElementByElementId.get(bioEntity.getElementId()) != null) {
-          throw new InconsistentModelException("More than one species with id: " + bioEntity.getElementId());
+        if (sbmlElementByElementId.get(getElementId(bioEntity)) != null) {
+          throw new InconsistentModelException("More than one species with id: " + getElementId(bioEntity));
         }
-        sbmlElementByElementId.put(bioEntity.getElementId(), sbmlElement);
+        sbmlElementByElementId.put(getElementId(bioEntity), sbmlElement);
       } catch (Exception e) {
         throw new InconsistentModelException(new ElementUtils().getElementTag(bioEntity) +
             "Problem with exporting bioEntity", e);
@@ -76,7 +89,7 @@ public abstract class SbmlBioEntityExporter<T extends BioEntity, S extends org.s
       for (T bioEntity : speciesList) {
         try {
           AbstractReferenceGlyph elementGlyph = createGlyph(bioEntity);
-          sbmlGlyphByElementId.put(bioEntity.getElementId(), elementGlyph);
+          sbmlGlyphByElementId.put(getElementId(bioEntity), elementGlyph);
         } catch (Exception e) {
           throw new InconsistentModelException(new ElementUtils().getElementTag(bioEntity) +
               "Problem with exporting bioEntity", e);
@@ -127,8 +140,8 @@ public abstract class SbmlBioEntityExporter<T extends BioEntity, S extends org.s
   protected abstract void assignLayoutToGlyph(T element, AbstractReferenceGlyph compartmentGlyph);
 
   protected AbstractReferenceGlyph createGlyph(T element) {
-    String sbmlElementId = sbmlElementByElementId.get(element.getElementId()).getId();
-    String glyphId = element.getElementId();
+    String sbmlElementId = sbmlElementByElementId.get(getElementId(element)).getId();
+    String glyphId = getElementId(element);
     AbstractReferenceGlyph elementGlyph = createElementGlyph(sbmlElementId, glyphId);
     assignLayoutToGlyph(element, elementGlyph);
     return elementGlyph;
@@ -403,4 +416,19 @@ public abstract class SbmlBioEntityExporter<T extends BioEntity, S extends org.s
     this.provideDefaults = provideDefaults;
   }
 
+  public String getElementId(BioEntity bioEntity) {
+    String result = bioEntity.getElementId();
+    if (!sbmlValidIdMatcher.matcher(result).matches()) {
+      if (correctedElementId.get(result) == null) {
+        String newElementId = this.getClass().getSimpleName() + "_" + getNextId();
+        logger.warn(new ElementUtils().getElementTag(bioEntity) + " Element id cannot be used in sbml. Changing to: "
+            + newElementId);
+        correctedElementId.put(result, newElementId);
+      }
+      result = correctedElementId.get(result);
+    }
+
+    return result;
+  }
+
 }

converter-sbml/src/main/java/lcsb/mapviewer/converter/model/sbml/SbmlExporter.java

@@ -73,7 +73,12 @@ public class SbmlExporter {
    */
   protected SBMLDocument toSbmlDocument(lcsb.mapviewer.model.map.model.Model model) throws InconsistentModelException {
     SBMLDocument doc = new SBMLDocument(3, 2);
-    Model result = doc.createModel(model.getIdModel());
+    Model result = doc.createModel();
+    try {
+      result.setId(model.getIdModel());
+    } catch (IllegalArgumentException e) {
+      logger.warn("Invalid model identifier: \"" + model.getIdModel() + "\". Ignoring.");
+    }
     result.setName(model.getName());
     try {
       result.setNotes(NotesUtility.prepareEscapedXmlNotes(model.getNotes()));

converter-sbml/src/main/java/lcsb/mapviewer/converter/model/sbml/compartment/SbmlCompartmentExporter.java

@@ -34,7 +34,7 @@ public class SbmlCompartmentExporter extends SbmlElementExporter<Compartment, or
     result.addAll(getMinervaModel().getCompartments());
     boolean defaultFound = false;
     for (Compartment compartment : result) {
-      if (compartment.getElementId().equals("default")) {
+      if (getElementId(compartment).equals("default")) {
         defaultFound = true;
       }
     }
@@ -48,7 +48,7 @@ public class SbmlCompartmentExporter extends SbmlElementExporter<Compartment, or
   @Override
   public org.sbml.jsbml.Compartment createSbmlElement(Compartment element) throws InconsistentModelException {
     org.sbml.jsbml.Compartment result;
-    if (element == null || element.getElementId().equals("default")) {
+    if (element == null || getElementId(element).equals("default")) {
       result = getSbmlModel().createCompartment("default");
     } else {
       result = getSbmlModel().createCompartment("comp_" + (getNextId()));
@@ -77,7 +77,7 @@ public class SbmlCompartmentExporter extends SbmlElementExporter<Compartment, or
 
   @Override
   protected String getSbmlIdKey(Compartment compartment) {
-    if (compartment == null || compartment.getElementId().equals("default")) {
+    if (compartment == null || getElementId(compartment).equals("default")) {
       return "default";
     }
     return compartment.getName();
@@ -131,7 +131,7 @@ public class SbmlCompartmentExporter extends SbmlElementExporter<Compartment, or
 
   @Override
   protected void assignLayoutToGlyph(Compartment element, AbstractReferenceGlyph speciesGlyph) {
-    if (element.getElementId().equals("default")) {
+    if (getElementId(element).equals("default")) {
       BoundingBox boundingBox = new BoundingBox();
 
       boundingBox.setPosition(new Point(element.getX(), element.getY(), 0));
@@ -145,7 +145,7 @@ public class SbmlCompartmentExporter extends SbmlElementExporter<Compartment, or
       speciesGlyph.setBoundingBox(boundingBox);
     } else {
       super.assignLayoutToGlyph(element, speciesGlyph);
-      TextGlyph textGlyph = getLayout().createTextGlyph("text_" + element.getElementId(), element.getName());
+      TextGlyph textGlyph = getLayout().createTextGlyph("text_" + getElementId(element), element.getName());
       Point2D point = element.getNamePoint();
       double width = element.getWidth() - (point.getX() - element.getX());
       double height = element.getHeight() - (point.getY() - element.getY());

converter-sbml/src/main/java/lcsb/mapviewer/converter/model/sbml/reaction/SbmlReactionExporter.java

@@ -98,11 +98,12 @@ public class SbmlReactionExporter extends SbmlBioEntityExporter<Reaction, org.sb
   }
 
   private String getReactionId(Reaction reaction) {
-    int separatorIndex = reaction.getElementId().indexOf("__");
+    String elementId = getElementId(reaction);
+    int separatorIndex = elementId.indexOf("__");
     if (separatorIndex > 0) {
-      return reaction.getElementId().substring(0, separatorIndex);
+      return elementId.substring(0, separatorIndex);
     }
-    return reaction.getElementId();
+    return elementId;
   }
 
   @Override
@@ -124,19 +125,22 @@ public class SbmlReactionExporter extends SbmlBioEntityExporter<Reaction, org.sb
       result.setSBOTerm(sboTerm);
     }
     for (Product product : reaction.getProducts()) {
-      Species sbmlSymbol = speciesExporter.getSbmlElementByElementId(product.getElement().getElementId());
+      Species sbmlSymbol = speciesExporter
+          .getSbmlElementByElementId(speciesExporter.getElementId(product.getElement()));
       SpeciesReference speciesReference = result.createProduct(sbmlSymbol);
       speciesReference.setConstant(false);
       speciesReferenceByReactionNode.put(product, speciesReference);
     }
     for (Reactant reactant : reaction.getReactants()) {
-      Species sbmlSymbol = speciesExporter.getSbmlElementByElementId(reactant.getElement().getElementId());
+      Species sbmlSymbol = speciesExporter
+          .getSbmlElementByElementId(speciesExporter.getElementId(reactant.getElement()));
       SpeciesReference speciesReference = result.createReactant(sbmlSymbol);
       speciesReference.setConstant(false);
       speciesReferenceByReactionNode.put(reactant, speciesReference);
     }
     for (Modifier modifier : reaction.getModifiers()) {
-      Species sbmlSymbol = speciesExporter.getSbmlElementByElementId(modifier.getElement().getElementId());
+      Species sbmlSymbol = speciesExporter
+          .getSbmlElementByElementId(speciesExporter.getElementId(modifier.getElement()));
       SimpleSpeciesReference speciesReference = result.createModifier(sbmlSymbol);
       speciesReferenceByReactionNode.put(modifier, speciesReference);
       String term = SBOTermModifierType.getTermByType(modifier.getClass());
@@ -151,8 +155,8 @@ public class SbmlReactionExporter extends SbmlBioEntityExporter<Reaction, org.sb
   }
 
   @Override
-  protected String getSbmlIdKey(Reaction element) {
-    return element.getClass().getSimpleName() + "\n" + element.getElementId();
+  protected String getSbmlIdKey(Reaction reaction) {
+    return reaction.getClass().getSimpleName() + "\n" + getElementId(reaction);
   }
 
   @Override
@@ -367,7 +371,8 @@ public class SbmlReactionExporter extends SbmlBioEntityExporter<Reaction, org.sb
 
   private SpeciesReferenceGlyph createNodeGlyph(ReactionGlyph reactionGlyph, ReactionNode node) {
     SpeciesReferenceGlyph reactantGlyph = reactionGlyph.createSpeciesReferenceGlyph("node_" + getNextId());
-    reactantGlyph.setSpeciesGlyph(speciesExporter.getSbmlGlyphByElementId(node.getElement().getElementId()).getId());
+    reactantGlyph.setSpeciesGlyph(
+        speciesExporter.getSbmlGlyphByElementId(speciesExporter.getElementId(node.getElement())).getId());
     Curve curve = createCurve(node.getLine(), node instanceof Reactant);
     if (curve.getCurveSegmentCount() == 0) {
       logger.warn(new ElementUtils().getElementTag(node) + " Problematic line");

converter-sbml/src/main/java/lcsb/mapviewer/converter/model/sbml/species/SbmlSpeciesExporter.java

@@ -466,7 +466,7 @@ public class SbmlSpeciesExporter extends SbmlElementExporter<Species, org.sbml.j
         }
       }
     }
-    TextGlyph textGlyph = getLayout().createTextGlyph("text_" + element.getElementId(), element.getName());
+    TextGlyph textGlyph = getLayout().createTextGlyph("text_" + getElementId(element), element.getName());
     textGlyph
         .setBoundingBox(createBoundingBox(element.getX(), element.getY(), element.getZ() + 1, element.getWidth(),
             element.getHeight()));

converter-sbml/src/test/java/lcsb/mapviewer/converter/model/sbml/SbmlExporterTest.java

@@ -3,7 +3,6 @@ package lcsb.mapviewer.converter.model.sbml;
 import static org.junit.Assert.*;
 
 import java.awt.Color;
-import java.awt.geom.Line2D;
 import java.awt.geom.Point2D;
 import java.io.File;
 import java.lang.reflect.Modifier;
@@ -17,7 +16,6 @@ import org.sbml.jsbml.SBMLDocument;
 import org.sbml.jsbml.ext.multi.*;
 
 import lcsb.mapviewer.common.Configuration;
-import lcsb.mapviewer.common.comparator.LineComparator;
 import lcsb.mapviewer.common.comparator.ListComparator;
 import lcsb.mapviewer.converter.ConverterParams;
 import lcsb.mapviewer.converter.model.celldesigner.CellDesignerXmlParser;
@@ -605,4 +603,80 @@ public class SbmlExporterTest extends SbmlTestFunctions {
     assertEquals(0, comparator.compare(model, originalModel));
   }
 
+  @Test
+  public void testInvalidSpeciesId() throws Exception {
+    Model model = createEmptyModel();
+
+    GenericProtein p = createProtein();
+    p.setElementId("a-b");
+    model.addElement(p);
+
+    String xml = exporter.toXml(model);
+    assertNotNull(xml);
+
+    assertEquals(1, getWarnings().size());
+  }
+
+  @Test
+  public void testInvalidReactionId() throws Exception {
+    Model model = createEmptyModel();
+
+    Protein p1 = createProtein();
+    Protein p2 = createProtein();
+    Reaction p = createReaction(p1, p2);
+    p.setIdReaction("a-b");
+    model.addElement(p1);
+    model.addElement(p2);
+    model.addReaction(p);
+
+    String xml = exporter.toXml(model);
+    assertNotNull(xml);
+    assertEquals(1, getWarnings().size());
+
+  }
+
+  @Test
+  public void testInvalidSpeciesIdInReaction() throws Exception {
+    Model model = createEmptyModel();
+
+    Protein p1 = createProtein();
+    Protein p2 = createProtein();
+    Reaction p = createReaction(p1, p2);
+    p1.setElementId("a-b");
+    model.addElement(p1);
+    model.addElement(p2);
+    model.addReaction(p);
+
+    String xml = exporter.toXml(model);
+    assertNotNull(xml);
+    assertEquals(1, getWarnings().size());
+
+  }
+
+  @Test
+  public void testInvalidCompartmentId() throws Exception {
+    Model model = createEmptyModel();
+
+    Compartment p = createCompartment();
+    p.setElementId("a-b");
+    model.addElement(p);
+
+    String xml = exporter.toXml(model);
+    assertNotNull(xml);
+
+    assertEquals(1, getWarnings().size());
+  }
+
+  @Test
+  public void testInvalidModelId() throws Exception {
+    Model model = createEmptyModel();
+    model.setIdModel("a-b");
+
+    String xml = exporter.toXml(model);
+    assertNotNull(xml);
+    assertEquals(1, getWarnings().size());
+
+  }
+
+
 }

converter-sbml/src/test/java/lcsb/mapviewer/converter/model/sbml/SbmlTestFunctions.java

@@ -18,7 +18,8 @@ import org.apache.http.entity.mime.MultipartEntityBuilder;
 import org.apache.http.impl.client.CloseableHttpClient;
 import org.apache.http.impl.client.HttpClients;
 import org.apache.http.util.EntityUtils;
-import org.apache.log4j.Logger;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.LogEvent;
 import org.junit.*;
 import org.w3c.dom.Document;
@@ -39,14 +40,14 @@ import lcsb.mapviewer.model.map.species.Element;
 import lcsb.mapviewer.model.map.species.GenericProtein;
 
 public class SbmlTestFunctions {
+  private Logger logger = LogManager.getLogger();
 
   private static int identifierCounter = 0;
+  
   @Rule
   public UnitTestFailedWatcher unitTestFailedWatcher = new UnitTestFailedWatcher();
   SbmlParser parser = new SbmlParser();
   SbmlExporter exporter;
-  @SuppressWarnings("unused")
-  private Logger logger = Logger.getLogger(SbmlTestFunctions.class);
   private MinervaLoggerAppender appender;
 
   public SbmlTestFunctions() {

rest-api/src/test/java/lcsb/mapviewer/api/convert/ConvertRestImplTest.java

@@ -211,4 +211,10 @@ public class ConvertRestImplTest extends RestTestFunctions {
     test2All(content, "SBGN-ML");
   }
 
+  @Test
+  public void testNewtSbgn2All() throws Exception {
+    String content = readFile("testFiles/convert/newt.sbgn");
+    test2All(content, "SBGN-ML");
+  }
+
 }

rest-api/testFiles/convert/newt.sbgn

+<?xml version='1.0' encoding='UTF-8' standalone='yes'?>
+<sbgn xmlns="http://sbgn.org/libsbgn/0.2">
+  <map language="process description">
+    <glyph id="nwtN_f034af08-f2b3-4912-aaa8-58b4f55f43b9" class="macromolecule">
+      <label text="dffgdf"/>
+      <bbox x="707" y="292" w="60" h="30"/>
+    </glyph>
+  </map>
+</sbgn>
\ No newline at end of file