QDox
  1. QDox
  2. QDOX-148

Allow to disable usage of "default" class loaders

    Details

    • Type: New Feature New Feature
    • Status: Resolved Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.2
    • Fix Version/s: 1.9.2
    • Component/s: Parser
    • Labels:
      None
    • Number of attachments :
      3

      Description

      Both available constructors of JavaDocBuilder will eventually call

      classLibrary.addDefaultLoader();
      

      As far as I see, there is currently no way to disable/remove these default loaders.

      This is problematic when the code using QDox and the code being analyzed depend on the same class file (but possibly a different version). Imagine a project with an annotated source file S that extends a class C from a third-party JAR. Further imagine a build tool that employs QDox to scan the previously mentioned source file S. When the scan walks up to the super class C of S, QDox ends up in ClassLibrary.getClass() and iterates over the class loaders to load the class C from its binary. Now imagine the build tool itself also depends on this binary class C and the build tool's class loader was used to create the JavaDocBuilder instance used for the scan. In this scenario, QDox will load C from the dependencies of the build tool instead of the dependencies of the project whose sources are actually scanned. In other words, the class path of the QDox client pollutes the analysis.

      Possible solutions could be a additional constructors for JavaDocBuilder to disable addition of the default class loaders or a new method clearClassLoaders() in ClassLibrary.

      1. qdox-148.1.patch
        6 kB
        Robert Scholte
      2. qdox-148.patch
        1 kB
        Robert Scholte
      3. qdox-148.patch
        3 kB
        Benjamin Bentmann

        Issue Links

          Activity

          Benjamin Bentmann made changes -
          Field Original Value New Value
          Link This issue relates to MNG-3686 [ MNG-3686 ]
          Hide
          Benjamin Bentmann added a comment -

          A not so beauty unit test for this.

          Show
          Benjamin Bentmann added a comment - A not so beauty unit test for this.
          Benjamin Bentmann made changes -
          Attachment qdox-148.patch [ 40180 ]
          Hide
          Robert Scholte added a comment -

          I might have found a cleaner solution then trying to unload some classloaders.
          When for some reason a binaryclass can't be resolved, an unknownClass will be created which will be added to the classcache. This code was already there, but it only checked if createBinaryClass(name) would result in null.
          The proposal it to surround it with a try/catch for ClassDefNotFoundException

          Show
          Robert Scholte added a comment - I might have found a cleaner solution then trying to unload some classloaders. When for some reason a binaryclass can't be resolved, an unknownClass will be created which will be added to the classcache. This code was already there, but it only checked if createBinaryClass(name) would result in null. The proposal it to surround it with a try/catch for ClassDefNotFoundException
          Robert Scholte made changes -
          Attachment qdox-148.patch [ 40192 ]
          Hide
          Benjamin Bentmann added a comment -

          While the try/catch is surely an improvement, I still believe the option to exclude the default class loaders is worth to consider. I mean if tool A uses QDox to scan the sources of project B, what's the rationale for QDox trying to load classes from A's class path when A and B are totally different/independent projects? A and B might have some dependencies in common but in the general case that's rather by incident than by design, isn't it? If A wants to fully traverse the class hierarchy of B, the only proper way is for A to register a custom class loader with QDox that resembles B's class path.

          Show
          Benjamin Bentmann added a comment - While the try/catch is surely an improvement, I still believe the option to exclude the default class loaders is worth to consider. I mean if tool A uses QDox to scan the sources of project B, what's the rationale for QDox trying to load classes from A's class path when A and B are totally different/independent projects? A and B might have some dependencies in common but in the general case that's rather by incident than by design, isn't it? If A wants to fully traverse the class hierarchy of B, the only proper way is for A to register a custom class loader with QDox that resembles B's class path.
          Hide
          Robert Scholte added a comment -

          I agree we need to have control over the classloaders. I'm thinking of introducing an new constructor to the JavaDocBuilder with a ClassLibrary as argument. This way you can add your own classloaders.
          And after a second look, the exception should be pulled up to the createBinaryClass. If a CDNF-Exception occurs it should return null.

          Show
          Robert Scholte added a comment - I agree we need to have control over the classloaders. I'm thinking of introducing an new constructor to the JavaDocBuilder with a ClassLibrary as argument. This way you can add your own classloaders. And after a second look, the exception should be pulled up to the createBinaryClass. If a CDNF-Exception occurs it should return null.
          Hide
          Robert Scholte added a comment -

          patch created as mentioned. JavaDocBuilder has new constructors with ClassLibrary support and there's a big try/catch inside createBinaryClass. Don't think it's usefull to catch every small step while builder a JavaClass, one mistake makes this JavaClass broken, so it's better to return null asap.

          Show
          Robert Scholte added a comment - patch created as mentioned. JavaDocBuilder has new constructors with ClassLibrary support and there's a big try/catch inside createBinaryClass. Don't think it's usefull to catch every small step while builder a JavaClass, one mistake makes this JavaClass broken, so it's better to return null asap.
          Robert Scholte made changes -
          Attachment qdox-148.1.patch [ 40254 ]
          Hide
          Benjamin Bentmann added a comment -

          Looks good to me

          Show
          Benjamin Bentmann added a comment - Looks good to me
          Hide
          Robert Scholte added a comment -

          r573 | rfscholte | 2009-03-02 19:26:08 CET

          patch applied for qdox-148
          ----------------------------------------------------------------------------

          Show
          Robert Scholte added a comment - r573 | rfscholte | 2009-03-02 19:26:08 CET patch applied for qdox-148 ----------------------------------------------------------------------------
          Robert Scholte made changes -
          Resolution Fixed [ 1 ]
          Assignee Robert Scholte [ rfscholte ]
          Status Open [ 1 ] Closed [ 6 ]
          Fix Version/s 1.10 [ 15020 ]
          Paul Hammant made changes -
          Resolution Fixed [ 1 ]
          Status Closed [ 6 ] Reopened [ 4 ]
          Paul Hammant made changes -
          Fix Version/s 1.10 [ 15020 ]
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 1.9.1 [ 15252 ]
          Resolution Fixed [ 1 ]
          Hide
          Benjamin Bentmann added a comment -

          There's a problem we missed with the new constructors: How to create the ClassLibrary to pass into the JavaDocBuilder? To create a ClassLibrary, a JavaClassCache needs to be provided for its constructor. However, the intention is to have the JavaDocBuilder as JavaClassCache so we have a cycle: It requires an existing JavaDocBuilder to create the ClassLibrary and it requires an existing ClassLibrary to create the JavaDocBuilder without default class loaders.

          Probably the easiest way out is to simply provide a constructor in JavaDocBuilder that provides a boolean parameter to skip the call to addDefaultLoader().

          FWIW, the improved try/catch logic for linkage errors already saved source-scanning Maven plugins from troubles so the remaining issue is of less concern.

          Show
          Benjamin Bentmann added a comment - There's a problem we missed with the new constructors: How to create the ClassLibrary to pass into the JavaDocBuilder ? To create a ClassLibrary , a JavaClassCache needs to be provided for its constructor. However, the intention is to have the JavaDocBuilder as JavaClassCache so we have a cycle: It requires an existing JavaDocBuilder to create the ClassLibrary and it requires an existing ClassLibrary to create the JavaDocBuilder without default class loaders. Probably the easiest way out is to simply provide a constructor in JavaDocBuilder that provides a boolean parameter to skip the call to addDefaultLoader() . FWIW, the improved try/catch logic for linkage errors already saved source-scanning Maven plugins from troubles so the remaining issue is of less concern.
          Benjamin Bentmann made changes -
          Status Resolved [ 5 ] Reopened [ 4 ]
          Resolution Fixed [ 1 ]
          Benjamin Bentmann made changes -
          Link This issue relates to MPLUGIN-150 [ MPLUGIN-150 ]
          Hide
          Paul Hammant added a comment -

          Benjamin, make a version for yourselves that works best (and field test), then get us the patch (or tell is what it was). It could be a bit trial and error if we try it any other way

          Show
          Paul Hammant added a comment - Benjamin, make a version for yourselves that works best (and field test), then get us the patch (or tell is what it was). It could be a bit trial and error if we try it any other way
          Hide
          Robert Scholte added a comment -

          During an IRC-session Benjamin and I have worked on this problem. Finally we came with this constructor-solution which seems very elegant. By using your own ClassLibrary you would have full control on the dependencies which may be used by the JavaDocBuilder. But now it seems it's not (yet) possible to create your own library because of the required JavaClassCache.
          It has become more of an architectural problem.
          Although the addClassLoader() is the most simple solution I don't really like this option. Maybe it's just not so nice that JavaDocBuilder implements JavaClassCache. It might be better to have the cache as a variable. It's just a first thought, can't see which complication might occur yet.
          Meanwhile it's not a problem if classes are missing, because of the try/catch-block, but it would be better if you can decide which classes are available on the classpath. Like the type says: it's a new feature, not a bug.

          Show
          Robert Scholte added a comment - During an IRC-session Benjamin and I have worked on this problem. Finally we came with this constructor-solution which seems very elegant. By using your own ClassLibrary you would have full control on the dependencies which may be used by the JavaDocBuilder . But now it seems it's not (yet) possible to create your own library because of the required JavaClassCache . It has become more of an architectural problem. Although the addClassLoader() is the most simple solution I don't really like this option. Maybe it's just not so nice that JavaDocBuilder implements JavaClassCache . It might be better to have the cache as a variable. It's just a first thought, can't see which complication might occur yet. Meanwhile it's not a problem if classes are missing, because of the try/catch-block, but it would be better if you can decide which classes are available on the classpath. Like the type says: it's a new feature, not a bug.
          Hide
          Robert Scholte added a comment -

          r600 | rfscholte | 2009-05-21 13:13:00 CEST

          I've introduced a JavaClassContext, to split the responsibilities of classes. You don't access the classLibrary or cache directly, but through the context. just ask the context for a JavaClass and it will look in it's cache. If it's not there, it'll use the classLibrary to get the definition of the class and with the javadocbuilder an new JavaClass will be generated and put into the cache.
          Important to know: JavaDocBuilder doesn't implement javaCache any longer.
          It's not anymore required to create a classLibrary with a cache, so everybody can have full control over the classloader.

          Show
          Robert Scholte added a comment - r600 | rfscholte | 2009-05-21 13:13:00 CEST I've introduced a JavaClassContext, to split the responsibilities of classes. You don't access the classLibrary or cache directly, but through the context. just ask the context for a JavaClass and it will look in it's cache. If it's not there, it'll use the classLibrary to get the definition of the class and with the javadocbuilder an new JavaClass will be generated and put into the cache. Important to know: JavaDocBuilder doesn't implement javaCache any longer. It's not anymore required to create a classLibrary with a cache, so everybody can have full control over the classloader.
          Robert Scholte made changes -
          Fix Version/s 1.10 [ 15020 ]
          Status Reopened [ 4 ] Resolved [ 5 ]
          Fix Version/s 1.9.1 [ 15252 ]
          Resolution Fixed [ 1 ]
          Paul Hammant made changes -
          Affects Version/s 1.9.2 [ 15471 ]
          Affects Version/s 1.6.1 [ 14090 ]
          Paul Hammant made changes -
          Fix Version/s 1.10 [ 15020 ]
          Affects Version/s 1.6.2 [ 14091 ]
          Affects Version/s 1.9.2 [ 15471 ]
          Fix Version/s 1.9.2 [ 15471 ]

            People

            • Assignee:
              Robert Scholte
              Reporter:
              Benjamin Bentmann
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: