db-jdo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Craig Russell <Craig.Russ...@Sun.COM>
Subject Re: JDOHelper patch
Date Sat, 16 Jul 2005 21:13:41 GMT
Hi Erik,

Thanks for the comments.

I've updated the JDOHelperTest based on your comments:


On Jul 16, 2005, at 9:43 AM, erik@jpox.org 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:Craig.Russell@sun.com
P.S. A good JDO? O, Gasp!


Mime
View raw message