#1267 IllegalAccessError on QueryChemObject

master
closed
John May
master (3)
9
2013-05-25
2012-10-22
John May
No

Looks like this bug has been around for a while - spotted by Asad as he's currently using master.

Currently there are lots of failing tests with IllegalAccessError. This is because of the new QueryChemObject in master which is used as a sub-class of QueryAtom/QueryAtomContainer etc.

The code in 1.4.x inherits from the relative cdk-data superclass (e.g. QueryAtomContainer extends AtomContainer). This allows correct access of the builder used.

The isomorphism module was (reworked)[https://github.com/egonw/cdk/commit/a5439dc69afd506c6fdba5e8d51552f3f18bc300#src/main/org/openscience/cdk/isomorphism/matchers/QueryAtomContainer.java] so it didn't depend on data - (e.g. could not extend AtomContainer) and thus the QueryChemObject was created. However because the QueryChemObject doesn't depend on cdk-data it can't provide a builder.

public IChemObjectBuilder getBuilder() {
    throw new IllegalAccessError(); // no message given
}

As some readers can create a QueryAtomContainer when reading the returned object will have a 'getBuilder()' method which won't work and throw this non-specific error. This method is used everywhere (even when typing the atoms) and there are numerous errors.

We can't return the default builder in this module was explicitly decoupled. I think I have a fix but will need to test first.

Discussion

  • John May

    John May - 2012-10-22

    Okay - have fixed. Required adding a lot of parameters but it fixes ~150 test fails in master.... which is pretty good.

     
  • Anonymous - 2012-10-24

    I have run into the same problem when trying to parse molecules with a query bond type (value=8), e.g., any complexed magnesium-hydroxyprotoporphyrins.

    Would be good to see that fixed in master.

     
  • John May

    John May - 2012-10-26

    Here's a solution: branch/commit

    It involves requiring that you pass the builder to the QueryChemObject during construction. This meant lots had to be delegated (e.g. all the SMARTS atoms). The actual impact on the front end is rather minimal - you need to now pass a builder to the SMARTSQueryTool and also the PubChemFingerprinter (which uses SMARTSQueryTool). Other uses of SMARTSQueryTool could utilise the builder of another ChemObject. Making it a required constructor argument completes the contract of IChemObject to always provide a builder. Another option would be to return a 'null' builder (e.g. one which couldn't build anything) but I think the test results clearly show that these builders do get used and are required.

    I haven't added documentation as I didn't want to waste my time if there is a working alternative.

    Test Results

    pre-patch

    total: run=18362 failed=94 error=223 time=670.116 secs
    

    post-patch

    total: run=18362 failed=100 error=75 time=558.2339999999998 secs
    

    J

    Edit: those 223 was because i forgot to jarTestdata... it's actually a lot less

    I should add that 54/75 errors are in qsar-molecule which I'll look at next

     
    Last edit: John May 2012-10-28
  • John May

    John May - 2012-10-28

    Fixed the errors on qsarmolecular now... these were because they used reflection to instantiate the objects (difficult to debug). Thus when I added the required IChemObjectBuilder there was an instantiation error as it couldn't find a default construtor.

    Need to neaten up will be push the changes to my branch tomorrow.

    J

     
  • John May

    John May - 2013-03-18

    I've merged this in to my master+ now

     
  • John May

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

    John May - 2013-05-25

    Fixed

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:





No, thanks