NXP-19361: make sure that a CoreSession cannot be used without a tran…

…saction

nuxeo-core/nuxeo-core-api/src/main/java/org/nuxeo/ecm/core/api/CoreSession.java

@@ -160,7 +160,10 @@ public interface CoreSession extends AutoCloseable {
 
     /**
      * Returns {@code true} if all sessions in the current thread share the same state.
+     *
+     * @deprecated since 8.4 as it always returns true by design
      */
+    @Deprecated
     boolean isStateSharedByAllThreadSessions();
 
     /**

nuxeo-core/nuxeo-core-api/src/main/java/org/nuxeo/ecm/core/api/InstanceRef.java

@@ -22,6 +22,8 @@
 import java.io.ObjectStreamException;
 import java.security.Principal;
 
+import org.nuxeo.runtime.transaction.TransactionHelper;
+
 /**
  * Document repository reference including the principal owner of the session.
  *
@@ -61,6 +63,11 @@ public Object reference() {
     }
 
     private Object readResolve() throws ObjectStreamException {
+        // we need a transaction for this
+        boolean started = false;
+        if (!TransactionHelper.isTransactionActiveOrMarkedRollback()) {
+            started = TransactionHelper.startTransaction();
+        }
         try {
             try (CoreSession session = CoreInstance.openCoreSession(repositoryName, principal)) {
                 referent = session.getDocument(ref);
@@ -72,6 +79,10 @@ private Object readResolve() throws ObjectStreamException {
                     "Cannot refetch " + ref + " from " + repositoryName);
             error.initCause(cause);
             throw error;
+        } finally {
+            if (started) {
+                TransactionHelper.commitOrRollbackTransaction();
+            }
         }
     }
 

nuxeo-core/nuxeo-core-api/src/main/java/org/nuxeo/ecm/core/api/UnrestrictedSessionRunner.java

@@ -126,25 +126,13 @@ public void runUnrestricted() {
             }
             try {
                 CoreSession baseSession = session;
-                if (baseSession != null && !baseSession.isStateSharedByAllThreadSessions()) {
-                    // save base session state for unrestricted one
-                    baseSession.save();
-                }
                 session = CoreInstance.openCoreSession(repositoryName);
                 try {
                     run();
                 } finally {
                     try {
-                        if (!session.isStateSharedByAllThreadSessions()) {
-                            // save unrestricted state for base session
-                            session.save();
-                        }
                         session.close();
                     } finally {
-                        if (baseSession != null && !baseSession.isStateSharedByAllThreadSessions()) {
-                            // process invalidations from unrestricted session
-                            baseSession.save();
-                        }
                         session = baseSession;
                     }
                 }

nuxeo-core/nuxeo-core-storage-dbs/src/main/java/org/nuxeo/ecm/core/storage/dbs/DBSRepositoryBase.java

@@ -331,18 +331,16 @@ public Session getSession() {
         Transaction transaction;
         try {
             transaction = TransactionHelper.lookupTransactionManager().getTransaction();
-            if (transaction != null && transaction.getStatus() != Status.STATUS_ACTIVE) {
-                transaction = null;
+            if (transaction == null) {
+                throw new NuxeoException("Missing transaction");
+            }
+            int status = transaction.getStatus();
+            if (status != Status.STATUS_ACTIVE && status != Status.STATUS_MARKED_ROLLBACK) {
+                throw new NuxeoException("Transaction in invalid state: " + status);
             }
         } catch (SystemException | NamingException e) {
-            transaction = null;
-        }
-
-        if (transaction == null) {
-            // no active transaction, use a regular session
-            return newSession();
+            throw new NuxeoException("Failed to get transaction", e);
         }
-
         TransactionContext context = transactionContexts.get(transaction);
         if (context == null) {
             context = new TransactionContext(transaction, newSession());

nuxeo-core/nuxeo-core-storage-dbs/src/main/java/org/nuxeo/ecm/core/storage/dbs/DBSSession.java

@@ -218,11 +218,6 @@ public void rollback() {
         transaction.rollback();
     }
 
-    @Override
-    public boolean isStateSharedByAllThreadSessions() {
-        return false;
-    }
-
     protected BlobManager getBlobManager() {
         return repository.getBlobManager();
     }

nuxeo-core/nuxeo-core-storage-sql/nuxeo-core-storage-sql/src/main/java/org/nuxeo/ecm/core/storage/sql/Session.java

@@ -51,11 +51,6 @@ public interface Session extends Connection {
      */
     boolean isLive();
 
-    /**
-     * Returns {@code true} if all sessions in the current thread share the same state.
-     */
-    boolean isStateSharedByAllThreadSessions();
-
     /**
      * Gets the session repository name.
      *

nuxeo-core/nuxeo-core-storage-sql/nuxeo-core-storage-sql/src/main/java/org/nuxeo/ecm/core/storage/sql/SessionImpl.java

@@ -288,12 +288,6 @@ public boolean isLive() {
         return live;
     }
 
-    @Override
-    public boolean isStateSharedByAllThreadSessions() {
-        // only the JCA handle returns true
-        return false;
-    }
-
     @Override
     public String getRepositoryName() {
         return repository.getName();

nuxeo-core/nuxeo-core-storage-sql/nuxeo-core-storage-sql/src/main/java/org/nuxeo/ecm/core/storage/sql/coremodel/SQLSession.java

@@ -148,11 +148,6 @@ public boolean isLive() {
         return session != null && session.isLive();
     }
 
-    @Override
-    public boolean isStateSharedByAllThreadSessions() {
-        return session.isStateSharedByAllThreadSessions();
-    }
-
     @Override
     public String getRepositoryName() {
         return repository.getName();

nuxeo-core/nuxeo-core-storage-sql/nuxeo-core-storage-sql/src/main/java/org/nuxeo/ecm/core/storage/sql/jdbc/JDBCMapper.java

@@ -52,6 +52,7 @@
 import java.util.concurrent.Callable;
 
 import javax.sql.XADataSource;
+import javax.transaction.Transaction;
 import javax.transaction.xa.XAException;
 import javax.transaction.xa.XAResource;
 import javax.transaction.xa.Xid;
@@ -86,6 +87,7 @@
 import org.nuxeo.ecm.core.storage.sql.jdbc.dialect.DialectOracle;
 import org.nuxeo.ecm.core.storage.sql.jdbc.dialect.SQLStatement.ListCollector;
 import org.nuxeo.runtime.api.Framework;
+import org.nuxeo.runtime.transaction.TransactionHelper;
 
 /**
  * A {@link JDBCMapper} maps objects to and from a JDBC database. It is specific to a given database connection, as it
@@ -156,10 +158,16 @@ public int getTableSize(String tableName) {
 
     @Override
     public void createDatabase(String ddlMode) {
+        // most databases can't create tables in a transaction, so suspend it
+        Transaction transaction = TransactionHelper.suspendTransaction();
         try {
             createTables(ddlMode);
         } catch (SQLException e) {
             throw new NuxeoException(e);
+        } finally {
+            if (transaction != null) {
+                TransactionHelper.resumeTransaction(transaction);
+            }
         }
     }
 

nuxeo-core/nuxeo-core-storage-sql/nuxeo-core-storage-sql/src/main/java/org/nuxeo/ecm/core/storage/sql/ra/ConnectionImpl.java

@@ -158,13 +158,6 @@ public boolean isLive() {
         return session != null && session.isLive();
     }
 
-    @Override
-    public boolean isStateSharedByAllThreadSessions() {
-        // the JCA semantics is that in the same thread all handles point to the
-        // same underlying session
-        return true;
-    }
-
     @Override
     public String getRepositoryName() {
         return getSession().getRepositoryName();

nuxeo-core/nuxeo-core-test/src/test/java/org/nuxeo/ecm/core/TestSQLRepositoryJTAJCA.java

@@ -40,9 +40,9 @@
 import org.nuxeo.ecm.core.api.CoreSession;
 import org.nuxeo.ecm.core.api.DocumentModel;
 import org.nuxeo.ecm.core.api.DocumentRef;
+import org.nuxeo.ecm.core.api.NuxeoException;
 import org.nuxeo.ecm.core.api.PathRef;
 import org.nuxeo.ecm.core.api.impl.DocumentModelImpl;
-import org.nuxeo.ecm.core.api.local.LocalException;
 import org.nuxeo.ecm.core.api.security.SecurityConstants;
 import org.nuxeo.ecm.core.event.EventService;
 import org.nuxeo.ecm.core.model.Repository;
@@ -187,9 +187,9 @@ public void testAccessWithoutTx() {
         try {
             session.getRootDocument();
             fail("should throw");
-        } catch (LocalException e) {
+        } catch (NuxeoException e) {
             String msg = e.getMessage();
-            assertTrue(msg, msg.contains("No transaction active, cannot reconnect"));
+            assertTrue(msg, msg.contains("Cannot use a CoreSession outside a transaction"));
         }
         TransactionHelper.startTransaction();
     }
@@ -318,9 +318,11 @@ public void testReconnectAfterCloseNoTx() throws Exception {
         try {
             closedSession.getRootDocument();
             fail("should throw");
-        } catch (LocalException e) {
+        } catch (NuxeoException e) {
             String msg = e.getMessage();
-            assertTrue(msg, msg.contains("No transaction active, cannot reconnect"));
+            assertTrue(msg, msg.contains("Cannot use a CoreSession outside a transaction"));
+        } finally {
+            TransactionHelper.startTransaction();
         }
     }
 
@@ -385,7 +387,7 @@ public void run() {
         t.join();
         Exception e = threadException[0];
         assertNotNull(e);
-        assertTrue(e.getMessage(), e instanceof LocalException);
+        assertTrue(e.getMessage(), e instanceof NuxeoException);
     }
 
     @Test

nuxeo-core/nuxeo-core-test/src/test/java/org/nuxeo/ecm/core/test/QueryResultsAreAutomaticallyClosedTest.java

@@ -21,6 +21,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import javax.inject.Inject;
 
@@ -30,6 +31,7 @@
 import org.junit.runner.RunWith;
 import org.nuxeo.ecm.core.api.CoreSession;
 import org.nuxeo.ecm.core.api.IterableQueryResult;
+import org.nuxeo.ecm.core.api.NuxeoException;
 import org.nuxeo.ecm.core.api.UnrestrictedSessionRunner;
 import org.nuxeo.ecm.core.storage.sql.ra.ConnectionImpl;
 import org.nuxeo.ecm.core.test.annotations.RepositoryConfig;
@@ -81,13 +83,15 @@ protected void assertWarnInLogs() throws NoLogCaptureFilterException {
     @Test
     public void testWithoutTransaction() throws Exception {
         TransactionHelper.commitOrRollbackTransaction();
-        IterableQueryResult results;
-        try (CoreSession session = coreFeature.openCoreSessionSystem()) {
-            results = session.queryAndFetch("SELECT * from Document", "NXQL");
+        try {
+            coreFeature.openCoreSessionSystem();
+            fail("Should not allow creation of CoreSession outside a transaction");
+        } catch (NuxeoException e) {
+            String msg = e.getMessage();
+            assertTrue(msg, msg.contains("Cannot create a CoreSession outside a transaction"));
+        } finally {
+            TransactionHelper.startTransaction();
         }
-        TransactionHelper.startTransaction();
-        assertFalse(results.mustBeClosed());
-        assertWarnInLogs();
     }
 
     // needs a JCA connection for this to work

nuxeo-core/nuxeo-core/src/main/java/org/nuxeo/ecm/core/api/local/LocalSession.java

@@ -31,6 +31,7 @@
 import org.nuxeo.ecm.core.api.AbstractSession;
 import org.nuxeo.ecm.core.api.CoreInstance;
 import org.nuxeo.ecm.core.api.CoreSession;
+import org.nuxeo.ecm.core.api.NuxeoException;
 import org.nuxeo.ecm.core.api.NuxeoPrincipal;
 import org.nuxeo.ecm.core.model.Session;
 import org.nuxeo.ecm.core.repository.RepositoryService;
@@ -66,6 +67,9 @@ public class LocalSession extends AbstractSession implements Synchronization {
     private final Set<SessionInfo> allSessions = Collections.newSetFromMap(new ConcurrentHashMap<SessionInfo, Boolean>());
 
     public static CoreSession createInstance() {
+        if (!TransactionHelper.isTransactionActiveOrMarkedRollback()) {
+            throw new NuxeoException("Cannot create a CoreSession outside a transaction");
+        }
         return new LocalSession();
     }
 
@@ -77,7 +81,7 @@ public String getRepositoryName() {
     @Override
     public void connect(String repositoryName, NuxeoPrincipal principal) {
         if (sessionId != null) {
-            throw new LocalException("CoreSession already connected");
+            throw new NuxeoException("CoreSession already connected");
         }
         this.repositoryName = repositoryName;
         this.principal = principal;
@@ -100,13 +104,13 @@ public String getSessionId() {
 
     @Override
     public Session getSession() {
+        if (!TransactionHelper.isTransactionActiveOrMarkedRollback()) {
+            throw new NuxeoException("Cannot use a CoreSession outside a transaction");
+        }
         SessionInfo si = sessionHolder.get();
         if (si == null || !si.session.isLive()) {
             // close old one, previously completed
             closeInThisThread();
-            if (!TransactionHelper.isTransactionActive()) {
-                throw new LocalException("No transaction active, cannot reconnect: " + sessionId);
-            }
             if (log.isDebugEnabled()) {
                 log.debug("Reconnecting CoreSession: " + sessionId);
             }
@@ -186,7 +190,8 @@ public NuxeoPrincipal getPrincipal() {
 
     @Override
     public boolean isStateSharedByAllThreadSessions() {
-        return getSession().isStateSharedByAllThreadSessions();
+        // by design we always share state when in the same thread (through the sessionHolder ThreadLocal)
+        return true;
     }
 
 }

nuxeo-core/nuxeo-core/src/main/java/org/nuxeo/ecm/core/model/Session.java

@@ -79,11 +79,6 @@ IterableQueryResult queryAndFetch(String query, String queryType, QueryFilter qu
      */
     boolean isLive();
 
-    /**
-     * Returns {@code true} if all sessions in the current thread share the same state.
-     */
-    boolean isStateSharedByAllThreadSessions();
-
     /**
      * Closes this session. Does not save.
      */

nuxeo-core/nuxeo-core/src/main/java/org/nuxeo/ecm/core/repository/RepositoryService.java

@@ -89,15 +89,8 @@ public void handleEvent(RuntimeServiceEvent event) {
     @Override
     public void applicationStarted(ComponentContext context) {
         RepositoryManager repositoryManager = Framework.getLocalService(RepositoryManager.class);
-        { // open repositories without a tx active
-            Transaction tx = TransactionHelper.suspendTransaction();
-            try {
-                for (String name : repositoryManager.getRepositoryNames()) {
-                    openRepository(name);
-                }
-            } finally {
-                TransactionHelper.resumeTransaction(tx);
-            }
+        for (String name : repositoryManager.getRepositoryNames()) {
+            openRepository(name);
         }
         // give up if no handler configured
         RepositoryInitializationHandler handler = RepositoryInitializationHandler.getInstance();

nuxeo-core/nuxeo-core/src/test/java/org/nuxeo/ecm/core/event/TestEventContext.java

@@ -20,11 +20,10 @@
 
 package org.nuxeo.ecm.core.event;
 
-import org.junit.Test;
-import static org.junit.Assert.*;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
 
-import org.nuxeo.ecm.core.api.CoreSession;
-import org.nuxeo.ecm.core.api.local.LocalSession;
+import org.junit.Test;
 import org.nuxeo.ecm.core.event.impl.EventContextImpl;
 import org.nuxeo.runtime.test.NXRuntimeTestCase;
 
@@ -40,9 +39,6 @@ public class TestEventContext extends NXRuntimeTestCase {
     @Test
     public void testEventContext() {
         EventContextImpl ctx = new EventContextImpl("arg1", "arg2");
-        CoreSession cs = LocalSession.createInstance();
-        ctx.setCoreSession(cs);
-        assertEquals(cs, ctx.getCoreSession());
         assertEquals("arg1", ctx.getArguments()[0]);
         assertEquals("arg2", ctx.getArguments()[1]);
         ctx.setProperty("p1", "v1");