[ https://issues.apache.org/jira/brow...action_12543726 ]

Craig Russell commented on JDO-545:
-----------------------------------

1. In JDOHelper, method  getPersistenceManagerFactory(ClassLoader
) should @se
e  #getPersistenceManagerFactory(Map,String
,ClassLoader,ClassLoader)

2. This seems to have an extra "[":
+     * <BR><code>javax.jdo.option.InstanceLifecycleListener.{listenerC
lass}[=[{pcClasses}]</code>

3. The javadoc @since should be 2.0 for this changed method:
+     * @since 2.1
+     * @see  #getPersistenceManagerFactory(Map,String
,ClassLoader,ClassLoade
r)
+     */
+    public static PersistenceManagerFactory getPersistenceManagerFactory
+        (String name, ClassLoader resourceLoader, ClassLoader pmfLoader) &#
123;
+

4. The comment might be improved from
+        // JDO 2.1:  else name was null, empty string, or no resource found
 by
+        // given name; first assume that name represents name of named PMF

to
+        // JDO 2.1:  else name was null, empty string, or no resource found
 by
+        // given name; first see if that name represents name of named PMF

5. Shouldn't the name trimming go before the use in getResourceAsStream?
+        // as a convenience in public usage, convert null to anonymous PMF 
name
+        // or trim if not null or if blank
+        name = name == null
+                ?  ANONYMOUS_PERSISTENCE_MANAGER_FACTORY_NA
ME
+                : name.trim();

6. We need to open another JIRA to fix security issues. All calls to Class.g
etMethod and Method.invoke (among others) need to be invoked inside a doPriv
ileged block.

7. It looks like there are some references to PersistenceUnit in the api2-le
gacy project.

8. We need to add the method static PersistenceManagerFactory getPersistence
ManagerFactory(Map, String, ClassLoader, ClassLoader) to PersistenceManagerF
actory interface.


> Allow users to supply property-overriding Map instances when configuring f
rom resources (properties, jdoconfig.xml, or persistence.xml)
> --------------------------------------------------------------------------
-------------------------------------------------------------
>
>                 Key: JDO-545
>                 URL: https://issues.apache.org/jira/browse/JDO-545
>             Project: JDO
>          Issue Type: Improvement
>          Components: api2, api2-legacy
>    Affects Versions: JDO 2 maintenance release 1
>            Reporter: Matthew T. Adams
>            Assignee: Matthew T. Adams
>             Fix For: JDO 2 maintenance release 1
>
>         Attachments: JDO-545.patch, jdo-545.v01.patch
>
>
> See my changes inline below, enclosed by "<<<" and ">>>", and my comments.
> -matthew
> On Oct 1, 2007, at 2:07 PM, Craig L Russell wrote:
> Javadogs,
> I've completed the specification update for these methods. Please take a l
ook and comment:
> Note that this section doesn't exactly match what Matthew has already impl
emented, but really close...
> <proposed>
> PersistenceManagerFactory methods
> The methods in this section provide for bootstrapping the PersistenceManag
erFactory by configuration according to the properties documented in Section
 11.1. Users have a choice of configuration techniques:
> The application provides a Map of properties used to construct a Persisten
ceManagerFactory
> The application provides a Map of override properties and <<<the name of a resourc
e in standard Java Properties format, the name of a named PersistenceManagerFactory,
 or a JPA persistence unit name>>> that are used to construct a PersistenceManagerFa
cto
ry
> The application provides the name of a resource in standard Java Propertie
s format whose contents define the properties for the PersistenceManagerFact
ory
> The application provides an InputStream in standard Java Properties format
 whose contents define the properties for the PersistenceManagerFactory
> The application provides a File whose contents are in standard Java Proper
ties format which define the properties for the PersistenceManagerFactory
> The application provides a JNDI name and context in which the name is defi
ned
> The application provides a resource named META-INF/jdoconfig.xml and optio
nally META-INF/services/javax.jdo.PersistenceManagerFactory which contain co
nfiguration information
> The application provides a resource named META-INF/persistence.xml and opt
ionally META-INF/services/javax.persistence.EntityManagerFactory which conta
in configuration information
> For the cases of InputStream, File, and resource name, a Properties instance is co
nstructed by JDOHelper and passed to one of the getPersistenceManagerFactory(Map) me
thods. When using these techniques, each configuration of PersistenceManagerFactory 
is
contained <<<either in a Java Properties resource, in a META-INF/jdoconfig.x
ml and optionally a META-INF/services/javax.jdo.PersistenceManagerFactory, o
r in a META-INF/persistence.xml and optionally a META-INF/services/javax.per
sistence.EntityManagerFacto
ry.  The >>> propsLoader parameter is used only to load the resource to construct the Map. O
nce the Map with configuration information is obtained, the loader is used for loading all o
ther resour<<<c>>>es,
> Comment:  I'm confused about what the "propsLoader" and the "loader" are.  I'll as
sume the "propsLoader" is the ClassLoader used to load resources required to build t
he Properties instance (aka "Map"), and the "loader" is the ClassLoader used to load
 cl
