JDO
  1. JDO
  2. JDO-621

Add javax.jdo.JDOEnhancerMain to call the enhancer via standard API

    Details

    • Type: New Feature New Feature
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: JDO 2 maintenance release 2 (2.2)
    • Fix Version/s: JDO 3 (3.0)
    • Component/s: api
    • Labels:
      None

      Description

      The proposed new class will use the standard enhancer API to enhance classes. The command line parameters would be similar to the original proposal of JDO-591, with changes as needed to use the new class in the TCK.

      java -cp classpath

      {enhancer-class}

      [options] [jdo-files] [class-files]
      where options can be
      -persistenceUnit persistence-unit-name : Name of a "persistence-unit" to enhance the classes for
      -d target-dir-name : Write the enhanced classes to the specified directory
      -checkonly : Just check the classes for enhancement status
      -v : verbose output

      1. jdo-621.patch
        59 kB
        Craig L Russell

        Issue Links

          Activity

          Hide
          Andy Jefferson added a comment -

          If anyone gets around to this before me, you could take code from the current DataNucleusEnhancer as a start point, which has a main() and similar args and uses similarly named methods to JDOEnhancer.
          See
          http://datanucleus.svn.sourceforge.net/viewvc/datanucleus/platform/enhancer/trunk/src/java/org/datanucleus/enhancer/DataNucleusEnhancer.java?revision=4202&view=markup
          near the end.

          Show
          Andy Jefferson added a comment - If anyone gets around to this before me, you could take code from the current DataNucleusEnhancer as a start point, which has a main() and similar args and uses similarly named methods to JDOEnhancer. See http://datanucleus.svn.sourceforge.net/viewvc/datanucleus/platform/enhancer/trunk/src/java/org/datanucleus/enhancer/DataNucleusEnhancer.java?revision=4202&view=markup near the end.
          Hide
          Ilan Kirsh added a comment -

          I would prefer:
          javax.jdo.Enhancer (instead of javax.jdo.JDOEnhancerMain)
          -pu (instead of -persistenceUnit, as -d and not -directory and -v and not -verbose)

          > java javax.jdo.Enhancer -pu my

          is easier to type (and remember) than

          > java javax.jdo.JDOEnhancerMain -persistenceUnit my

          Show
          Ilan Kirsh added a comment - I would prefer: javax.jdo.Enhancer (instead of javax.jdo.JDOEnhancerMain) -pu (instead of -persistenceUnit, as -d and not -directory and -v and not -verbose) > java javax.jdo.Enhancer -pu my is easier to type (and remember) than > java javax.jdo.JDOEnhancerMain -persistenceUnit my
          Hide
          Ilan Kirsh added a comment -

          Following is the current ObjectDB Enhancer usage:

          Usage: java com.objectdb.Enhancer [ <options> | <class> | <filename> ] ...
          <class> - name of a class (without .class suffix) in the CLASSPATH
          <filename> - path to class or jar file(s), *? wildcards supported
          <options> include:
          -cp <dir> : path to input user classes
          -pu <name> : persistence unit name
          -s : include sub directories in search
          -d <dir> : output path for enhanced classes

          It would be nice to support wlidcards and subdirectories search:

          > java javax.jdo.Enhancer -s *.class

          Also, javax.jdo.Enhancer should send all the arguments, including unknown arguments to the implementation.

          Show
          Ilan Kirsh added a comment - Following is the current ObjectDB Enhancer usage: Usage: java com.objectdb.Enhancer [ <options> | <class> | <filename> ] ... <class> - name of a class (without .class suffix) in the CLASSPATH <filename> - path to class or jar file(s), *? wildcards supported <options> include: -cp <dir> : path to input user classes -pu <name> : persistence unit name -s : include sub directories in search -d <dir> : output path for enhanced classes It would be nice to support wlidcards and subdirectories search: > java javax.jdo.Enhancer -s *.class Also, javax.jdo.Enhancer should send all the arguments, including unknown arguments to the implementation.
          Hide
          Andy Jefferson added a comment -

          Whether it is called "Enhancer" or JDOEnhancerMain I really don't care since a user will invoke via a plugin or Ant task etc.

          All options should have shortform and longform, so "-pu" and "-persistenceUnit" should be valid (just as "-v"/"-verbose" will be)

          > Also, javax.jdo.Enhancer should send all the arguments, including unknown arguments to the implementation.

          How is it to do that ? The JDOEnhancerMain/Enhancer will be looking for an implementation of JDOEnhancer in the classpath via the services registry. When it finds one it creates an instance and makes use of the JDOEnhancer API. This API defines what features are supported.

          Show
          Andy Jefferson added a comment - Whether it is called "Enhancer" or JDOEnhancerMain I really don't care since a user will invoke via a plugin or Ant task etc. All options should have shortform and longform, so "-pu" and "-persistenceUnit" should be valid (just as "-v"/"-verbose" will be) > Also, javax.jdo.Enhancer should send all the arguments, including unknown arguments to the implementation. How is it to do that ? The JDOEnhancerMain/Enhancer will be looking for an implementation of JDOEnhancer in the classpath via the services registry. When it finds one it creates an instance and makes use of the JDOEnhancer API. This API defines what features are supported.
          Hide
          Ilan Kirsh added a comment -

          It is a good idea to have long and short forms for all options.

          Unknown options can be sent to a new method of JDOEnhancer, setHints(String... hints) , or addHints, but with that solution an exception will have to be thrown for unsupported hints. Actually, the only hint that I need for ObjectDB at the moment is "-s", if support for searching subdirectories is not added to the standard.

          Another issue, which JDOEnhancer implementation will be used when more than one implementation is found?

          Show
          Ilan Kirsh added a comment - It is a good idea to have long and short forms for all options. Unknown options can be sent to a new method of JDOEnhancer, setHints(String... hints) , or addHints, but with that solution an exception will have to be thrown for unsupported hints. Actually, the only hint that I need for ObjectDB at the moment is "-s", if support for searching subdirectories is not added to the standard. Another issue, which JDOEnhancer implementation will be used when more than one implementation is found?
          Hide
          Craig L Russell added a comment -

          > Whether it is called "Enhancer" or JDOEnhancerMain I really don't care since a user will invoke via a plugin or Ant task etc.

          I originally called it JDOEnhancerMain to distinguish it from the name of the interface. But javax.jdo.Enhancer is fine since it doesn't conflict.

          > It would be nice to support wlidcards and subdirectories search:

          I agree. So you could have javax.jdo.Enhancer -s /home/clr/myproject/target/classes
          and all of the classes in the target/classes dir would be enhanced using metadata from the target/classes directory.

          Would you also support javax.jdo.Enhancer -s /home/clr/myproject/target/classes/com/myproject
          as well? Then the enhancer would only enhance the classes beginning with com.myproject. But the output directory -d option would still put the classes in the "right place" based on the complete class name.

          > All options should have shortform and longform, so "-pu" and "-persistenceUnit" should be valid (just as "-v"/"-verbose" will be)

          I agree. How about setSearchSubdirectories(boolean flag).

          > > Also, javax.jdo.Enhancer should send all the arguments, including unknown arguments to the implementation.

          > How is it to do that ? The JDOEnhancerMain/Enhancer will be looking for an implementation of JDOEnhancer in the classpath via the services registry. When it finds one it creates an instance and makes use of the JDOEnhancer API. This API defines what features are supported.

          We have always supported extension points for the JDO API and I don't think this should be an exception.

          I can see how to support flags that are passed as a single element of the args array; that is, they don't take arguments. These could be collected and passed as Ilan suggests as a String[ ] of hints. I don't see how to pass flags with arguments, since the argument would be treated as a file or class name.

          I'd propose that any hints would be ignored if not recognized, just as we do in other parts of the spec.

          > Another issue, which JDOEnhancer implementation will be used when more than one implementation is found?

          The way it works now is that it takes the first implementation that it finds.

          We can add a property to the persistence.xml to specify the implementation, similar to the way the PMF is specified in jdoconfig.xml. And could we use something in jdoconfig.xml?

          If you like, propose something different.

          Show
          Craig L Russell added a comment - > Whether it is called "Enhancer" or JDOEnhancerMain I really don't care since a user will invoke via a plugin or Ant task etc. I originally called it JDOEnhancerMain to distinguish it from the name of the interface. But javax.jdo.Enhancer is fine since it doesn't conflict. > It would be nice to support wlidcards and subdirectories search: I agree. So you could have javax.jdo.Enhancer -s /home/clr/myproject/target/classes and all of the classes in the target/classes dir would be enhanced using metadata from the target/classes directory. Would you also support javax.jdo.Enhancer -s /home/clr/myproject/target/classes/com/myproject as well? Then the enhancer would only enhance the classes beginning with com.myproject. But the output directory -d option would still put the classes in the "right place" based on the complete class name. > All options should have shortform and longform, so "-pu" and "-persistenceUnit" should be valid (just as "-v"/"-verbose" will be) I agree. How about setSearchSubdirectories(boolean flag). > > Also, javax.jdo.Enhancer should send all the arguments, including unknown arguments to the implementation. > How is it to do that ? The JDOEnhancerMain/Enhancer will be looking for an implementation of JDOEnhancer in the classpath via the services registry. When it finds one it creates an instance and makes use of the JDOEnhancer API. This API defines what features are supported. We have always supported extension points for the JDO API and I don't think this should be an exception. I can see how to support flags that are passed as a single element of the args array; that is, they don't take arguments. These could be collected and passed as Ilan suggests as a String[ ] of hints. I don't see how to pass flags with arguments, since the argument would be treated as a file or class name. I'd propose that any hints would be ignored if not recognized, just as we do in other parts of the spec. > Another issue, which JDOEnhancer implementation will be used when more than one implementation is found? The way it works now is that it takes the first implementation that it finds. We can add a property to the persistence.xml to specify the implementation, similar to the way the PMF is specified in jdoconfig.xml. And could we use something in jdoconfig.xml? If you like, propose something different.
          Hide
          Ilan Kirsh added a comment -

          > Would you also support javax.jdo.Enhancer -s /home/clr/myproject/target/classes/com/myproject?

          The following should be supported:
          javax.jdo.Enhancer -s /home/clr/myproject/target/classes/com/myproject/*.class
          because currently you have to specify files not a directory, but of course, this could be changed.

          > I'd propose that any hints would be ignored if not recognized, just as we do in other parts of the spec.

          The drawback is inaccurate response in a case of misspelling, etc.

          Show
          Ilan Kirsh added a comment - > Would you also support javax.jdo.Enhancer -s /home/clr/myproject/target/classes/com/myproject? The following should be supported: javax.jdo.Enhancer -s /home/clr/myproject/target/classes/com/myproject/*.class because currently you have to specify files not a directory, but of course, this could be changed. > I'd propose that any hints would be ignored if not recognized, just as we do in other parts of the spec. The drawback is inaccurate response in a case of misspelling, etc.
          Hide
          Craig L Russell added a comment -

          Please review this patch. It adds the new javax.jdo.Enhancer main class to invoke the enhancer.

          There are a few things that can be improved. There is no checking that a file is actually loadable, or that a .class file actually contains a class, or that a .jdo file has an xml formatted metadata document. There is so little processing done that I'd prefer that the main program simply pass all of the file names to the JDOEnhancer instead of trying to figure out what kind of file is there. That would involve removing the addClasses and addJarFile methods.

          There is also no processing of the -cp <extra class path> parameter except to take the parameter and assign it to the classLoaderParameter. This will be done for the next version of the patch.

          One oddity in the JDOEnhancer API: it only allows the processing of one jar file. Can anyone explain this?

          Show
          Craig L Russell added a comment - Please review this patch. It adds the new javax.jdo.Enhancer main class to invoke the enhancer. There are a few things that can be improved. There is no checking that a file is actually loadable, or that a .class file actually contains a class, or that a .jdo file has an xml formatted metadata document. There is so little processing done that I'd prefer that the main program simply pass all of the file names to the JDOEnhancer instead of trying to figure out what kind of file is there. That would involve removing the addClasses and addJarFile methods. There is also no processing of the -cp <extra class path> parameter except to take the parameter and assign it to the classLoaderParameter. This will be done for the next version of the patch. One oddity in the JDOEnhancer API: it only allows the processing of one jar file. Can anyone explain this?
          Hide
          Andy Jefferson added a comment -

          > One oddity in the JDOEnhancer API: it only allows the processing of one jar file. Can anyone explain this?

          JDOEnhancer has a method addJar(String jarFilename) so by definition can take in multiple by calling more than once. Or maybe that isn't what you mean ?

          Show
          Andy Jefferson added a comment - > One oddity in the JDOEnhancer API: it only allows the processing of one jar file. Can anyone explain this? JDOEnhancer has a method addJar(String jarFilename) so by definition can take in multiple by calling more than once. Or maybe that isn't what you mean ?
          Hide
          Craig L Russell added a comment -

          The oddity is this:
          /**

          • Add class(es) to the items to be enhanced.
            */
            JDOEnhancer addClasses(String... classNames);

          /**

          • Add metadata file(s) to the items to be enhanced.
            */
            JDOEnhancer addFiles(String... metadataFiles);

          /**

          • Add a jar file to the items to be enhanced.
            */
            JDOEnhancer addJar(String jarFileName);

          The signatures for addFiles and addClasses take a String... but the addJar takes a String. Why are jar files treated differently?

          My suggestion is to remove addJar and addClasses and use the addFiles for any of the four types of files: .jar, .class, .jdo, or directories.

          Similarly, if multiple persistence units are allowed, shouldn't we have addPersistenceUnits(String... pus) instead.

          And as long as we're talking about serious changes, how about changing the signatures to use Iterable<String> as the parameter type instead of String or String...? The main program is using an ArrayList<String> for its holder and the implementation side is clearly going to iterate anything that is passed.

          Show
          Craig L Russell added a comment - The oddity is this: /** Add class(es) to the items to be enhanced. */ JDOEnhancer addClasses(String... classNames); /** Add metadata file(s) to the items to be enhanced. */ JDOEnhancer addFiles(String... metadataFiles); /** Add a jar file to the items to be enhanced. */ JDOEnhancer addJar(String jarFileName); The signatures for addFiles and addClasses take a String... but the addJar takes a String. Why are jar files treated differently? My suggestion is to remove addJar and addClasses and use the addFiles for any of the four types of files: .jar, .class, .jdo, or directories. Similarly, if multiple persistence units are allowed, shouldn't we have addPersistenceUnits(String... pus) instead. And as long as we're talking about serious changes, how about changing the signatures to use Iterable<String> as the parameter type instead of String or String...? The main program is using an ArrayList<String> for its holder and the implementation side is clearly going to iterate anything that is passed.
          Hide
          Craig L Russell added a comment -

          I'm also still not sold on the requirement to pass resource names to the APIs. Why aren't file names sufficient?

          Show
          Craig L Russell added a comment - I'm also still not sold on the requirement to pass resource names to the APIs. Why aren't file names sufficient?
          Hide
          Andy Jefferson added a comment -

          How can somebody pass in a "filename" when they are using the enhancer programmatically and they know the class name ? (e.g they have a class present called "my.pkg.MyClass") so it is somewhere in the classpath and that's all they know. Not everyone is invoking this from command lines.

          The method names for addXXX were based around the primary use-cases that I've come across for a typical application (and what clients are using now with this API). This "typical" app consists of multiple classes, or multiple XML files (or multiple classes + XML files), or a persistence-unit, or a jar. Yes, an app could put their model classes in multiple jars, or multiple persistence-units but then that's why we have them called addXXX. That was the reasoning anyway and I don't see where it is no longer valid.

          The method addFiles() taking in XML files could do with a rename anyway, since it is explicitly for metadata XML files so should state that.

          Yes we could add a generic method addFiles(...) certainly, and/or rename the addJar/addPersistenceUnit to take in plural. I'm against removal of those specific type methods though since if the user knows what type they already have then why inflict on the implementation the need to determine it again ?

          Show
          Andy Jefferson added a comment - How can somebody pass in a "filename" when they are using the enhancer programmatically and they know the class name ? (e.g they have a class present called "my.pkg.MyClass") so it is somewhere in the classpath and that's all they know. Not everyone is invoking this from command lines. The method names for addXXX were based around the primary use-cases that I've come across for a typical application (and what clients are using now with this API). This "typical" app consists of multiple classes, or multiple XML files (or multiple classes + XML files), or a persistence-unit, or a jar. Yes, an app could put their model classes in multiple jars, or multiple persistence-units but then that's why we have them called addXXX. That was the reasoning anyway and I don't see where it is no longer valid. The method addFiles() taking in XML files could do with a rename anyway, since it is explicitly for metadata XML files so should state that. Yes we could add a generic method addFiles(...) certainly, and/or rename the addJar/addPersistenceUnit to take in plural. I'm against removal of those specific type methods though since if the user knows what type they already have then why inflict on the implementation the need to determine it again ?
          Hide
          Craig L Russell added a comment -

          Please review this patch.

          Most functionality of Enhancer is implemented and tested using a mock JDOEnhancer.

          Still missing: classpath processing

          Some notes:

          Usage is always printed to stderr, not stdout.

          Multiple persistence unit names are accepted and passed to JDOEnhancer, one at a time.

          Multiple jar file names are accepted and passed to JDOEnhancer, one at a time.

          Only one classpath entry is expected, but currently no error checking on the second one

          Only one output directory is expected, but currently no error checking on the second one

          Testing is done by adding test/resources to the test class path but these artifacts aren't put into the api2 jar file.

          Comments?

          Show
          Craig L Russell added a comment - Please review this patch. Most functionality of Enhancer is implemented and tested using a mock JDOEnhancer. Still missing: classpath processing Some notes: Usage is always printed to stderr, not stdout. Multiple persistence unit names are accepted and passed to JDOEnhancer, one at a time. Multiple jar file names are accepted and passed to JDOEnhancer, one at a time. Only one classpath entry is expected, but currently no error checking on the second one Only one output directory is expected, but currently no error checking on the second one Testing is done by adding test/resources to the test class path but these artifacts aren't put into the api2 jar file. Comments?
          Hide
          Craig L Russell added a comment -

          Please review this patch.

          It is complete except for testing with the tck. The next task is to invoke the Enhancer from within the tck to enhance the classes instead of calling the RI enhancer directly.

          Show
          Craig L Russell added a comment - Please review this patch. It is complete except for testing with the tck. The next task is to invoke the Enhancer from within the tck to enhance the classes instead of calling the RI enhancer directly.
          Hide
          Craig L Russell added a comment -

          This fixes the problem noticed by Michael that the wrong path separator is used when constructing the java command.

          Show
          Craig L Russell added a comment - This fixes the problem noticed by Michael that the wrong path separator is used when constructing the java command.
          Hide
          Craig L Russell added a comment -

          Please try this patch.

          It fixes the EnhancerTest to create a thread for stdout and stderr streams so even Windows should be able to complete the Enhancer process.

          Show
          Craig L Russell added a comment - Please try this patch. It fixes the EnhancerTest to create a thread for stdout and stderr streams so even Windows should be able to complete the Enhancer process.
          Hide
          Michael Bouschen added a comment -

          The new patch looks good!

          I tried it on Windows and the EnhancerTest completes successfully. So reading each of the error and output streams in its own Thread solves the hanging problem on Windows.

          Show
          Michael Bouschen added a comment - The new patch looks good! I tried it on Windows and the EnhancerTest completes successfully. So reading each of the error and output streams in its own Thread solves the hanging problem on Windows.
          Hide
          Craig L Russell added a comment -

          Please review this patch. It is for the api2 project as well as the tck2 project.

          The changes since the last patch:

          1. The JDOEnhancerTest uses the basedir (maven) system property instead of <current-directory> to allow running the api2 build/test from either the api2 directory or the trunk (branch) directory.

          2. Updates the tck2 signature file to incorporate the changes to Constants.java.

          3. Updates the tck2 project.properties file to use the standard javax.jdo.Enhancer to enhance the tck model classes.

          I've run the tck in both ri and tck mode with these changes.

          Show
          Craig L Russell added a comment - Please review this patch. It is for the api2 project as well as the tck2 project. The changes since the last patch: 1. The JDOEnhancerTest uses the basedir (maven) system property instead of <current-directory> to allow running the api2 build/test from either the api2 directory or the trunk (branch) directory. 2. Updates the tck2 signature file to incorporate the changes to Constants.java. 3. Updates the tck2 project.properties file to use the standard javax.jdo.Enhancer to enhance the tck model classes. I've run the tck in both ri and tck mode with these changes.
          Hide
          Michael Bouschen added a comment -

          The patch looks good!

          Just two comments:

          • Class javax.jdo.Enhancer implements the interface Constants in order to access the constants defines in the interface. I propose to consider a static import of the needed constants instead of implementing the interface. The same holds true for class MockEnhancer.
          • I think the related issue JDO-591 can be set to resolved as soon as this issue is resolved.
          Show
          Michael Bouschen added a comment - The patch looks good! Just two comments: Class javax.jdo.Enhancer implements the interface Constants in order to access the constants defines in the interface. I propose to consider a static import of the needed constants instead of implementing the interface. The same holds true for class MockEnhancer. I think the related issue JDO-591 can be set to resolved as soon as this issue is resolved.
          Hide
          Craig L Russell added a comment -

          The patch has been applied.

          svn commit -m "JDO-621 Add javax.jdo.Enhancer to API" api2
          Sending api2/pom.xml
          Sending api2/project.xml
          Sending api2/src/java/javax/jdo/Bundle.properties
          Sending api2/src/java/javax/jdo/Constants.java
          Adding api2/src/java/javax/jdo/Enhancer.java
          Sending api2/src/java/javax/jdo/JDOHelper.java
          Adding api2/test/java/javax/jdo/EnhancerTest.java
          Adding api2/test/java/javax/jdo/MockEnhancer.java
          Adding api2/test/resources
          Adding api2/test/resources/META-INF
          Adding api2/test/resources/META-INF/services
          Adding api2/test/resources/META-INF/services/javax.jdo.JDOEnhancer
          Transmitting file data .........
          Committed revision 818642.

          svn commit -m "JDO-621 Adapted TCK2 to use javax.jdo.Enhancer when running with RI" api2 tck2/project.properties tck2/src/conf/jdo-2_3-signatures.txt
          Sending api2/src/java/javax/jdo/Constants.java
          Sending tck2/project.properties
          Sending tck2/src/conf/jdo-2_3-signatures.txt
          Transmitting file data ...
          Committed revision 818646.

          Show
          Craig L Russell added a comment - The patch has been applied. svn commit -m " JDO-621 Add javax.jdo.Enhancer to API" api2 Sending api2/pom.xml Sending api2/project.xml Sending api2/src/java/javax/jdo/Bundle.properties Sending api2/src/java/javax/jdo/Constants.java Adding api2/src/java/javax/jdo/Enhancer.java Sending api2/src/java/javax/jdo/JDOHelper.java Adding api2/test/java/javax/jdo/EnhancerTest.java Adding api2/test/java/javax/jdo/MockEnhancer.java Adding api2/test/resources Adding api2/test/resources/META-INF Adding api2/test/resources/META-INF/services Adding api2/test/resources/META-INF/services/javax.jdo.JDOEnhancer Transmitting file data ......... Committed revision 818642. svn commit -m " JDO-621 Adapted TCK2 to use javax.jdo.Enhancer when running with RI" api2 tck2/project.properties tck2/src/conf/jdo-2_3-signatures.txt Sending api2/src/java/javax/jdo/Constants.java Sending tck2/project.properties Sending tck2/src/conf/jdo-2_3-signatures.txt Transmitting file data ... Committed revision 818646.

            People

            • Assignee:
              Craig L Russell
              Reporter:
              Craig L Russell
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development