Details

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

      Description

      Having a standard interface to invoke the enhancer makes a lot of sense so we can have interchangeability of enhancers (for implementations that support BinaryCompatibility).

      A start point (for discussions) could be
      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

      This then allows enhancement of the specified classes, or the classes defined by the specified JDO files, or the classes defined by the specified persistence-unit.

      What other control would people like to see ?

      1. jdoenhancer-5.patch
        12 kB
        Andy Jefferson

        Issue Links

          Activity

          Hide
          Andy Jefferson added a comment -

          Should have added that "classpath" should include
          jdo2-api.jar
          classes to enhance
          JDO metadata files
          enhancer jar

          Show
          Andy Jefferson added a comment - Should have added that "classpath" should include jdo2-api.jar classes to enhance JDO metadata files enhancer jar
          Hide
          Erik Bengtson added a comment -

          The enhancer API should implement the javaagent java 5 interfaces to allow runtime enhancement

          Show
          Erik Bengtson added a comment - The enhancer API should implement the javaagent java 5 interfaces to allow runtime enhancement
          Hide
          Ilan Kirsh added a comment -

          -persistenceUnit is too long and error prone, should be -pu.

          IMO, class files or persistence unit should be sufficient, and jdo-files might be a non portable extension (especially when annotations can replace XML).

          On the other hand, supporting enhancement of a JAR file could be useful.

          +1 for java agent support

          Show
          Ilan Kirsh added a comment - -persistenceUnit is too long and error prone, should be -pu. IMO, class files or persistence unit should be sufficient, and jdo-files might be a non portable extension (especially when annotations can replace XML). On the other hand, supporting enhancement of a JAR file could be useful. +1 for java agent support
          Hide
          Andy Jefferson added a comment -

          Below is an outline API interface :-

          package javax.jdo.enhancer;

          import java.lang.instrument.ClassFileTransformer;

          public interface JDOEnhancer extends ClassFileTransformer

          { void setVerbose(boolean arg); void setOutputDirectory(String dirName); void enhancePersistenceUnit(String persistenceUnit); void enhanceClasses(String[] classNames); void enhanceJdoFiles(String[] jdoFileNames); void enhanceJar(String jarFileName); }

          also define a new Exception, something like JDOEnhanceException thrown for failures in enhancement.

          Comments :-
          1). The four things that it is important to be able to enhance from each have their own method. The type passed in to each could be made more flexible, e.g use of URI instead of file names. Same goes for the names of the methods - could just have setters for the different types of "input" data, and then a method enhance().
          2). I consider enhancement from "jdo-files" equal in importance to "class-files" since this is what the user generates and can specify multiple classes hence is easier to specify than a long list of classes.
          3). A "persistence-unit" can include jar-file(s), class-file(s), and mapping-file(s) so it could be argued that this would be the only option required, but then having the others makes sense for backwards compat.

          Show
          Andy Jefferson added a comment - Below is an outline API interface :- package javax.jdo.enhancer; import java.lang.instrument.ClassFileTransformer; public interface JDOEnhancer extends ClassFileTransformer { void setVerbose(boolean arg); void setOutputDirectory(String dirName); void enhancePersistenceUnit(String persistenceUnit); void enhanceClasses(String[] classNames); void enhanceJdoFiles(String[] jdoFileNames); void enhanceJar(String jarFileName); } also define a new Exception, something like JDOEnhanceException thrown for failures in enhancement. Comments :- 1). The four things that it is important to be able to enhance from each have their own method. The type passed in to each could be made more flexible, e.g use of URI instead of file names. Same goes for the names of the methods - could just have setters for the different types of "input" data, and then a method enhance(). 2). I consider enhancement from "jdo-files" equal in importance to "class-files" since this is what the user generates and can specify multiple classes hence is easier to specify than a long list of classes. 3). A "persistence-unit" can include jar-file(s), class-file(s), and mapping-file(s) so it could be argued that this would be the only option required, but then having the others makes sense for backwards compat.
          Hide
          Craig L Russell added a comment -

          Moving this to 2.3; awaiting a volunteer to take it.

          Show
          Craig L Russell added a comment - Moving this to 2.3; awaiting a volunteer to take it.
          Hide
          Richard Schilling added a comment -

          This is certainly needed for enhancers.

          I might add some methods to the API presented to interrogate a class after enhancement to answer the following questions:

          • what enhancer processed the class?
          • what methods/members were added to the class by the enhancer?
          • how many bytes were added to the class when it was enhanced?
          • what version of JDO does the enhanced class conform to?

          It might also be convenient to give the enhancer the ability to do on-demand enhancing of a single class. And, to specify a debugging level. Thoughts?

          I've added to the suggested API for discussion...

          package javax.jdo.enhancer;

          import java.lang.instrument.ClassFileTransformer;

          public interface JDOEnhancer extends ClassFileTransformer

          { public long bytesAdded(String enhancedClassName); // how many bytes were added to the class public String[] methodsAdded(String enhancedClassName); // list of method signatures added public static String enhancerVendor(); // whose enhancer is this? public String enhancedBy(String enhancedClassName); // who enhanced this class? public String[] membersAdded(String enhancedClassName); // list of member signautures added to the class public long enhanceDate(String enhancedClassName); // when was the class enhanced (ms since epoch) public Class enhance(String className, File jdoConfigFile); // enhance a single class given a config public Class enhance(String className); // enhance using config file found in default locations public void setDebugLevel(enum level); // set the debugging level to an enumaration/constant value void setVerbose(boolean arg); void setOutputDirectory(String dirName); void enhancePersistenceUnit(String persistenceUnit); void enhanceClasses(String[] classNames); void enhanceJdoFiles(String[] jdoFileNames); void enhanceJar(String jarFileName); }

          Would it also be useful to add some methods so that the enhancer reports what it plans on doing to a class that has not been processed yet? I'm not suggesting it, but I'm curious to know what people think. Something like:

          public String generatePlan(String className);

          Show
          Richard Schilling added a comment - This is certainly needed for enhancers. I might add some methods to the API presented to interrogate a class after enhancement to answer the following questions: what enhancer processed the class? what methods/members were added to the class by the enhancer? how many bytes were added to the class when it was enhanced? what version of JDO does the enhanced class conform to? It might also be convenient to give the enhancer the ability to do on-demand enhancing of a single class. And, to specify a debugging level. Thoughts? I've added to the suggested API for discussion... package javax.jdo.enhancer; import java.lang.instrument.ClassFileTransformer; public interface JDOEnhancer extends ClassFileTransformer { public long bytesAdded(String enhancedClassName); // how many bytes were added to the class public String[] methodsAdded(String enhancedClassName); // list of method signatures added public static String enhancerVendor(); // whose enhancer is this? public String enhancedBy(String enhancedClassName); // who enhanced this class? public String[] membersAdded(String enhancedClassName); // list of member signautures added to the class public long enhanceDate(String enhancedClassName); // when was the class enhanced (ms since epoch) public Class enhance(String className, File jdoConfigFile); // enhance a single class given a config public Class enhance(String className); // enhance using config file found in default locations public void setDebugLevel(enum level); // set the debugging level to an enumaration/constant value void setVerbose(boolean arg); void setOutputDirectory(String dirName); void enhancePersistenceUnit(String persistenceUnit); void enhanceClasses(String[] classNames); void enhanceJdoFiles(String[] jdoFileNames); void enhanceJar(String jarFileName); } Would it also be useful to add some methods so that the enhancer reports what it plans on doing to a class that has not been processed yet? I'm not suggesting it, but I'm curious to know what people think. Something like: public String generatePlan(String className);
          Hide
          Richard Schilling added a comment -

          The debugging level suggestion, BTW, is so the enhancer can include debugging code in the enhanced class if necessary, as opposed to showing debug output of the enhancer activity.

          Another question, too....

          Would it be useful or too complicated to programmatically set/get the JDO configuration? As, in

          public boolean setConfig(String jdoConfigString);
          public boolean setConfig(File jdoConfigFile);
          public String getConfig();

          Show
          Richard Schilling added a comment - The debugging level suggestion, BTW, is so the enhancer can include debugging code in the enhanced class if necessary, as opposed to showing debug output of the enhancer activity. Another question, too.... Would it be useful or too complicated to programmatically set/get the JDO configuration? As, in public boolean setConfig(String jdoConfigString); public boolean setConfig(File jdoConfigFile); public String getConfig();
          Hide
          Andy Jefferson added a comment -

          Hi Richard,
          There is no way of knowing the "enhance date".
          Things like "methodsAdded", "bytesAdded" etc should only ever be returnable from enhanceXXX() rather than as a separate method; it is the enhance process that would define these.
          Presumably "enhanceVendor" is to return the vendor name of the enhancer being used?

          I don't see the need for
          public Class enhance(String className);
          since
          void enhanceClasses(String... classNames);
          provides that.

          Similarly the signature "enhance(String className, File jdoConfigFile);" since the class name implies the location of the config file (JDO compliant locations in the CLASSPATH).

          What is "JDO configuration" ? Why would you set it ?

          Show
          Andy Jefferson added a comment - Hi Richard, There is no way of knowing the "enhance date". Things like "methodsAdded", "bytesAdded" etc should only ever be returnable from enhanceXXX() rather than as a separate method; it is the enhance process that would define these. Presumably "enhanceVendor" is to return the vendor name of the enhancer being used? I don't see the need for public Class enhance(String className); since void enhanceClasses(String... classNames); provides that. Similarly the signature "enhance(String className, File jdoConfigFile);" since the class name implies the location of the config file (JDO compliant locations in the CLASSPATH). What is "JDO configuration" ? Why would you set it ?
          Hide
          Andy Jefferson added a comment -

          Patch for api2 project to add JDOEnhancer and JDOEnhanceException.
          Relative to the original proposal I've added getProperties() to match what the PMF has and can return "VendorName", "VersionNumber" (so catering for one of Richards suggestions).

          Show
          Andy Jefferson added a comment - Patch for api2 project to add JDOEnhancer and JDOEnhanceException. Relative to the original proposal I've added getProperties() to match what the PMF has and can return "VendorName", "VersionNumber" (so catering for one of Richards suggestions).
          Hide
          Michael Bouschen added a comment -

          The patch looks good. Just a couple of questions/remarks:

          • Does the enhancer replace the class files in place? Or does tis depend on whether setOutputDirectory has been called? I would be nice to have both options: replace the class file with th enhanced class file or write the enhanced class file to a different place.
          • Method enhanceClasses should take a ClassLoader in addition to the array of classNames. The method should use the context class loader if null is passed.
          • The arguments passed to methods enhanceJdoFiles and enhanceJar specify files names. I assume the files names may be absolute or relativ and in the latter case they are taken relative to the current working directory, correct? Would it make sense to add an option to find the file as a resource using the classpath (e.g. getResourceAsStream)? Then we need to specify a class loader used to find the resources.

          Michael

          Show
          Michael Bouschen added a comment - The patch looks good. Just a couple of questions/remarks: Does the enhancer replace the class files in place? Or does tis depend on whether setOutputDirectory has been called? I would be nice to have both options: replace the class file with th enhanced class file or write the enhanced class file to a different place. Method enhanceClasses should take a ClassLoader in addition to the array of classNames. The method should use the context class loader if null is passed. The arguments passed to methods enhanceJdoFiles and enhanceJar specify files names. I assume the files names may be absolute or relativ and in the latter case they are taken relative to the current working directory, correct? Would it make sense to add an option to find the file as a resource using the classpath (e.g. getResourceAsStream)? Then we need to specify a class loader used to find the resources. Michael
          Hide
          Andy Jefferson added a comment -

          Re: replace class files
          Yes, thats the obvious default to have for "target", and then if the user specifies a target they get written there.

          Also need to provide JDOHelper.getJDOEnhancer() which will search for a services file for an enhancer and return an instance.

          Show
          Andy Jefferson added a comment - Re: replace class files Yes, thats the obvious default to have for "target", and then if the user specifies a target they get written there. Also need to provide JDOHelper.getJDOEnhancer() which will search for a services file for an enhancer and return an instance.
          Hide
          Ilan Kirsh added a comment -

          Some ideas:

          (1)
          void enhanceClasses(String... classNames)
          (String... instead of String[])
          And also support import style wildcard, e.g. "my.pc.*" for all the classes in package my.pc.

          (2)
          void enhanceFiles(String... fileNames) -

          • one common method for all file types (class / directory / JDO / JAR)
          • support wildcards
          • Directory will represent all the class / JDO / JAR in that directory and subdirectories.

          (3)
          setClassLoader(...) instead of additional parameter (as setOutputDirectory). Also can have chaining methods: addPersistenceUnit(...) + addClasses(...) + addFiles(...) + enhance() method with no args.

          Show
          Ilan Kirsh added a comment - Some ideas: (1) void enhanceClasses(String... classNames) (String... instead of String[]) And also support import style wildcard, e.g. "my.pc.*" for all the classes in package my.pc. (2) void enhanceFiles(String... fileNames) - one common method for all file types (class / directory / JDO / JAR) support wildcards Directory will represent all the class / JDO / JAR in that directory and subdirectories. (3) setClassLoader(...) instead of additional parameter (as setOutputDirectory). Also can have chaining methods: addPersistenceUnit(...) + addClasses(...) + addFiles(...) + enhance() method with no args.
          Hide
          Andy Jefferson added a comment -

          Agree with setClassLoader(), use of varargs, and chaining methods (so all setters return the JDOEnhancer) from Ilan.

          Another usecase that is required is the dynamic construction of classes in memory (in a custom class loader). These then need enhancing. The user typically has the "byte[]" of the class and so we need something pertaining to

          byte[] enhanceClass(byte[])

          taking in the original bytecode and returning the modified bytecode. In this case it wouldn't be expected to save to disk the enhanced class.

          Show
          Andy Jefferson added a comment - Agree with setClassLoader(), use of varargs, and chaining methods (so all setters return the JDOEnhancer) from Ilan. Another usecase that is required is the dynamic construction of classes in memory (in a custom class loader). These then need enhancing. The user typically has the "byte[]" of the class and so we need something pertaining to byte[] enhanceClass(byte[]) taking in the original bytecode and returning the modified bytecode. In this case it wouldn't be expected to save to disk the enhanced class.
          Hide
          Andy Jefferson added a comment -

          Updated patch with varargs, chained setter capability, setClassLoader, enhance of byte[] in-memory class

          Show
          Andy Jefferson added a comment - Updated patch with varargs, chained setter capability, setClassLoader, enhance of byte[] in-memory class
          Hide
          Andy Jefferson added a comment -

          The only problem with Ilan's chaining of class addition is that the requirement to enhance a class in-memory (supplied via bytes) would then not have a way of returning the updated bytes. So what we could do is have
          addPersistenceUnit(...);
          addClasses(...);
          addFiles(...);
          enhance();

          and
          byte[] enhanceClass(String className, byte[] bytes);

          as distinct operations.

          Also propose a way of validating the enhancement state of classes, to work off the classes defined by the same add chaining mechanism above, then :-
          validate();

          Validation errors should be thrown as a nested set of JDOEnhanceException, one per class with errors.

          The enhanceXXX()/validate() method(s) don't currently return any feedback on the number of classes enhanced. Change the return type to int to be the number of classes enhanced ?

          Show
          Andy Jefferson added a comment - The only problem with Ilan's chaining of class addition is that the requirement to enhance a class in-memory (supplied via bytes) would then not have a way of returning the updated bytes. So what we could do is have addPersistenceUnit(...); addClasses(...); addFiles(...); enhance(); and byte[] enhanceClass(String className, byte[] bytes); as distinct operations. Also propose a way of validating the enhancement state of classes, to work off the classes defined by the same add chaining mechanism above, then :- validate(); Validation errors should be thrown as a nested set of JDOEnhanceException, one per class with errors. The enhanceXXX()/validate() method(s) don't currently return any feedback on the number of classes enhanced. Change the return type to int to be the number of classes enhanced ?
          Hide
          Andy Jefferson added a comment -

          Updated patch adding JDOHelper.getEnhancer() method taking in class loader to load the enhancer from; searches for "META-INF/services/javax.jdo.JDOEnhancer" file in CLASSPATH.

          Show
          Andy Jefferson added a comment - Updated patch adding JDOHelper.getEnhancer() method taking in class loader to load the enhancer from; searches for "META-INF/services/javax.jdo.JDOEnhancer" file in CLASSPATH.
          Hide
          Andy Jefferson added a comment -

          Further updated to include Ilan's chaining of addXXX methods. Now has

          JDOEnhancer addClass(String className, byte[] bytes);
          JDOEnhancer addClasses(String... classNames);
          JDOEnhancer addFiles(String... metadataFiles);
          JDOEnhancer addPersistenceUnit(String unitName);
          JDOEnhancer addJar(String jarName);

          void enhance();

          byte[] getEnhancedBytes(String className);

          so if a user has an in-memory class then they can use addClass(...) and after enhance call getEnhancedBytes(...)

          Show
          Andy Jefferson added a comment - Further updated to include Ilan's chaining of addXXX methods. Now has JDOEnhancer addClass(String className, byte[] bytes); JDOEnhancer addClasses(String... classNames); JDOEnhancer addFiles(String... metadataFiles); JDOEnhancer addPersistenceUnit(String unitName); JDOEnhancer addJar(String jarName); void enhance(); byte[] getEnhancedBytes(String className); so if a user has an in-memory class then they can use addClass(...) and after enhance call getEnhancedBytes(...)
          Hide
          Craig L Russell added a comment -

          Nice work. Regarding the patch:

          1.
          +EXC_GetEnhancerNoValidEnhancerAvailable=\
          +There were no valid JDOEnhancer implementations identified in the CLASSPATH.

          Could add a little color:
          +EXC_GetEnhancerNoValidEnhancerAvailable=\
          +There were no valid JDOEnhancer implementations identified in the CLASSPATH. \
          + The file META-INF/services/javax.jdo.JDOEnhancer should name the implementation class.

          2. Copy/paste error in JDOEnhanceException:
          + * Constructs a new <code>JDOReadOnlyException</code> with the
          + * Constructs a new <code>JDOReadOnlyException</code> with the
          + * Constructs a new <code>JDOReadOnlyException</code> with the
          \ No newline at end of file

          3. Javadoc is incomplete, missing @return tags for many methods.

          4. Suggest adding some javadoc to setOutputDirectory
          + * Mutator to set the location where enhanced classes are written.
          + * @param dirName Name of the directory

          + * Mutator to set the location where enhanced classes are written.
          + * If this method is not called, classes will be enhanced in place,
          + * overwriting the existing classes. If overwriting classes in a jar file,
          + * the existing files in the jar file will be written unchanged except
          + * for the enhanced classes.
          + * @param dirName Name of the directory

          5. For addClass methods, the format of the class names should be specified (e.g. binary names http://java.sun.com/javase/6/docs/api/java/lang/ClassLoader.html#name use $ as the delimiter for inner class/interface names).

          6. For the method
          + protected static JDOEnhancer getEnhancer(ClassLoader loader) {

          Shouldn't this method be public? It's a part of the public "interface".

          7. Also, the use of null to mean "the context class loader" is non-standard in the JDOHelper pattern for class loaders. It would be better to use a different method to mean "use the context class loader, and use the existing method to get the context class loader in a doPrivileged block, e.g.
          + protected static JDOEnhancer getEnhancer()

          { + return getEnhancer(getContextClassLoader()); + }

          8. The method loadClass is protected. You should probably use the existing JDOHelper method private static Class forName( for this purpose.

          9. This comment seems left over from a copy/paste:
          // remember exceptions from failed pmf invocations

          10. This comment is misleading. If there is no implementation, there shouldn't be a file named META-INF/services/javax.jdo.JDOEnhancer.
          + * The contents of the file is a string that is the enhancer class name,
          + * null or blank.

          Suggest:
          + * The contents of the file is a string that is the fully qualified enhancer class name.

          Show
          Craig L Russell added a comment - Nice work. Regarding the patch: 1. +EXC_GetEnhancerNoValidEnhancerAvailable=\ +There were no valid JDOEnhancer implementations identified in the CLASSPATH. Could add a little color: +EXC_GetEnhancerNoValidEnhancerAvailable=\ +There were no valid JDOEnhancer implementations identified in the CLASSPATH. \ + The file META-INF/services/javax.jdo.JDOEnhancer should name the implementation class. 2. Copy/paste error in JDOEnhanceException: + * Constructs a new <code>JDOReadOnlyException</code> with the + * Constructs a new <code>JDOReadOnlyException</code> with the + * Constructs a new <code>JDOReadOnlyException</code> with the \ No newline at end of file 3. Javadoc is incomplete, missing @return tags for many methods. 4. Suggest adding some javadoc to setOutputDirectory + * Mutator to set the location where enhanced classes are written. + * @param dirName Name of the directory + * Mutator to set the location where enhanced classes are written. + * If this method is not called, classes will be enhanced in place, + * overwriting the existing classes. If overwriting classes in a jar file, + * the existing files in the jar file will be written unchanged except + * for the enhanced classes. + * @param dirName Name of the directory 5. For addClass methods, the format of the class names should be specified (e.g. binary names http://java.sun.com/javase/6/docs/api/java/lang/ClassLoader.html#name use $ as the delimiter for inner class/interface names). 6. For the method + protected static JDOEnhancer getEnhancer(ClassLoader loader) { Shouldn't this method be public? It's a part of the public "interface". 7. Also, the use of null to mean "the context class loader" is non-standard in the JDOHelper pattern for class loaders. It would be better to use a different method to mean "use the context class loader, and use the existing method to get the context class loader in a doPrivileged block, e.g. + protected static JDOEnhancer getEnhancer() { + return getEnhancer(getContextClassLoader()); + } 8. The method loadClass is protected. You should probably use the existing JDOHelper method private static Class forName( for this purpose. 9. This comment seems left over from a copy/paste: // remember exceptions from failed pmf invocations 10. This comment is misleading. If there is no implementation, there shouldn't be a file named META-INF/services/javax.jdo.JDOEnhancer. + * The contents of the file is a string that is the enhancer class name, + * null or blank. Suggest: + * The contents of the file is a string that is the fully qualified enhancer class name.
          Hide
          Andy Jefferson added a comment -

          Thx for reviewing the patch. Attached is updated patch with all comments included. Also added validate() method to JDOEnhancer interface.

          Note that will need a way of registering MetaData with the enhancer, but that can be defined as part of JDO-615

          Show
          Andy Jefferson added a comment - Thx for reviewing the patch. Attached is updated patch with all comments included. Also added validate() method to JDOEnhancer interface. Note that will need a way of registering MetaData with the enhancer, but that can be defined as part of JDO-615
          Hide
          Andy Jefferson added a comment -

          Patch applied to SVN trunk

          Show
          Andy Jefferson added a comment - Patch applied to SVN trunk
          Hide
          Andy Jefferson added a comment -

          DataNucleus SVN updated to provide implementation of JDOEnhancer. Simple invocation of JDOHelper.getEnhancer() finds the services file, and creates an instance. Calling addClass, addFiles, addClasses, addPersistenceUnit all seem to work

          Need unit tests adding to TCK presumably - but not all implementations will provide their own enhancer so where do these go?

          Follow on items :-

          • Maven2 plugin that looks for the services file, creates the available JDOEnhancer and allows persistence unit, file-based enhancement. This could be done in a separate JIRA
          Show
          Andy Jefferson added a comment - DataNucleus SVN updated to provide implementation of JDOEnhancer. Simple invocation of JDOHelper.getEnhancer() finds the services file, and creates an instance. Calling addClass, addFiles, addClasses, addPersistenceUnit all seem to work Need unit tests adding to TCK presumably - but not all implementations will provide their own enhancer so where do these go? Follow on items :- Maven2 plugin that looks for the services file, creates the available JDOEnhancer and allows persistence unit, file-based enhancement. This could be done in a separate JIRA
          Hide
          Andy Jefferson added a comment -

          In conjunction with JDO-621 we need to decide if the current API is final. I added
          addClass(String className, byte[] bytes); // for generated class
          addClasses(String... classes); // For classes (in classpath, or files)
          addFiles(String filenames); // For mapping files
          addJar(String jarFilename); // For jar file

          so we mirror more-or-less the same entries in persistence.xml. Maybe the addFiles() should be called addMappingFiles()
          to use the persistence.xml name

          The other thing is if we allow specification of some directory, and subdirectories, where does that come in. The API needs to be clear, and adding this subdirectories control would have no bearing on some of the above options. Could add

          addDirectory(String dirName, boolean subdirs);

          Show
          Andy Jefferson added a comment - In conjunction with JDO-621 we need to decide if the current API is final. I added addClass(String className, byte[] bytes); // for generated class addClasses(String... classes); // For classes (in classpath, or files) addFiles(String filenames); // For mapping files addJar(String jarFilename); // For jar file so we mirror more-or-less the same entries in persistence.xml. Maybe the addFiles() should be called addMappingFiles() to use the persistence.xml name The other thing is if we allow specification of some directory, and subdirectories, where does that come in. The API needs to be clear, and adding this subdirectories control would have no bearing on some of the above options. Could add addDirectory(String dirName, boolean subdirs);
          Hide
          Ilan Kirsh added a comment -

          I think that subdirectories should be added as a global flag, not attached to particular directory - to match command line enhancement.

          Also, maybe one add(String... args) method should replace addClasses, addFiles, addJar and addDirectory.
          This may help implementing the command line processing without accessing and checking files.

          Show
          Ilan Kirsh added a comment - I think that subdirectories should be added as a global flag, not attached to particular directory - to match command line enhancement. Also, maybe one add(String... args) method should replace addClasses, addFiles, addJar and addDirectory. This may help implementing the command line processing without accessing and checking files.
          Hide
          Andy Jefferson added a comment -

          What bearing does "subdirectories" have on the input from addPersistenceUnit(), or addClass() ? nothing IMHO, hence why I suggest it is separate. A persistence-unit is not a file, neither is what is input into addClass().
          Classes and mappingfiles can be files (though the "classes" may not be, instead could be some resource in the classpath) so potentially could be impacted by some subdirectories flag - one way would be :-
          addPersistenceUnit(String);
          addClass(String, byte[]);
          addMappingFiles(boolean subdirs, String... filenames);
          addClassFiles(boolean subdirs, String filenames);
          addClasses(String... classnames);
          or you merge the addMappingFiles/addClassFiles into addFiles().

          WRT "add(String[] ...)" how does the impl know if it is a persistence-unit, or a mapping file, or a class file, or a wildcard, or a directory name, or some random junk ? the impl has to spend its time traversing "persistence.xml", and directories and files, and open them and see whats inside? No I don't like that, sorry.

          Presumably there needs to be
          addExtension(String option);
          to comply with what is discussed in JDO-621.

          Show
          Andy Jefferson added a comment - What bearing does "subdirectories" have on the input from addPersistenceUnit(), or addClass() ? nothing IMHO, hence why I suggest it is separate. A persistence-unit is not a file, neither is what is input into addClass(). Classes and mappingfiles can be files (though the "classes" may not be, instead could be some resource in the classpath) so potentially could be impacted by some subdirectories flag - one way would be :- addPersistenceUnit(String); addClass(String, byte[]); addMappingFiles(boolean subdirs, String... filenames); addClassFiles(boolean subdirs, String filenames); addClasses(String... classnames); or you merge the addMappingFiles/addClassFiles into addFiles(). WRT "add(String[] ...)" how does the impl know if it is a persistence-unit, or a mapping file, or a class file, or a wildcard, or a directory name, or some random junk ? the impl has to spend its time traversing "persistence.xml", and directories and files, and open them and see whats inside? No I don't like that, sorry. Presumably there needs to be addExtension(String option); to comply with what is discussed in JDO-621 .
          Hide
          Ilan Kirsh added a comment -

          > how does the impl know if it is a persistence-unit

          I agree that persistence unit is different, and I do not suggest merging it with anything.
          I wrote: "one add(String... args) method should replace addClasses, addFiles, addJar and addDirectory".
          addPersistenceUnit(String) is probably required anyway.

          > how does the impl know if it is ... or a mapping file, or a class file, or a wildcard, or a directory name, or some random junk ?

          Files should be identified anyway for command line enhancement. It is very easy to check if a file exists and if it is a directory. Jar and class files can be identified by their suffix. If it is not a file, it is either a class or junk.

          Actually ObjectDB works this way, and there is no real impact on performance (also, usually there are only few arguments to check, such as .class or package.). This is negligible, relative for instance to searching package.jdo files everywhere, and writing all the result class files.

          > What bearing does "subdirectories" have on the input from addPersistenceUnit(), or addClass() ?

          Sure, but the same is true for the command line, and still -s is a global option, doesn't it?
          I just wanted to synchronize JDO-591 with JDO-621.

          Show
          Ilan Kirsh added a comment - > how does the impl know if it is a persistence-unit I agree that persistence unit is different, and I do not suggest merging it with anything. I wrote: "one add(String... args) method should replace addClasses, addFiles, addJar and addDirectory". addPersistenceUnit(String) is probably required anyway. > how does the impl know if it is ... or a mapping file, or a class file, or a wildcard, or a directory name, or some random junk ? Files should be identified anyway for command line enhancement. It is very easy to check if a file exists and if it is a directory. Jar and class files can be identified by their suffix. If it is not a file, it is either a class or junk. Actually ObjectDB works this way, and there is no real impact on performance (also, usually there are only few arguments to check, such as .class or package. ). This is negligible, relative for instance to searching package.jdo files everywhere, and writing all the result class files. > What bearing does "subdirectories" have on the input from addPersistenceUnit(), or addClass() ? Sure, but the same is true for the command line, and still -s is a global option, doesn't it? I just wanted to synchronize JDO-591 with JDO-621 .
          Hide
          Craig L Russell added a comment -

          The corresponding javax.jdo.Enhancer patch in JDO-621 has been checked in. There is no more work for this JIRA.

          Show
          Craig L Russell added a comment - The corresponding javax.jdo.Enhancer patch in JDO-621 has been checked in. There is no more work for this JIRA.

            People

            • Assignee:
              Andy Jefferson
              Reporter:
              Andy Jefferson
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development