Re: JDOHelper patch

[Craig Russell]

Hi Erik, Thanks for the comments. I've updated the JDOHelperTest based on your comments:

jdohelpertest.patch
Description: Binary data

On Jul 16, 2005, at 9:43 AM, [EMAIL PROTECTED] wrote: Hi, There are two issues in JDOHelper.getPersistenceManagerFactory 1 - the JDOHelper.getPMF raises will raise an exception if there is no getPMF(Properties) or getPMF(Map) in the implementation. The exception for the properties method is not raised, but in its place the exception for the map method. I did consider this when I wrote the code, and decided that the Map exception was correct. The requirement in JDO 2.0 has changed from getPMF(Properties) to getPMF(Map), so a compliant implementation will recompile with Map as the formal parameter.  <spec> A11.1-32 [An implementation must provide a method to construct a PersistenceManagerFactory by a Map instance. This static method is called by the JDOHelper method getPersistenceManagerFactory (Map props). static PersistenceManagerFactory getPersistenceManagerFactory (Map props);] A11.1-33 [The properties consist of: “javax.jdo.PersistenceManagerFactoryClass”, whose value is the name of the implementation class; any JDO vendor-specific properties; and the following standard property names, which correspond to the properties as documented in this chapter: "javax.jdo.option.Optimistic" "javax.jdo.option.RetainValues" ... </spec> Since Properties is a Map, applications need not change. By the way, the parameter being a Map instead of Properties was an excellent suggestion by you, IIRC. The exception is only raised if no getPMF method is found, and since the requirement is for getPMF(Map) it seems like this is the right exception to report. I included the JDOHelper code to allow getPMF(Properties) for convenience of implementations until they have compiled with the new API20 library. I don't expect that the Properties code will need to be there for the final release. 2 - a try catch block is hidding JDOExceptions. Here is the patch: Index: C:/trunck/api20/test/java/javax/jdo/JDOHelperTest.java =================================================================== --- C:/trunck/api20/test/java/javax/jdo/JDOHelperTest.java    (revision 219338) +++ C:/trunck/api20/test/java/javax/jdo/JDOHelperTest.java    (working copy) @@ -18,14 +18,14 @@  import java.io.File;  import java.io.InputStream; - +import java.util.HashMap;  import java.util.Map;  import java.util.Properties;  import javax.jdo.pc.PCPoint; +import javax.jdo.spi.I18NHelper;  import javax.jdo.util.AbstractTest;  import javax.jdo.util.BatchTestRunner; -  import javax.naming.Context;  import javax.naming.InitialContext;  import javax.naming.NamingException; @@ -38,6 +38,9 @@   * TBD: getPMF for valid PMF class   */  public class JDOHelperTest extends AbstractTest { +    /** The Internationalization message helper. +     */ +    private final static I18NHelper msg = I18NHelper.getInstance ("javax.jdo.Bundle"); //NOI18N There is no specific error message in the specification, so it's not clear that we need to test for error message text.      /** */      public static void main(String args[]) { @@ -130,6 +133,7 @@          // TBD test JDOHelper.isDeleted(pc) for persistent instance      } +      /** Test null String resource with no class loader.       */      public void testGetPMFNullResource() { @@ -401,7 +405,21 @@          catch (JDOFatalInternalException ex) {              if (verbose)                  println("Caught expected exception " + ex); +            assertEquals(msg.msg("EXC_GetPMFNoSuchMethod"),ex.getMessage());          } Adding a new test method with a Map parameter is a good idea. But it should be a separate test case not added to testGetPMFNullResource. +        Map map = new HashMap(); +        map.put("javax.jdo.PersistenceManagerFactoryClass", +                "javax.jdo.JDOHelperTest$BadPMFNoGetPMFMethod"); +        try { +            pmf = JDOHelper.getPersistenceManagerFactory(map); +            fail("Bad PersistenceManagerFactoryClass should result in JDOFatalUserException "); +        } +        catch (JDOFatalInternalException ex) { +            if (verbose) +                println("Caught expected exception " + ex); +            assertEquals(msg.msg("EXC_GetPMFNoSuchMethod"),ex.getMessage()); +        } +      }      /** Test bad PMF class non-static getPMF method. Index: C:/trunck/api20/src/java/javax/jdo/JDOHelper.java =================================================================== --- C:/trunck/api20/src/java/javax/jdo/JDOHelper.java    (revision 219338) +++ C:/trunck/api20/src/java/javax/jdo/JDOHelper.java    (working copy) @@ -24,26 +24,21 @@  import java.io.File;  import java.io.FileInputStream;  import java.io.FileNotFoundException; -import java.io.InputStream;  import java.io.IOException; - -import java.lang.reflect.Method; +import java.io.InputStream;  import java.lang.reflect.InvocationTargetException; - +import java.lang.reflect.Method; +import java.security.AccessController; +import java.security.PrivilegedAction;  import java.util.Map;  import java.util.Properties; -import java.security.AccessController; -import java.security.PrivilegedAction; -  import javax.jdo.spi.I18NHelper;  import javax.jdo.spi.PersistenceCapable; -import javax.jdo.spi.StateManager; // for javadoc - +import javax.jdo.spi.StateManager;  import javax.naming.Context;  import javax.naming.InitialContext;  import javax.naming.NamingException; -  import javax.rmi.PortableRemoteObject; @@ -338,7 +333,7 @@                  } else if (propsMethod != null) {                      pmfMethod = propsMethod;                  } else { -                    throw mapGetMethodException; +                    throw propsGetMethodException; As discussed above, I don't believe this is a correct change.                  }              }              return (PersistenceManagerFactory) pmfMethod.invoke (null, new Object[] {props}); @@ -357,6 +352,8 @@              throw new JDOFatalInternalException (msg.msg("EXC_GetPMFNullPointerException", pmfClassName), e); //NOI18N          } catch (ClassCastException e) {              throw new JDOFatalInternalException (msg.msg("EXC_GetPMFClassCastException", pmfClassName), e); //NOI18N +        } catch (JDOException jdoex) { +            throw jdoex; This exception is not thrown by Method.invoke. It will be caught by the catch (InvocationTargetException). Reflective invocation wraps all exceptions in InvocationTargetException which we then unwrap and rethrow JDOExceptions. I've added a new test case in JDOHelperTest to test that a JDOException thrown from getPersistenceManagerFactory will be thrown to the user.          } catch (Exception e) {              throw new JDOFatalInternalException (msg.msg("EXC_GetPMFUnexpectedException"), e); //NOI18N          } Craig Russell Architect, Sun Java Enterprise System http://java.sun.com/products/jdo 408 276-5638 mailto:[EMAIL PROTECTED] P.S. A good JDO? O, Gasp!

smime.p7s
Description: S/MIME cryptographic signature