OPENJPA-1742: move cfName logic to JDBCConfiguration, add tests for nonJTADataSource

git-svn-id: https://svn.apache.org/repos/asf/openjpa/branches/2.0.x@966602 13f79535-47bb-0310-9956-ffa450edef68

openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/conf/JDBCConfigurationImpl.java

@@ -55,6 +55,7 @@ import org.apache.openjpa.lib.jdbc.JDBCListener;
 import org.apache.openjpa.lib.log.Log;
 import org.apache.openjpa.lib.util.Localizer;
 import org.apache.openjpa.meta.MetaDataFactory;
+import org.apache.openjpa.util.UserException;
 
 /**
  * Default implementation of the {@link JDBCConfiguration} interface.
@@ -92,6 +93,8 @@ public class JDBCConfigurationImpl
     private DecoratingDataSource dataSource = null;
     private DecoratingDataSource dataSource2 = null;
     
+    private static final Localizer _loc = Localizer.forPackage(JDBCConfigurationImpl.class);
+
     /**
      * Default constructor. Attempts to load default properties.
      */
@@ -842,14 +845,69 @@ public class JDBCConfigurationImpl
     }
 
     public DataSource getDataSource(StoreContext ctx) {       
-        return getDataSource(ctx, (DataSource) getConnectionFactory());
+        Log log = getLog(LOG_RUNTIME);
+        DataSource ds = null;
+        
+        if(ctx != null && StringUtils.isNotEmpty(ctx.getConnectionFactoryName())) {
+            ds =  getDataSource(ctx, (DataSource) ctx.getConnectionFactory()); 
+            // fail fast if a cfName has been provided, but was not available in JNDI
+            if (ds == null) {
+                throw new UserException(_loc.get("invalid-datasource", ctx.getConnectionFactoryName())).setFatal(true);
+            }
+            if(! (ds instanceof DecoratingDataSource)) { 
+                ds = DataSourceFactory.decorateDataSource(ds, this, false);
+            }
+            if (log.isTraceEnabled()) {
+                log.trace("Found datasource1: " + ds + " from StoreContext using jndiName: "
+                    + ctx.getConnectionFactory2Name());
+            }
+            return ds; 
+        }
+        else {
+            ds = getDataSource(ctx, (DataSource) getConnectionFactory());
+            if (log.isTraceEnabled()) {
+                log.trace("Found datasource1: " + ds + " from configuration. StoreContext: " + ctx );
+            }
+            return ds; 
+        }
     }
 
     public DataSource getDataSource2(StoreContext ctx) {
-        // if there is no connection factory 2, use the primary factory
+        Log log = getLog(LOG_RUNTIME);
-        DataSource ds = (DataSource) getConnectionFactory2();
+        DataSource ds = null;
-        if (ds == null)
+
+        // Try to obtain from the StoreContext first.
+        if (ctx != null && StringUtils.isNotEmpty(ctx.getConnectionFactory2Name())) {
+            ds = (DataSource) ctx.getConnectionFactory2();
+            if (ds == null) {
+                // fail fast. If the non-jta-data-source is configured on the context we want an immediate error. 
+                throw new UserException(_loc.get("invalid-datasource", ctx.getConnectionFactory2Name())).setFatal(true);
+            }
+            if(! (ds instanceof DecoratingDataSource)) { 
+                ds = DataSourceFactory.decorateDataSource(ds, this, false);
+            }
+            if (log.isTraceEnabled()) {
+                log.trace("Found datasource2: " + ds + " from StoreContext using jndiName: "
+                    + ctx.getConnectionFactory2Name());
+            }
+            return ds;
+        }
+
+        // If not set on context or value from context is not available try cf2 from config
+        else{ 
+            ds = (DataSource) getConnectionFactory2();
+            if (log.isTraceEnabled()) {
+                log.trace("Found datasource 2: "+ ds + " from config. StoreContext: " + ctx);
+            }
+        }
+        
+        // fallback to cf1 / datasource1
+        if (ds == null) {
+            if(log.isTraceEnabled()) { 
+                log.trace("Trying datasource1");
+            }
             return getDataSource(ctx);
+        }
 
         // prefer the global connection 2 auth info if given
         String user = getConnection2UserName();

openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/kernel/JDBCStoreManager.java

@@ -170,34 +170,14 @@ public class JDBCStoreManager
         DataSource ds;
 
         if(useConnectionFactory2(ctx)) { 
-            ds = (DataSource) ctx.getConnectionFactory2();
-            if (ds != null) {
-                ds = DataSourceFactory.decorateDataSource(ds, _conf, false);
-            }
-            else {
             ds = _conf.getDataSource2(ctx); 
         }
-        } else {
-            ds = (DataSource) ctx.getConnectionFactory();
-            if (ds != null) {
-                ds = DataSourceFactory.decorateDataSource(ds, _conf, false);   
-            }
         else {
             ds = _conf.getDataSource(ctx);
         }
-        }
         return ds; 
     }
 
