Uploaded image for project: 'OpenJPA'
  1. OpenJPA
  2. OPENJPA-2632

select new not working if result class is not in same classloader

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.4.2
    • Component/s: jdbc
    • Labels:
      None

      Description

      in my case i'm using an ear with multiple war files.

      the entities are located in ear/lib and my select new result class and the service which loads it are located in a war (doesn't matter if in WEB-INF/lib or WEB-INF/classes)

      openJPA uses the CL stored in QueryImpl but this one can not load my class.

      imo if the class can not be loaded with the stored class loader, we can try to use the TCCL to load the class.

      1. OPENJPA-2632.patch
        7 kB
        Reinhard Sandtner

        Activity

        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user rsandtner opened a pull request:

        https://github.com/apache/openjpa/pull/4

        OPENJPA-2632 use TCCL as fallback

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/rsandtner/openjpa OPENJPA-2632

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/openjpa/pull/4.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #4


        commit b44db2f7754c785da44f084849e7caac9ff216e9
        Author: rsandtner <rsandtner@apache.org>
        Date: 2016-03-04T13:44:25Z

        OPENJPA-2632 use TCCL as fallback


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user rsandtner opened a pull request: https://github.com/apache/openjpa/pull/4 OPENJPA-2632 use TCCL as fallback You can merge this pull request into a Git repository by running: $ git pull https://github.com/rsandtner/openjpa OPENJPA-2632 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/openjpa/pull/4.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #4 commit b44db2f7754c785da44f084849e7caac9ff216e9 Author: rsandtner <rsandtner@apache.org> Date: 2016-03-04T13:44:25Z OPENJPA-2632 use TCCL as fallback
        Hide
        rsandtner Reinhard Sandtner added a comment -

        pull request is also available here https://github.com/apache/openjpa/pull/4
        patch was made with git

        i've no idea how to add a unit test for this issue. you can reproduce the issue with https://github.com/struberg/was_bugs/tree/master/was_bug19

        mvn clean install tomee:run in was_bug19_ear
        browse to http://localhost:8080/was_bug19
        click 'Insert Data' -> 1000 entries will be inserted
        click the 'Load Data' -> kawumm...

        after appliing the patch just do the same with -Dopenjpa.version=2.4.2-SNAPSHOT and it should work

        Show
        rsandtner Reinhard Sandtner added a comment - pull request is also available here https://github.com/apache/openjpa/pull/4 patch was made with git i've no idea how to add a unit test for this issue. you can reproduce the issue with https://github.com/struberg/was_bugs/tree/master/was_bug19 mvn clean install tomee:run in was_bug19_ear browse to http://localhost:8080/was_bug19 click 'Insert Data' -> 1000 entries will be inserted click the 'Load Data' -> kawumm... after appliing the patch just do the same with -Dopenjpa.version=2.4.2-SNAPSHOT and it should work
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Reinhard Sandtnernot sure i get it, if entities are in a parent classloader it should work.Falling back on tccl can create a mess in OSGi and it shoudlnt be done since the broker should be limited to one classloader and not drag all classes (suppose you have 2 wars with the same class names)

        To add a test propably just create a hierarchic classloader (you can take batchee url class loader first impl).

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Reinhard Sandtner not sure i get it, if entities are in a parent classloader it should work.Falling back on tccl can create a mess in OSGi and it shoudlnt be done since the broker should be limited to one classloader and not drag all classes (suppose you have 2 wars with the same class names) To add a test propably just create a hierarchic classloader (you can take batchee url class loader first impl).
        Hide
        rsandtner Reinhard Sandtner added a comment -

        Romain Manni-Bucau my problem is, that the class for my 'select new' can not be found by the CL stored in query impl. Strings.toClass(name, _loader) blows up with a ClassNotFoundException which is catched away by catch (Throwable t) and wrapped in an IllegalArgumentException(t.toString()).

        missed to write the exact problem in the issue description...

        but of course i think the fallback in the provided patch is in the wrong place... maybe it is better to load the result class from TCCL as fallback where we try to find the constructor in JPQLExpressionBuilder? and if that fails too, then throw the 'no-constructor' exception.

        Show
        rsandtner Reinhard Sandtner added a comment - Romain Manni-Bucau my problem is, that the class for my 'select new' can not be found by the CL stored in query impl. Strings.toClass(name, _loader) blows up with a ClassNotFoundException which is catched away by catch (Throwable t) and wrapped in an IllegalArgumentException(t.toString()) . missed to write the exact problem in the issue description... but of course i think the fallback in the provided patch is in the wrong place... maybe it is better to load the result class from TCCL as fallback where we try to find the constructor in JPQLExpressionBuilder ? and if that fails too, then throw the 'no-constructor' exception.
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Reinhard Sandtner can you try to describe or draw the classloader hierarchy you have (making explicit where is the entity and the broker classloaders). I understand you do a em.createxxxQuery(..., Foo.class) with Foo in WEB-INF/* and em in lib/META-INF/persistence.xml so it sounds normal it fails. Do I miss something?

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Reinhard Sandtner can you try to describe or draw the classloader hierarchy you have (making explicit where is the entity and the broker classloaders). I understand you do a em.createxxxQuery(..., Foo.class) with Foo in WEB-INF/* and em in lib/META-INF/persistence.xml so it sounds normal it fails. Do I miss something?
        Hide
        rsandtner Reinhard Sandtner added a comment -

        hey,

        my ear looks like this (simplified for the sample - see also https://github.com/struberg/was_bugs/tree/master/was_bug19)

        ear
        | - lib
        |    | - myjar
        |    |      | - TheEntity.class
        |    |      | - persistence.xml
        | - mywar
        |    | - WEB-INF
        |    |      | - classes
        |    |      |      | - Result.class
        |    |      |      | - ServiceUsingResult.class
        

        as i understand, the entity gets loaded via the ear-CL and this did not know the Result.class. i've not found any restrictions in the spec that the result class must be in the same classloader as the entity.

        Show
        rsandtner Reinhard Sandtner added a comment - hey, my ear looks like this (simplified for the sample - see also https://github.com/struberg/was_bugs/tree/master/was_bug19 ) ear | - lib | | - myjar | | | - TheEntity.class | | | - persistence.xml | - mywar | | - WEB-INF | | | - classes | |  | | - Result.class | | | | - ServiceUsingResult.class as i understand, the entity gets loaded via the ear-CL and this did not know the Result.class . i've not found any restrictions in the spec that the result class must be in the same classloader as the entity.
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Reinhard Sandtner you are right no restriction but it is an open door to leak - in particular with the enhancement the spec implies in some cases. What about adding your patch if a flag is set in the persistence unit properties? If not set or false then keep current behavior, if set to true use your patch. wdyt?

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Reinhard Sandtner you are right no restriction but it is an open door to leak - in particular with the enhancement the spec implies in some cases. What about adding your patch if a flag is set in the persistence unit properties? If not set or false then keep current behavior, if set to true use your patch. wdyt?
        Hide
        rsandtner Reinhard Sandtner added a comment -

        Romain Manni-Bucau sounds good... but let me try to resolve the class via TCCL only for select new. i think this way it is more secure.

        Show
        rsandtner Reinhard Sandtner added a comment - Romain Manni-Bucau sounds good... but let me try to resolve the class via TCCL only for select new . i think this way it is more secure.
        Hide
        rsandtner Reinhard Sandtner added a comment -

        Romain Manni-Bucau can you take a look to the second patch pls.
        OPENJPA-2632_onlySelectNew.patch

        but this one neither makes me happy - maybe you have a better idea?

        Show
        rsandtner Reinhard Sandtner added a comment - Romain Manni-Bucau can you take a look to the second patch pls. OPENJPA-2632 _onlySelectNew.patch but this one neither makes me happy - maybe you have a better idea?
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        Think the flag is a good compromise (in particular cause openjpa is used in OSGi). Also take care Thread.currentThread().getContextClassLoader() can be null and IIRC openjpa has a utility method for that going through security manager if needed.

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - Think the flag is a good compromise (in particular cause openjpa is used in OSGi). Also take care Thread.currentThread().getContextClassLoader() can be null and IIRC openjpa has a utility method for that going through security manager if needed.
        Hide
        rsandtner Reinhard Sandtner added a comment -

        Romain Manni-Bucau can you pls take a another look if the 'fix' is acceptable now? https://github.com/rsandtner/openjpa/tree/OPENJPA-2632

        Show
        rsandtner Reinhard Sandtner added a comment - Romain Manni-Bucau can you pls take a another look if the 'fix' is acceptable now? https://github.com/rsandtner/openjpa/tree/OPENJPA-2632
        Hide
        romain.manni-bucau Romain Manni-Bucau added a comment -

        looks good

        Show
        romain.manni-bucau Romain Manni-Bucau added a comment - looks good
        Hide
        rsandtner Reinhard Sandtner added a comment -

        what is the next step now? pull request or a patch?

        Show
        rsandtner Reinhard Sandtner added a comment - what is the next step now? pull request or a patch?
        Hide
        struberg Mark Struberg added a comment -

        +1 looks good.

        Smallish things to check:

        + if (System.getSecurityManager() == null) {

        shouldn't that be != null?

        We also need to add the new flag to our documentation openjpa.option.UseTCCLinSelectNew

        Show
        struberg Mark Struberg added a comment - +1 looks good. Smallish things to check: + if (System.getSecurityManager() == null) { shouldn't that be != null? We also need to add the new flag to our documentation openjpa.option.UseTCCLinSelectNew
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1734966 from Mark Struberg in branch 'openjpa/trunk'
        [ https://svn.apache.org/r1734966 ]

        OPENJPA-2632 fallback to TCCL for select new if class cannot be found

        That might happen if the entities are loaded within a shared ear lib
        but the actual "select new" is performed from another ClassLoader (e.g. WAR)
        Contributed by Reinhard Sandtner (apacheId: rsandtner). Txs for the patch!

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1734966 from Mark Struberg in branch 'openjpa/trunk' [ https://svn.apache.org/r1734966 ] OPENJPA-2632 fallback to TCCL for select new if class cannot be found That might happen if the entities are loaded within a shared ear lib but the actual "select new" is performed from another ClassLoader (e.g. WAR) Contributed by Reinhard Sandtner (apacheId: rsandtner). Txs for the patch!
        Hide
        struberg Mark Struberg added a comment -

        patch applied, txs Reinhard!
        Will just add some docs for it.

        Show
        struberg Mark Struberg added a comment - patch applied, txs Reinhard! Will just add some docs for it.
        Hide
        jira-bot ASF subversion and git services added a comment -

        Commit 1735190 from Mark Struberg in branch 'openjpa/trunk'
        [ https://svn.apache.org/r1735190 ]

        OPENJPA-2632 add documentation for the new UseTCCLinSelectNew config switch

        Show
        jira-bot ASF subversion and git services added a comment - Commit 1735190 from Mark Struberg in branch 'openjpa/trunk' [ https://svn.apache.org/r1735190 ] OPENJPA-2632 add documentation for the new UseTCCLinSelectNew config switch
        Hide
        ilgrosso Francesco Chicchiriccò added a comment -

        Bulk close for 2.4.2

        Show
        ilgrosso Francesco Chicchiriccò added a comment - Bulk close for 2.4.2

          People

          • Assignee:
            struberg Mark Struberg
            Reporter:
            rsandtner Reinhard Sandtner
          • Votes:
            0 Vote for this issue
            Watchers:
            6 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development