Merge pull request #248 from cdk/patch/safetychecks

Patch/safetychecks
cdk · Nov 9, 2016 · 1ec0d36 · 1ec0d36
2 parents 7897397 + 46ba16c
commit 1ec0d36
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 4 deletions.
diff --git a/base/data/src/main/java/org/openscience/cdk/ChemObject.java b/base/data/src/main/java/org/openscience/cdk/ChemObject.java
@@ -359,11 +359,17 @@ public void setID(String identifier) {
         notifyChanged();
     }
 
+    private boolean isPowerOfTwo(int num) {
+        return (num == 1) || (num & (num-1)) == 0;
+    }
+
     /**
-     * @inheritDoc
+     * {@inheritDoc}
      */
     @Override
     public void setFlag(int mask, boolean value) {
+        if (mask > Short.MAX_VALUE || !isPowerOfTwo(mask))
+            throw new IllegalArgumentException("setFlag() must be provided a valid CDKConstant and not used for custom properties");
         // set/unset a bit in the flags value
         if (value)
             flags |= mask;

diff --git a/base/interfaces/src/main/java/org/openscience/cdk/interfaces/IChemObject.java b/base/interfaces/src/main/java/org/openscience/cdk/interfaces/IChemObject.java
@@ -205,7 +205,9 @@ public interface IChemObject extends ICDKObject {
     /**
      * Sets the value of some flag. The flag is a mask from a given
      * CDKConstant (e.g. {@link org.openscience.cdk.CDKConstants#ISAROMATIC}
-     * or {@link org.openscience.cdk.CDKConstants#VISITED}).
+     * or {@link org.openscience.cdk.CDKConstants#VISITED}). The flags are
+     * intrinsic internal properties and should not be used to store custom
+     * values, please use {@link #setProperty(Object, Object)}.
      *
      * <pre>{@code
      * // set this chem object to be aromatic

diff --git a/base/silent/src/main/java/org/openscience/cdk/silent/ChemObject.java b/base/silent/src/main/java/org/openscience/cdk/silent/ChemObject.java
@@ -302,11 +302,17 @@ public void setID(String identifier) {
         this.identifier = identifier;
     }
 
+    private boolean isPowerOfTwo(int num) {
+        return (num == 1) || (num & (num-1)) == 0;
+    }
+
     /**
      * @inheritDoc
      */
     @Override
     public void setFlag(int mask, boolean value) {
+        if (mask > Short.MAX_VALUE || !isPowerOfTwo(mask))
+            throw new IllegalArgumentException("setFlag() must be provided a valid CDKConstant and not used for custom properties");
         // set/unset a bit in the flags value
         if (value)
             flags |= mask;

diff --git a/.../test-interfaces/src/test/java/org/openscience/cdk/interfaces/AbstractChemObjectTest.java b/.../test-interfaces/src/test/java/org/openscience/cdk/interfaces/AbstractChemObjectTest.java
@@ -129,6 +129,18 @@ public void testGetID() {
         Assert.assertNull(chemObject.getID());
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void setFlagThatIsTooBig() {
+        IChemObject chemObject = newChemObject();
+        chemObject.setFlag(1 << 17, true);
+    }
+
+    @Test(expected = IllegalArgumentException.class)
+    public void setFlagThatIsInvalid() {
+        IChemObject chemObject = newChemObject();
+        chemObject.setFlag(999, true);
+    }
+
     @Test
     public void testSetFlags_arrayboolean() {
         IChemObject chemObject = newChemObject();

diff --git a/storage/smiles/src/main/java/org/openscience/cdk/smiles/CDKToBeam.java b/storage/smiles/src/main/java/org/openscience/cdk/smiles/CDKToBeam.java
@@ -248,7 +248,11 @@ static Edge toBeamEdge(IBond b, int flavour, Map<IAtom, Integer> indices) throws
      */
     private static Bond toBeamEdgeLabel(IBond b, int flavour) throws CDKException {
 
-        if (SmiFlavor.isSet(flavour, SmiFlavor.UseAromaticSymbols) && b.getFlag(CDKConstants.ISAROMATIC)) return Bond.AROMATIC;
+        if (SmiFlavor.isSet(flavour, SmiFlavor.UseAromaticSymbols) && b.isAromatic()) {
+            if (!b.getAtom(0).isAromatic() || !b.getAtom(1).isAromatic())
+                throw new IllegalStateException("Aromatic bond connects non-aromatic atomic atoms");
+            return Bond.AROMATIC;
+        }
 
         if (b.getOrder() == null) throw new CDKException("A bond had undefined order, possible query bond?");
 
@@ -264,7 +268,7 @@ private static Bond toBeamEdgeLabel(IBond b, int flavour) throws CDKException {
             case QUADRUPLE:
                 return Bond.QUADRUPLE;
             default:
-                if (!SmiFlavor.isSet(flavour, SmiFlavor.UseAromaticSymbols) && b.getFlag(CDKConstants.ISAROMATIC))
+                if (!SmiFlavor.isSet(flavour, SmiFlavor.UseAromaticSymbols) && b.isAromatic())
                     throw new CDKException("Cannot write Kekulé SMILES output due to aromatic bond with unset bond order - molecule should be Kekulized");
                 throw new CDKException("Unsupported bond order: " + order);
         }

diff --git a/storage/smiles/src/test/java/org/openscience/cdk/smiles/SmilesGeneratorTest.java b/storage/smiles/src/test/java/org/openscience/cdk/smiles/SmilesGeneratorTest.java
@@ -1228,6 +1228,16 @@ public void assignDbStereo() throws Exception {
         assertThat(smigen.create(r2), is(smigen.create(r3)));
     }
 
+    @Test(expected = IllegalStateException.class)
+    public void inconsistentAromaticState() throws Exception {
+        SmilesParser smipar = new SmilesParser(SilentChemObjectBuilder.getInstance());
+        IAtomContainer mol = smipar.parseSmiles("c1ccccc1");
+        for (IAtom atom : mol.atoms())
+            atom.setIsAromatic(false);
+        SmilesGenerator smigen = new SmilesGenerator(SmiFlavor.UseAromaticSymbols);
+        smigen.create(mol);
+    }
+
     static ITetrahedralChirality anticlockwise(IAtomContainer container, int central, int a1, int a2, int a3, int a4) {
         return new TetrahedralChirality(container.getAtom(central), new IAtom[]{container.getAtom(a1),
                                                                                 container.getAtom(a2), container.getAtom(a3), container.getAtom(a4)},