OpenJPA
  1. OpenJPA
  2. OPENJPA-932

Runtime enhancer doesn't work propery if there is a trailing persistence.xml file on the classpath.

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.2.0, 2.0.0-M1, 2.0.0-M2
    • Fix Version/s: 1.3.0, 2.0.0-M3
    • Component/s: kernel
    • Labels:
      None

      Description

      I have an application where I am using runtime class enhancement via the javaagent and I set openjpa.RuntimeUnenhancedClasses=unsupported. My application works fine when running on 1.1.0 but when I moved to 1.2.0 (and 2.0.0) I started getting errors because my classes aren't being enhanced.

      After debugging I determined that I had a jar on the end of my classpath that had a META-INF/persistence.xml file in it. The extra persistence.xml file has no PUs defined, and should have been ignored. When I removed that jar from my classpath things starting working again. It also works if I put my META-INF/persistence.xml file on the end of my classpath.

      1. OPENJPA932.test.patch
        7 kB
        Rick Curtis
      2. OPENJPA-932.patch
        10 kB
        Rick Curtis
      3. OPENJPA-932.patch
        12 kB
        Rick Curtis

        Activity

        Hide
        Rick Curtis added a comment -

        The bug is that we parse all URLs matching the provided resource name, but only return the PU names from the last URL that was parsed. We should be saving the results after each time we parse a URL and returning ALL results.

        In addition to the bug fix I added some error detection. If there are persistence unit name collisions for the provided resource name, a WARNING message will be logged. See below for a snippet from a test run.

        ...
        [java] 297 WARN [main] openjpa.Persistence - The persistence unit "jpa.connection-pool" was found in the following resources "[file:/C:/jpa/workspace-samples/projects/j
        pa.connection.pool/bin/META-INF/persistence.xml, jar:file:/C:/jpa/workspace-samples/projects/lib/test-bad.jar!/META-INF/persistence.xml]". Please correct the problem as it may
        have unexpected results.
        ...

        Show
        Rick Curtis added a comment - The bug is that we parse all URLs matching the provided resource name, but only return the PU names from the last URL that was parsed. We should be saving the results after each time we parse a URL and returning ALL results. In addition to the bug fix I added some error detection. If there are persistence unit name collisions for the provided resource name, a WARNING message will be logged. See below for a snippet from a test run. ... [java] 297 WARN [main] openjpa.Persistence - The persistence unit "jpa.connection-pool" was found in the following resources "[file:/C:/jpa/workspace-samples/projects/j pa.connection.pool/bin/META-INF/persistence.xml, jar: file:/C:/jpa/workspace-samples/projects/lib/test-bad.jar!/META-INF/persistence.xml ]". Please correct the problem as it may have unexpected results. ...
        Hide
        Rick Curtis added a comment -

        Updated the patch to get a logger in a different manner. This patch is for trunk.

        Show
        Rick Curtis added a comment - Updated the patch to get a logger in a different manner. This patch is for trunk.
        Hide
        Rick Curtis added a comment -

        Changed the way I obtained a logger once again.

        Show
        Rick Curtis added a comment - Changed the way I obtained a logger once again.
        Hide
        Michael Dick added a comment -

        Hi Rick,

        The current patch will always log to StdErr. I think we'd be better off saving the fact that there was a name collision and logging it after we've parsed the configuration.

        I'm also not sure we need a new logging level for this issue. Wouldn't one of the other levels, ie Runtime have sufficed?

        Show
        Michael Dick added a comment - Hi Rick, The current patch will always log to StdErr. I think we'd be better off saving the fact that there was a name collision and logging it after we've parsed the configuration. I'm also not sure we need a new logging level for this issue. Wouldn't one of the other levels, ie Runtime have sufficed?
        Hide
        Jeremy Bauer added a comment -

        Comments on patch dated 2009-03-06 08:24 AM:

        • I don't think a new trace group should be added specifically for this warning. (OpenJPA trace groups are well documented in the manual, so if a new group would need to be doc'd if consensus is that a new group should be added.). While this exception shows up during enhancement, it is in product derivation code so openjpa.Runtime may be better?
        • I'm still struggling a bit with constructing a new config to get a logger, but OpenJPA doesn't really have anything configured at this point and I can't think of a cleaner way to get this important warning out there without other potentially messy changes. The null check is good, IMHO. The comment regarding its necessity can be removed.
        • The error message isn't completely clear as to what the problem is or how to fix it. I think wording which includes the notion that "persistence unit names should be unique" may help. (I'd say must, but as this problem points out, we don't enforce it)
        Show
        Jeremy Bauer added a comment - Comments on patch dated 2009-03-06 08:24 AM: I don't think a new trace group should be added specifically for this warning. (OpenJPA trace groups are well documented in the manual, so if a new group would need to be doc'd if consensus is that a new group should be added.). While this exception shows up during enhancement, it is in product derivation code so openjpa.Runtime may be better? I'm still struggling a bit with constructing a new config to get a logger, but OpenJPA doesn't really have anything configured at this point and I can't think of a cleaner way to get this important warning out there without other potentially messy changes. The null check is good, IMHO. The comment regarding its necessity can be removed. The error message isn't completely clear as to what the problem is or how to fix it. I think wording which includes the notion that "persistence unit names should be unique" may help. (I'd say must, but as this problem points out, we don't enforce it)
        Hide
        Rick Curtis added a comment -

        The new patch has the following updates:

        • Removed the Persistence trace group.
        • The previous PU name collision detection and logging was removed as it would only detect collisions when using the PCEnhancerAgent.
        • PU name collisions are now detected and saved away, but not logged until an EMF is created for that PU. The capability to make a PU name collision a fatal error exists, but I decided that course action is a bit extreme. I think new message is verbose enough that a user should be able to figure out what's going on if they're having problems.

        One question - Do I need a separate Jira for the PU name collision detection/logging since it is pretty much separate from the bug that this Jira was opened for?

        -Rick

        Show
        Rick Curtis added a comment - The new patch has the following updates: Removed the Persistence trace group. The previous PU name collision detection and logging was removed as it would only detect collisions when using the PCEnhancerAgent. PU name collisions are now detected and saved away, but not logged until an EMF is created for that PU. The capability to make a PU name collision a fatal error exists, but I decided that course action is a bit extreme. I think new message is verbose enough that a user should be able to figure out what's going on if they're having problems. One question - Do I need a separate Jira for the PU name collision detection/logging since it is pretty much separate from the bug that this Jira was opened for? -Rick
        Hide
        Rick Curtis added a comment -

        For the unit test I needed to have multiple META-INF/persistence.xml files on my classpath and the test validates that all PUs are found that exist in those xml files. I wasn't quite sure what the 'proper' way was to do this, but what I posted works... Please correct me if there is a better way.

        Note: The attached jar file needs to be expanded into openjpa-parent\openjpa-persistence\src\test.

        -Rick

        Show
        Rick Curtis added a comment - For the unit test I needed to have multiple META-INF/persistence.xml files on my classpath and the test validates that all PUs are found that exist in those xml files. I wasn't quite sure what the 'proper' way was to do this, but what I posted works... Please correct me if there is a better way. Note: The attached jar file needs to be expanded into openjpa-parent\openjpa-persistence\src\test. -Rick
        Hide
        Rick Curtis added a comment -

        Modified slightly to build the bad-persistence.jar rather than check it into svn.

        Show
        Rick Curtis added a comment - Modified slightly to build the bad-persistence.jar rather than check it into svn.
        Hide
        Jeremy Bauer added a comment -

        Comments on patches dated 2009-03-10 07:49 AM and 2009-03-11 08:41 AM.

        1) PUNameCollision and related methods in ProductDerivations should be moved to PersistenceProductDerivation to keep openjpa-lib free of persistence artifacts.

        2) Creating a jar is much preferred to checking one in, but I'm not sold on that yet either since (unless I'm mistaken) it is always in the classpath when running tests. Anyone have other ideas for testing this scenario?

        Show
        Jeremy Bauer added a comment - Comments on patches dated 2009-03-10 07:49 AM and 2009-03-11 08:41 AM. 1) PUNameCollision and related methods in ProductDerivations should be moved to PersistenceProductDerivation to keep openjpa-lib free of persistence artifacts. 2) Creating a jar is much preferred to checking one in, but I'm not sold on that yet either since (unless I'm mistaken) it is always in the classpath when running tests. Anyone have other ideas for testing this scenario?
        Hide
        Rick Curtis added a comment -
        • Refactored the code in response to comment one from above.
        Show
        Rick Curtis added a comment - Refactored the code in response to comment one from above.
        Hide
        Jeremy Bauer added a comment -

        Rick - I applied this patch and have a few comments/issues.

        • I'm OK with loading an additional jar to the test classpath in the openjpa-persistence sub-project since there are very few tests that run in that package. The code is only testing a warning path and shouldn't affect the behavior of those tests.
        • I could not force the production of the new warning message using the provided code (which I think is the intent of bad-persistence.xml) since the persistence.xml's all use unique pu names. Also, there was no test code to drive the code where the warning is logged. Is one of the names used in bad-persistence supposed to be a duplicate? Based on the code changes, the warning does not get logged unless one tries to create an emf with pu name that is a duplicate. I created a simple test program to force the condition and it worked fine, but I think a test needs to be provided in order to produce and validate the warning condition, if possible.
        Show
        Jeremy Bauer added a comment - Rick - I applied this patch and have a few comments/issues. I'm OK with loading an additional jar to the test classpath in the openjpa-persistence sub-project since there are very few tests that run in that package. The code is only testing a warning path and shouldn't affect the behavior of those tests. I could not force the production of the new warning message using the provided code (which I think is the intent of bad-persistence.xml) since the persistence.xml's all use unique pu names. Also, there was no test code to drive the code where the warning is logged. Is one of the names used in bad-persistence supposed to be a duplicate? Based on the code changes, the warning does not get logged unless one tries to create an emf with pu name that is a duplicate. I created a simple test program to force the condition and it worked fine, but I think a test needs to be provided in order to produce and validate the warning condition, if possible.
        Hide
        Rick Curtis added a comment -

        Looks like I forgot add my test case to SVN when I built the previous patch.

        Show
        Rick Curtis added a comment - Looks like I forgot add my test case to SVN when I built the previous patch.
        Hide
        Jeremy Bauer added a comment -

        Committed patch(es) for Rick to trunk under revision 769901.

        Show
        Jeremy Bauer added a comment - Committed patch(es) for Rick to trunk under revision 769901.
        Hide
        Rick Curtis added a comment -

        The attached file removes changes I introduced in the pom.xml to test this patch. All of the magic is now contained within the test.

        Show
        Rick Curtis added a comment - The attached file removes changes I introduced in the pom.xml to test this patch. All of the magic is now contained within the test.
        Hide
        Jeremy Bauer added a comment -

        OPENJPA932.test.patch committed for Rick under revision 774393. Nice work, Rick. The code you've added to dynamically create and load a persistence jar on the fly would be a great addition to a general test utility.

        Show
        Jeremy Bauer added a comment - OPENJPA932.test.patch committed for Rick under revision 774393. Nice work, Rick. The code you've added to dynamically create and load a persistence jar on the fly would be a great addition to a general test utility.

          People

          • Assignee:
            Michael Dick
            Reporter:
            Rick Curtis
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development