#1256 AtomContainer.clone() should say or check that AC has atoms

cdk-1.4.x
closed
5
2013-05-25
2012-09-28
No

Either AtomContainer.clone documentation misses the precondition that atomless containers (possibly containing bonds) should not be cloned, or it should check the AC for this, before cloning. Reason: the code throws an ArrayIndexOutOfBoundsException: -1 when given such an AC.

java.lang.ArrayIndexOutOfBoundsException: -1
at org.openscience.cdk.AtomContainer.getAtom(AtomContainer.java:289)
at org.openscience.cdk.AtomContainer.clone(AtomContainer.java:1596)
at org.openscience.jchempaint.action.CopyPasteAction.addToClipboard(CopyPasteAction.java:119)

Discussion

  • Ralf Stephan

    Ralf Stephan - 2012-09-28

    Also, I'm quite sure that the same exception is thrown when only one of a bond's atom is present in the AC.

     
  • Egon Willighagen

    Mmmm... I have not been able to reproduce it yet...:

    /**
     * @cdk.bug 1256
     */
    @Test public void testClone_SingleAtomIBond() throws Exception {
        IAtomContainer molecule = (IAtomContainer)newChemObject();
        IAtom atom = molecule.getBuilder().newInstance(IAtom.class, "C");
        molecule.addAtom(atom);
        IBond bond = molecule.getBuilder().newInstance(IBond.class, atom, null, IBond.Order.SINGLE);
        molecule.addBond(bond);
        Assert.assertEquals(bond, molecule.getBond(0));
        IAtomContainer clone = (IAtomContainer)molecule.clone();
        Assert.assertEquals(1, clone.getBondCount());
    }
    
    /**
     * @cdk.bug 1256
     */
    @Test public void testClone_EmptyContainer() throws Exception {
        IAtomContainer molecule = (IAtomContainer)newChemObject();
        IAtomContainer clone = (IAtomContainer)molecule.clone();
        Assert.assertNotNull(clone);
    }
    

    These both work fine.

    It must have to do with a bond referring to an atom that does not exist or so...

     
  • John May

    John May - 2012-11-03

    Interesting bug... it happens on this line:

    newLp.setAtom(clone.getAtom(getAtomNumber(lp.getAtom())));
    

    and the -1 is from :

        public int getAtomNumber(IAtom atom)
        {
            for (int f = 0; f < atomCount; f++)
            {
                if (atoms[f] == atom) return f;
            }
            return -1;
        }
    

    What might be happening is a lone pair has been added with an atom which isn't in the container. The addLonePair code doesn't enforce that the given atom is actually in the
    container.

        public void addLonePair(ILonePair lonePair)
        {
            if (lonePairCount >= lonePairs.length) growLonePairArray();
            lonePairs[lonePairCount] = lonePair;
            ++lonePairCount;
            notifyChanged();
        }
    

    When the container is cloned it attempts to find the index for the atom - which isn't in the container and so returns -1. This then cause the index out of bounds exception. I'm not sure whether the lone pair should check the atom is in the container as that's a pretty costly operation.

    Anyways, this won't happen with the new cloning as the map.get() returns null if a value is not found for the key. Not sure if that's desired behaviour but it's better then an index out of bounds.

    J

     
    Last edit: John May 2012-11-03
  • Ralf Stephan

    Ralf Stephan - 2012-11-07

    Sorry for giving incomplete information (I thought at the time it sufficed).
    The original JCP issue leading to this report is #139; it was caused by unchecked
    calls to clone in CopyPasteAction.java and a different selection logic that allowed
    for selection of half bonds (bond+1atom). It should have been possible to recreate the original offending container for this bug but I was unsuccessful in this.

    Proposed to be invalid/wontfix.

     
  • John May

    John May - 2013-03-26
     
  • John May

    John May - 2013-03-26

    I second wont-fix

     
  • John May

    John May - 2013-05-25
    • status: open --> closed