-    private boolean useContextToGetDataSource(StoreContext ctx) { 
-        // configuration check to enable goes here. 
-        if (StringUtils.isBlank(ctx.getConnectionFactoryName()) 
-                && StringUtils.isBlank(ctx.getConnectionFactory2Name())) {
-            return false;
-        }
-        return true;
-    }
-
     public JDBCConfiguration getConfiguration() {
         return _conf;
     }

openjpa-jdbc/src/main/resources/org/apache/openjpa/jdbc/conf/localizer.properties

@@ -246,3 +246,5 @@ map-factory: Using mapping factory "{0}".
 meta-factory: Using metadata factory "{0}".
 unknown-datasource: JNDI lookup for "{0}" returned "{1}", which is not a  \
 	javax.sql.DataSource object. 
+invalid-datasource: JNDI lookup for "{0}" specified on the StoreContext (\
+    EntityManager) returned null, the resulting EntityManager cannot be used. 

openjpa-kernel/src/main/java/org/apache/openjpa/kernel/AbstractBrokerFactory.java

@@ -31,6 +31,7 @@ import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.locks.ReentrantLock;
 
+import javax.naming.NamingException;
 import javax.transaction.Status;
 import javax.transaction.Synchronization;
 import javax.transaction.Transaction;
@@ -38,6 +39,7 @@ import javax.transaction.TransactionManager;
 
 import org.apache.commons.collections.set.MapBackedSet;
 import org.apache.commons.lang.StringUtils;
+import org.apache.commons.lang.exception.NestableRuntimeException;
 import org.apache.openjpa.conf.BrokerValue;
 import org.apache.openjpa.conf.MetaDataRepositoryValue;
 import org.apache.openjpa.conf.OpenJPAConfiguration;
