diff --git a/core/src/org/sbml/jsbml/AbstractNamedSBaseWithUnit.java b/core/src/org/sbml/jsbml/AbstractNamedSBaseWithUnit.java index 7ce63fabb..caedd4034 100644 --- a/core/src/org/sbml/jsbml/AbstractNamedSBaseWithUnit.java +++ b/core/src/org/sbml/jsbml/AbstractNamedSBaseWithUnit.java @@ -24,6 +24,9 @@ import org.apache.log4j.Logger; import org.sbml.jsbml.Unit.Kind; import org.sbml.jsbml.util.TreeNodeChangeEvent; +import org.sbml.jsbml.validator.SyntaxChecker; +import org.sbml.jsbml.validator.offline.factory.SBMLErrorCodes; +import org.sbml.jsbml.validator.offline.factory.SBMLErrorFactory; /** * This simple implementation of the interfaces @@ -283,7 +286,7 @@ public void setUnits(Kind unitKind) { @Override public void setUnits(String units) { if ((units != null) && (units.trim().length() == 0)) { - units = null; // If we pass the empty String or null, the value is reset. + units = null; } String oldUnits = unitsID; @@ -292,24 +295,40 @@ public void setUnits(String units) { unitsID = null; } else { units = units.trim(); + unitsID = units; - boolean illegalArgument = false; + boolean isSyntaxValid = SyntaxChecker.isValidId(units, getLevel(), getVersion()); + boolean isReferenceValid = isSyntaxValid && Unit.isValidUnit(getModel(), units); - if (!Unit.isValidUnit(getModel(), units)) { - illegalArgument = true; // TODO - make use of the offline validation once attributes validation is in place. - } - if (illegalArgument) { + if (!isSyntaxValid || !isReferenceValid) { if (!isReadingInProgress()) { - throw new IllegalArgumentException(MessageFormat.format( - JSBML.ILLEGAL_UNIT_EXCEPTION_MSG, units)); + SBMLDocument doc = getSBMLDocument(); + if (doc != null && doc.getErrorLog() != null) { + int errorCode = !isSyntaxValid ? SBMLErrorCodes.CORE_10311 : SBMLErrorCodes.CORE_10313; + + org.sbml.jsbml.SBMLError error = SBMLErrorFactory.createError( + errorCode, + getLevel(), + getVersion(), + false, // Let the factory fetch the official SBML specification message! + this + ); + + if (error != null) { + doc.getErrorLog().add(error); + } + } else { + unitsID = oldUnits; + throw new IllegalArgumentException(MessageFormat.format( + JSBML.ILLEGAL_UNIT_EXCEPTION_MSG, units)); + } } else { logger.info(MessageFormat.format(JSBML.ILLEGAL_UNIT_EXCEPTION_MSG, units)); } } - unitsID = units; } - if (oldUnits != unitsID) { + if (oldUnits != null ? !oldUnits.equals(unitsID) : unitsID != null) { firePropertyChange(TreeNodeChangeEvent.units, oldUnits, unitsID); } } diff --git a/core/test/org/sbml/jsbml/xml/test/TestAbstractNamedSBaseWithUnits.java b/core/test/org/sbml/jsbml/xml/test/TestAbstractNamedSBaseWithUnits.java index 58251ce59..605d29885 100644 --- a/core/test/org/sbml/jsbml/xml/test/TestAbstractNamedSBaseWithUnits.java +++ b/core/test/org/sbml/jsbml/xml/test/TestAbstractNamedSBaseWithUnits.java @@ -26,9 +26,11 @@ import org.junit.Test; import org.sbml.jsbml.AbstractNamedSBaseWithUnit; import org.sbml.jsbml.Model; +import org.sbml.jsbml.Parameter; import org.sbml.jsbml.SBMLDocument; import org.sbml.jsbml.Unit; import org.sbml.jsbml.UnitDefinition; +import org.sbml.jsbml.validator.offline.factory.SBMLErrorCodes; /** @@ -127,4 +129,42 @@ public void testIsPredefinedUnitsID() { assertTrue(!sbase.isPredefinedUnitsID(kind.toString().toLowerCase())); } + /** + * Test method for {@link org.sbml.jsbml.AbstractNamedSBaseWithUnit#setUnits(java.lang.String)} + * when an invalid unit syntax is set on an attached node. + */ + @Test + public void testSetInvalidUnitSyntaxWithDocumentLogsError() { + SBMLDocument doc = sbase.getSBMLDocument(); + int initialErrorCount = doc.getErrorLog().getErrorCount(); + sbase.setUnits("123 invalid syntax!"); // Malformed SId + org.junit.Assert.assertEquals("Error count should increment", initialErrorCount + 1, doc.getErrorLog().getErrorCount()); + org.junit.Assert.assertEquals("Should log CORE_10311 for invalid UnitSId syntax", + (long) SBMLErrorCodes.CORE_10311, (long) doc.getErrorLog().getError(initialErrorCount).getCode()); + } + + /** + * Test method for {@link org.sbml.jsbml.AbstractNamedSBaseWithUnit#setUnits(java.lang.String)} + * when an undefined unit is set on an attached node. + */ + @Test + public void testSetMissingUnitReferenceWithDocumentLogsError() { + SBMLDocument doc = sbase.getSBMLDocument(); + int initialErrorCount = doc.getErrorLog().getErrorCount(); + sbase.setUnits("valid_syntax_but_missing"); // Valid SId, but not defined in model or built-ins + org.junit.Assert.assertEquals("Error count should increment", initialErrorCount + 1, doc.getErrorLog().getErrorCount()); + org.junit.Assert.assertEquals("Should log CORE_10313 for missing unit reference", + (long) SBMLErrorCodes.CORE_10313, (long) doc.getErrorLog().getError(initialErrorCount).getCode()); + } + + /** + * Test method for {@link org.sbml.jsbml.AbstractNamedSBaseWithUnit#setUnits(java.lang.String)} + * when an invalid unit is set on an isolated node. + */ + @Test(expected = IllegalArgumentException.class) + public void testSetInvalidUnitWithoutDocumentThrowsException() { + Parameter isolatedParam = new Parameter(3, 1); + isolatedParam.setUnits("invalid syntax!"); + } + }