asses and whatever other resources whose loading can't be controlled.
> including the persistence.xml, jdoconfig.xml, services locators, and Persi
stenceManagerFactory class.
> Comment:  This is not true; some of these resources are loaded by propsLoader, som
e are not.  If all native JDO means of creating a Properties instance (aka "Map") fa
il, we delegate to javax.persistence.Persistence, and we can't tell Persistence.crea
teE
ntityManagerFactory(..) which ClassLoader to use for resource loading and which to use for c
lass loading; in fact, we can't tell it anything about which ClassLoader(s) to use.
> In order to build the Properties instance (aka "Map"), the following resources are
 loaded by the propsLoader:  the Java Properties resource with the given name, and i
f that's not found, all META-INF/jdoconfig.xml files on the classpath.  If, after co
nst
ructing a Properties instance there is no javax.jdo.PersistenceManagerFactor
yClass property, then the propsLoader is used again to load a META-INF/servi
ces/javax.jdo.PersistenceManagerFactory; the exact one is determined by Clas
sLoader's getResourceAsStre
am(String) method.  If no META-INF/jdoconfig.xml files are found or no Persi
stenceManagerFactory is found with the requested name, then JDOHelper delega
tes to javax.persistence.Persistence.createEntityManagerFactory(String) to l
ook up the META-INF/persist
ence.xml file with the persistence
unit name given, then casts the returned EntityManagerFactory to a PersistenceManagerFactory
.
> A8.6-22 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(Map props, String name, ClassLoader loader);]
> A8.6-23 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(Map props, String name);]
> These methods create a new copy of the Map parameter, add the property "ja
vax.jdo.option.Name" with the value of the name parameter and delegate to th
e corresponding JDOHelper method taking a Map parameter.
> Comment:  This is new and not yet coded.  I understand the motivation to add these
 methods (which is Persistence. createEntityManagerFactory(String,Map)),
 so I'm cool 
with adding these, but we'll have to tighten up the description of the overriding Pr
ope
rties, especially in the case that no Java Properties resource is found and the named PMF is
 not found.  Do we pass the given Map down to Persistence.createEntityManagerFactory(String,
Map)?  I am inclined to say yes (see end comments plus the patch).
> A8.6-13 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(File propsFile);]
> A8.6-14 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(File propsFile, ClassLoader loader);]
> A8.6-17 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(InputStream stream);]
> A8.6-18 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(InputStream stream, ClassLoader loader);]
> These methods use the parameter(s) passed as arguments to construct a Prop
erties instance, and then delegate to the JDOHelper method getPersistenceMan
agerFactory that takes a Map parameter.
> A8.6-15 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(String propsResourceName);]
> A8.6-16 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(String propsResourceName, ClassLoader loader);]
> A8.6-21[public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(String propsResourceName, ClassLoader propsLoader,
> 			ClassLoader pmfLoader);]
> These methods use the propsLoader getResourceAsStream method with the propsResourc
eName as an argument to obtain an InputStream, construct a new Properties instance, 
then delegate to the corresponding JDOHelper method getPersistenceManagerFactory tha
t t
akes a Map parameter.
> Comment:  Correct.
> If the named resource does not exist, JDOHelper constructs a new Map, adds
 a property named "javax.jdo.option.Name" with the value of the propsResourc
eName parameter, and then delegates to the corresponding JDOHelper method ta
king a Map parameter.
> Comment:  Minor detail (see below).  JDOHelper conveniently converts a nul
l name to an empty string.
> The empty string (not a null string) identifies the default PersistenceMan
agerFactory name.
> Comment:  JDOHelper's gerPersistenceManagerFactory(String[,ClassLoader[,Cl
assLoader]]) methods treat the empty string and a null string equally.  Officially, 
the empty string is the name of the anonymous or unnamed PMF; null strings are just 
converted t
o empty strings for convenience.  We should probably add a constant ANONYMOU
S_PERSISTENCE_MANAGER_FACTOR_NAME (or UNNAMED_PERSISTENCE_MANAGER_FACTORY_NA
ME) String with the value "" to javax.jdo.Constants just so that it's explic
it in the code.  What do yo
u think?
> A8.6-2 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(Map props);]
> A8.6-3 [This method delegates to the corresponding method with a class
 loader parameter, using the calling thread's current contextClassLoader as 
the class loader.]
> A8.6-1 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(Map props, ClassLoader loader);]
> This method returns a PersistenceManagerFactory based on entries contained
 in the Map parameter. Three map entries are significant to the processing o
f this method:
> javax.jdo.option.PersistenceUnitName: If not null, this identifies the name of the
 persistence unit to be accessed. The Persistence.createEntityManagerFactory method 