@@ -190,6 +192,17 @@ public abstract class AbstractBrokerFactory
         String cf1Name, String cf2Name) {
         try {
             assertOpen();
+            
+            if(StringUtils.isNotEmpty(cf1Name)) {
+                // If the cfName has been set on the broker try looking up now.
+                try { 
+                    _conf.getConnectionFactory();
+                }
+                catch(UserException ue) { 
+                     // try setting the broker's CF into the configuration. 
+                    _conf.setConnectionFactoryName(cf1Name); 
+                }
+            }
             makeReadOnly();
 
             Broker broker = null;

openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/conf/Person.java

@@ -47,6 +47,12 @@ public class Person {
         setId(id);
     }
 
+    public Person(int id, String name) {
+        super();
+        setId(id);
+        setName(name);
+    }
+
     public int getId() {
         return id;
     }

openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/conf/TestOverrideNonJtaDataSource.java

@@ -0,0 +1,169 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.    
+ */
+package org.apache.openjpa.persistence.conf;
+
+import java.util.HashMap;
+import java.util.Map;
+
+import javax.persistence.EntityExistsException;
+import javax.persistence.EntityManager;
+import javax.persistence.EntityManagerFactory;
+import javax.persistence.RollbackException;
+
+import org.apache.openjpa.persistence.ArgumentException;
+import org.apache.openjpa.persistence.test.AbstractPersistenceTestCase;
+
+public class TestOverrideNonJtaDataSource extends AbstractPersistenceTestCase {
+    private String defaultJndiName = "jdbc/mocked";
+    private String[] jndiNames = { "jdbc/mocked1" };
+
+    protected void init(String cfName) {
+        EntityManagerFactory emf = getEmf("openjpa.ConnectionFactoryName", cfName, true);
+        EntityManager em = emf.createEntityManager();
+        em.getTransaction().begin();
+        em.createQuery("Delete from confPerson").executeUpdate();
+        em.getTransaction().commit();
+        em.close();
+        emf.close();
+    }
+
+    protected void setUp() {
+        // create an EMF for each database.
+        init(defaultJndiName);
+        init(jndiNames[0]);
+    }
+    
+    protected EntityManagerFactory getEmf(String cfPropertyName, String cfPropertyValue) {
+        return getEmf(cfPropertyName, cfPropertyValue, false);
+    }
+
+    protected EntityManagerFactory getEmf(String cfPropertyName, String cfPropertyValue, boolean syncMappings) {
+        // null out the driver to prevent system properties from taking effect.
+        if (syncMappings) {
+            return createEMF(
+                "openjpa.jdbc.SynchronizeMappings", "buildSchema",
+                "openjpa.ConnectionDriverName", "",
+                "openjpa.ConnectionFactoryMode", "managed",
+                "openjpa.ConnectionFactoryName", defaultJndiName,  // must have a cf1, to initialize configuration
+                cfPropertyName,cfPropertyValue, 
+                Person.class);
+        }
+        return createEMF(
+            "openjpa.ConnectionDriverName", "", 
+            "openjpa.ConnectionFactoryMode", "managed",
+            "openjpa.ConnectionFactoryName", defaultJndiName, // must have a cf1, to initialize configuration
+            cfPropertyName,cfPropertyValue, 
+            Person.class);
+    }
+
+    protected EntityManager getEm(EntityManagerFactory emf, String name, String value) {
+        Map<String, Object> props = new HashMap<String, Object>();
+        props.put(name, value);
+        return emf.createEntityManager(props);
+    }
+
+    public String getPersistenceUnitName() {
+        return "TestCfSwitching";
+    }
+
+    public void testConnectionFactoryName() {
+        // TODO Disable for non derby.
+        // split out so that we can try javax.persistence.jtaDataSource in the future.
+        overridePropertyOnEM("openjpa.ConnectionFactory2Name", jndiNames[0]);
+    }
+
+    public void testJtaDataSource() {
+        // TODO Disable for non derby.
+        // split out so that we can try javax.persistence.jtaDataSource in the future.
+        overridePropertyOnEM("javax.persistence.nonJtaDataSource", jndiNames[0]);
+    }
+
+    public void overridePropertyOnEM(String name, String value) {
+        // use the default JndiName for the base EntityManagerFactory
+        EntityManagerFactory emf = getEmf(name, defaultJndiName);
+        assertNotNull(emf);
+
+        EntityManager em = emf.createEntityManager();
+        assertNotNull(em);
+
+        EntityManager em1 = getEm(emf, name, value);
+        assertNotNull(em1);
+
+        // 'prove' that we're using a different database by inserting the same row
+        em.getTransaction().begin();
+        em.persist(new Person(1, "em"));
+        em.getTransaction().commit();
+
+        em1.getTransaction().begin();
+        em1.persist(new Person(1, "em1"));
+        em1.getTransaction().commit();
+
+        em.clear();
+        em1.clear();
+
+        Person p = em.find(Person.class, 1);
+        Person p1 = em1.find(Person.class, 1);
+        assertNotSame(p, p1);
+        assertEquals("em", p.getName());
+        assertEquals("em1", p1.getName());
+
+        em.clear();
+        em1.clear();
+
+        // make sure inserting the same row again fails.
+        em.getTransaction().begin();
+        em.persist(new Person(1));
+        try {
+            em.getTransaction().commit();
+            fail("Should not be able to commit the same row a second time");
+        } catch (RollbackException rbe) {
+            assertTrue(rbe.getCause() instanceof EntityExistsException);
+            // expected
+        }
+
+        em1.getTransaction().begin();
+        em1.persist(new Person(1));
+        try {
+            em1.getTransaction().commit();
+            fail("Should not be able to commit the same row a second time");
+        } catch (RollbackException rbe) {
+            assertTrue(rbe.getCause() instanceof EntityExistsException);
+            // expected
+        }
+        em.close();
+        em1.close();
+        emf.close();
+    }
+
+    public void testInvalidCfName() throws Exception {
+        // ensure EM creation fails - when provided an invalid JNDI name
+        EntityManagerFactory emf = null;
+        try {
+            emf = getEmf("openjpa.ConnectionFactory2Name", defaultJndiName);
+            getEm(emf, "openjpa.ConnectionFactory2Name", "jdbc/NotReal");
+            fail("Expected an excepton when creating an EM with a bogus JNDI name");
+        } catch (ArgumentException e) {
+            assertTrue(e.isFatal());
+            System.out.println(e);
+            assertTrue(e.getMessage().contains("jdbc/NotReal")); // ensure failing JNDI name is in the message
+            assertTrue(e.getMessage().contains("EntityManager")); // ensure where the JNDI name came from is in message
+        }
+    }
+}
+

openjpa-persistence-jdbc/src/test/java/org/apache/openjpa/persistence/conf/TestSwitchConnection.java

@@ -26,31 +26,45 @@ import javax.persistence.EntityManager;
 import javax.persistence.EntityManagerFactory;
 import javax.persistence.RollbackException;
 
+import org.apache.openjpa.persistence.ArgumentException;
 import org.apache.openjpa.persistence.test.AbstractPersistenceTestCase;
 
 public class TestSwitchConnection extends AbstractPersistenceTestCase {
     private String defaultJndiName = "jdbc/mocked";
     private String[] jndiNames = { "jdbc/mocked1" };
 
-    protected void initEMF(String cfName) { 
+    protected void init(String cfName) {
-        EntityManagerFactory emf = getEmf("openjpa.ConnectionFactoryName", cfName);
+        EntityManagerFactory emf = getEmf("openjpa.ConnectionFactoryName", cfName, true);
-        
         EntityManager em = emf.createEntityManager();
         em.getTransaction().begin();
         em.createQuery("Delete from confPerson").executeUpdate();
         em.getTransaction().commit();
         em.close();
-        
         emf.close();
     }
 
+    protected void setUp() {
+        // create an EMF for each database.
+        init(defaultJndiName);
+        init(jndiNames[0]);
+    }
+
     protected EntityManagerFactory getEmf(String cfPropertyName, String cfPropertyValue) {
+        return getEmf(cfPropertyName, cfPropertyValue, false);
+    }
+    
+    protected EntityManagerFactory getEmf(String cfPropertyName, String cfPropertyValue, boolean syncMappings) {
         // null out the driver to prevent system properties from taking effect.
         // do not set connectionFactoryModeManaged - or connectionFactory2 will be used.
+        if(syncMappings) { 
+            return createEMF( 
+                "openjpa.jdbc.SynchronizeMappings", "buildSchema",
+                "openjpa.ConnectionDriverName", "",
+                cfPropertyName,cfPropertyValue);
+        }
         return createEMF(
             "openjpa.ConnectionDriverName", "",
-            cfPropertyName, cfPropertyValue,
+            cfPropertyName,cfPropertyValue);
-            Person.class); 
     }
 
     protected EntityManager getEm(EntityManagerFactory emf, String name, String value) {
@@ -59,21 +73,23 @@ public class TestSwitchConnection extends AbstractPersistenceTestCase {
         return emf.createEntityManager(props);
     }
 
-    protected void createTables() { 
+    public String getPersistenceUnitName() {
-        // create an EMF for each database;
+        return "TestCfSwitching";
-        initEMF(defaultJndiName);
-        initEMF(jndiNames[0]);
     }
 
     public void testConnectionFactoryName() {
+        // TODO Disable for non derby.
         // split out so that we can try javax.persistence.jtaDataSource in the future.
         overridePropertyOnEM("openjpa.ConnectionFactoryName", jndiNames[0]);
     }
     
-    public void overridePropertyOnEM(String name, String value) {
+    public void testJtaDataSource() {
         // TODO Disable for non derby.
-        createTables();
+        // split out so that we can try javax.persistence.jtaDataSource in the future.
+        overridePropertyOnEM("javax.persistence.jtaDataSource", jndiNames[0]);
+    }
 
+    public void overridePropertyOnEM(String name, String value) {
         // use the default JndiName for the base EntityManagerFactory
         EntityManagerFactory emf = getEmf(name, defaultJndiName);
         assertNotNull(emf);
@@ -86,18 +102,26 @@ public class TestSwitchConnection extends AbstractPersistenceTestCase {
 
         // 'prove' that we're using a different database by inserting the same row
         em.getTransaction().begin();
-        em.persist(new Person(1));
+        em.persist(new Person(1, "em"));
         em.getTransaction().commit();
 
         em1.getTransaction().begin();
-        em1.persist(new Person(1));
+        em1.persist(new Person(1, "em1"));
         em1.getTransaction().commit();
 
         em.clear();
         em1.clear();
 
-        // sanity test, make sure inserting the same row again fails.
+        Person p = em.find(Person.class, 1);
+        Person p1 = em1.find(Person.class, 1);
+        assertNotSame(p, p1);
+        assertEquals("em", p.getName());
+        assertEquals("em1", p1.getName());
 
+        em.clear();
+        em1.clear();
+
+        // make sure inserting the same row again fails.
         em.getTransaction().begin();
         em.persist(new Person(1));
         try {
@@ -117,9 +141,22 @@ public class TestSwitchConnection extends AbstractPersistenceTestCase {
             assertTrue(rbe.getCause() instanceof EntityExistsException);
             // expected
         }
-        
         em.close();
         em1.close();
         emf.close();
     }
+
+    public void testInvalidCfName() throws Exception {
+        // ensure EM creation fails - when provided an invalid JNDI name
+        EntityManagerFactory emf = null;
+        try {
+            emf = getEmf("openjpa.ConnectionFactoryName", defaultJndiName); 
+            getEm(emf, "openjpa.ConnectionFactoryName", "jdbc/NotReal");
+            fail("Expected an excepton when creating an EM with a bogus JNDI name");
+        } catch (ArgumentException e) {
+            assertTrue(e.isFatal());
+            assertTrue(e.getMessage().contains("jdbc/NotReal")); // ensure failing JNDI name is in the message
+            assertTrue(e.getMessage().contains("EntityManager")); // ensure where the JNDI name came from is in message
+        }
+    }
 }

openjpa-persistence-jdbc/src/test/resources/META-INF/persistence.xml

@@ -365,4 +365,8 @@
         </properties>
     </persistence-unit>    
     
+    <persistence-unit name="TestCfSwitching">
+        <class>org.apache.openjpa.persistence.conf.Person</class>
+    </persistence-unit>
+
 </persistence>

openjpa-persistence/src/main/java/org/apache/openjpa/persistence/EntityManagerFactoryImpl.java

@@ -30,6 +30,7 @@ import javax.persistence.EntityManagerFactory;
 import javax.persistence.PersistenceUnitUtil;
 import javax.persistence.spi.LoadState;
 
+import org.apache.commons.lang.StringUtils;
 import org.apache.openjpa.conf.OpenJPAConfiguration;
 import org.apache.openjpa.kernel.AutoDetach;
 import org.apache.openjpa.kernel.Broker;
@@ -39,6 +40,7 @@ import org.apache.openjpa.kernel.DelegatingFetchConfiguration;
 import org.apache.openjpa.kernel.FetchConfiguration;
 import org.apache.openjpa.lib.conf.Configurations;
 import org.apache.openjpa.lib.conf.Value;
+import org.apache.openjpa.lib.log.Log;
 import org.apache.openjpa.lib.util.Closeable;
 import org.apache.openjpa.lib.util.Localizer;
 import org.apache.openjpa.persistence.criteria.CriteriaBuilderImpl;
@@ -165,6 +167,7 @@ public class EntityManagerFactoryImpl
             props = new HashMap(props);
 
         OpenJPAConfiguration conf = getConfiguration();
+        Log log = conf.getLog(OpenJPAConfiguration.LOG_RUNTIME);
         String user = (String) Configurations.removeProperty("ConnectionUserName", props);
         if (user == null)
             user = conf.getConnectionUserName();
@@ -210,6 +213,10 @@ public class EntityManagerFactoryImpl
             cf2Name = (String) Configurations.removeProperty("ConnectionFactory2Name", props);
         }
         
+        if (log != null && log.isTraceEnabled()) {
+            log.trace("Found ConnectionFactoryName from props: " + cfName);
+        }
+
         Broker broker = _factory.newBroker(user, pass, managed, retainMode, false, cfName, cf2Name);
             
         // add autodetach for close and rollback conditions to the configuration