is called with the props and PersistenceUnitName as parameters. The result is cast t
o P
ersistenceManagerFactory and returned to the user.
> javax.jdo.option.Name: If not null, this identifies the name of the PersistenceMan
agerFactory listed in jdoconfig. The class loader loads META-INF/jdoconfig.xml and f
inds the PersistenceManagerFactory by name. If the value of the entry javax.jdo.opti
on.
Name is the empty string, this identifies the unnamed PersistenceManagerFact
ory listed in jdoconfig. JDOHelper constructs a Map instance with the conten
ts of the jdoconfig entry and overrides its contents with the props paramete
r. Processing continues wit
h PersistenceManagerFactoryClass.
> javax.jdo.PersistenceManagerFactoryClass:
> If not null, this identifies the name of the PersistenceManagerFactory cla
ss. JDOHelper uses the loader to resolve the PersistenceManagerFactory class
 name.
> A8.6-6 [If the class named by the PersistenceManagerFactoryClass property cann
ot be found, or is not accessible to the user, then JDOFatalUserException is thrown.
] JDOHelper invokes the static method getPersistenceManagerFactory in the named clas
s, pass
ing the props as the parameter. A8.6-7 [If there is no public static imp
lementation of the getPersistenceManagerFactory(Map) method, ]A8.6-5 [or
 if there are any exceptions while trying to call the static method,] A8.6-8
 [or if the implementation of the s
tatic getPersistenceManagerFactory(Map) method throws an exception, then JDOFatalInternalExc
eption is thrown]. The nested exception indicates the root cause of the exception.
> Comment:  Correct so far.
> If null, JDOHelper loads the PersistenceManagerFactory class using the services lo
okup pattern. That is, resources named META-INF/services/<<<javax.jdo.>>>Persistence
ManagerFactory are loaded by the loader and each class in turn is used to call its s
tat
ic getPersistenceManagerFactory method, passing the props as the parameter. 
If one of the resources succeeds in returning a non-null PersistenceManagerF
actory, it is returned to the user and any exceptions (thrown by other resou
rces) are ignored. If no re
source succeeds, JDOFatalUserException exception is thrown to the user, with a nested except
ion for each failure.
> Comment:  The implementation only attempts to find one resource named META
-INF/services/javax.jdo/PersistenceManagerFactory (via ClassLoader's getReso
urceAsStream(String) method), not all of them.  Are you suggesting that we c
hange this?
> The standard key values for the properties are found in Section 11.1.
> Comment:  To implement support for user-supplied, property-overriding Maps
, I suggest that we make the existing methods
> getPersistenceManagerFactory(String name)
> getPersistenceManagerFactory(String name, ClassLoader resourceLoader)
> getPersistenceManagerFactory(String name, ClassLoader resourceLoader, Clas
sLoader pmfLoader)
> and the new methods
> getPersistenceManagerFactory(Map overrides, String name)
> getPersistenceManagerFactory(Map overrides, String name, ClassLoader resou
rceLoader)
> convenience methods that delegate to the meaty method
> getPersistenceManagerFactory(Map overrides, String name, ClassLoader resou
rceLoader, ClassLoader pmfLoader)
> If I understand all of this correctly, then the last operation of this met
hod before delegating to
> getPersistenceManagerFactory(Map props, ClassLoader resourceLoader, ClassL
oader pmfLoader)
> is to replace any values in props with values from overrides.  If this met
hod ends up delegating instead to Persistence.createEntityManagerFactory(Str
ing,Map), then it will pass the overriding properties as the Map in that met
hod.
> I implemented this while going through this post.  Please review the attac
hed patch to make sure that I got it right.
> Note that it is only in JDOHelper's getPersistenceManagerFactory(Map props, ClassL
oader resourceLoader, ClassLoader pmfLoader method that META-INF/services lookup is 
used, and then only when there is no javax.jdo.PersistenceManagerFactoryClass proper
ty
in props!
> JDO implementations are permitted to define key values of their own. A8.6-9 [A
ny key values not recognized by the implementation must be ignored.] A8.6-10 [Ke
y values that are recognized but not supported by an implementation must result in a
 JDOFatalUs
erException thrown by the method.]
> A8.6-11 [The returned PersistenceManagerFactory is not configurable (the setXX
X methods will throw an exception).] A8.6-12 [JDO implementations might manage a
 map of instantiated PersistenceManagerFactory instances based on specified property
 key values
, and return a previously instantiated PersistenceManagerFactory instance. In this case, the
 properties of the returned instance must exactly match the requested properties.]
> A8.6-19 [public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(String jndiLocation, Context context);]
> A8.6-20 [@deprecated public static
> 	PersistenceManagerFactory getPersistenceManagerFactory
> 		(String jndiLocation, Context context, ClassLoader loader);]
> These methods look up the PersistenceManagerFactory using the naming context and n
ame supplied. The implementation's factory method is not called. The behavior of thi
s method depends on the implementation of the context and its interaction with the s
ave
d PersistenceManagerFactory object. As with the other factory methods, the returned Persiste
nceManagerFactory is not configurable.
> </proposed>

--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

[ Post a follow-up to this message ]