From cceb7c035cda5f3e1a441e1f544402bd59363171 Mon Sep 17 00:00:00 2001 From: Rob Bygrave Date: Sat, 5 Jul 2014 22:10:53 +1200 Subject: [PATCH] Fix for #159 - Refactor - cleanup clode when closing query only transaction + change default option --- .../ebeaninternal/api/SpiTransaction.java | 5 + .../server/core/BeanRequest.java | 28 ++--- .../server/core/DefaultServer.java | 103 +++++------------- .../server/core/OrmQueryRequest.java | 6 +- .../server/core/RelationalQueryRequest.java | 16 +-- .../server/core/SpiOrmQueryRequest.java | 2 - .../server/transaction/JdbcTransaction.java | 51 ++++++--- .../transaction/TransactionManager.java | 16 +-- .../avaje/tests/basic/TestErrorBindLog.java | 1 - .../tests/query/TestQueryFindIterate.java | 48 ++++++++ .../avaje/tests/query/TestQueryFindVisit.java | 30 +++++ 11 files changed, 162 insertions(+), 144 deletions(-) diff --git a/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java b/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java index 67304e390..f46215278 100644 --- a/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java +++ b/src/main/java/com/avaje/ebeaninternal/api/SpiTransaction.java @@ -15,6 +15,11 @@ import com.avaje.ebeaninternal.server.persist.BatchControl; */ public interface SpiTransaction extends Transaction { + /** + * End the transaction when had query only use. + */ + public void endQueryOnly(); + /** * Return the string prefix with the transactin id and label used in logging. */ diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/BeanRequest.java b/src/main/java/com/avaje/ebeaninternal/server/core/BeanRequest.java index 87fe4d378..b9aad6820 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/BeanRequest.java +++ b/src/main/java/com/avaje/ebeaninternal/server/core/BeanRequest.java @@ -32,8 +32,6 @@ public abstract class BeanRequest { protected boolean createdTransaction; - protected boolean readOnly; - public BeanRequest(SpiEbeanServer ebeanServer, SpiTransaction t) { this.ebeanServer = ebeanServer; this.serverName = ebeanServer.getName(); @@ -62,29 +60,19 @@ public abstract class BeanRequest { if (transaction == null || !transaction.isActive()) { // create an implicit transaction to execute this query transaction = ebeanServer.createServerTransaction(false, -1); - // commented out for performance reasons... - // TODO: review performance of trans.setReadOnly(true) - //if (readOnlyTransaction) { - // readOnly = true; - // transaction.setReadOnly(true); - //} createdTransaction = true; } } } - /** - * Commit this transaction if it was created for this request. - */ - public void commitTransIfRequired() { - if (createdTransaction) { - if (readOnly) { - transaction.rollback(); - } else { - transaction.commit(); - } - } - } + /** + * Commit this transaction if it was created for this request. + */ + public void commitTransIfRequired() { + if (createdTransaction) { + transaction.commit(); + } + } /** * Rollback the transaction if it was created for this request. diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java b/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java index 472caa707..49b86f295 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java +++ b/src/main/java/com/avaje/ebeaninternal/server/core/DefaultServer.java @@ -1198,18 +1198,12 @@ public final class DefaultServer implements SpiEbeanServer { } SpiOrmQueryRequest request = createQueryRequest(desc, spiQuery, t); - try { request.initTransIfRequired(); + return (T) request.findId(); - T bean = (T) request.findId(); + } finally { request.endTransIfRequired(); - - return bean; - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; } } @@ -1256,15 +1250,10 @@ public final class DefaultServer implements SpiEbeanServer { try { request.initTransIfRequired(); - Set set = (Set) request.findSet(); + return (Set) request.findSet(); + + } finally { request.endTransIfRequired(); - - return set; - - } catch (RuntimeException ex) { - // String stackTrace = throwablePrinter.print(ex); - request.rollbackTransIfRequired(); - throw ex; } } @@ -1280,15 +1269,10 @@ public final class DefaultServer implements SpiEbeanServer { try { request.initTransIfRequired(); - Map map = (Map) request.findMap(); + return (Map) request.findMap(); + + } finally { request.endTransIfRequired(); - - return map; - - } catch (RuntimeException ex) { - // String stackTrace = throwablePrinter.print(ex); - request.rollbackTransIfRequired(); - throw ex; } } @@ -1303,14 +1287,10 @@ public final class DefaultServer implements SpiEbeanServer { SpiOrmQueryRequest request = createQueryRequest(Type.ROWCOUNT, query, t); try { request.initTransIfRequired(); - int rowCount = request.findRowCount(); + return request.findRowCount(); + + } finally { request.endTransIfRequired(); - - return rowCount; - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; } } @@ -1326,14 +1306,10 @@ public final class DefaultServer implements SpiEbeanServer { SpiOrmQueryRequest request = createQueryRequest(Type.ID_LIST, query, t); try { request.initTransIfRequired(); - List list = request.findIds(); + return request.findIds(); + + } finally { request.endTransIfRequired(); - - return list; - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; } } @@ -1434,10 +1410,8 @@ public final class DefaultServer implements SpiEbeanServer { try { request.initTransIfRequired(); request.findVisit(visitor); - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; + } finally { + // do nothing - findVisit garuntee's cleanup of the transaction if required } } @@ -1448,10 +1422,9 @@ public final class DefaultServer implements SpiEbeanServer { try { request.initTransIfRequired(); return request.findIterate(); - // request.endTransIfRequired(); - + } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); + request.endTransIfRequired(); throw ex; } } @@ -1468,14 +1441,10 @@ public final class DefaultServer implements SpiEbeanServer { try { request.initTransIfRequired(); - List list = request.findList(); + return request.findList(); + + } finally { request.endTransIfRequired(); - - return list; - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; } } @@ -1518,14 +1487,10 @@ public final class DefaultServer implements SpiEbeanServer { try { request.initTransIfRequired(); - List list = request.findList(); + return request.findList(); + + } finally { request.endTransIfRequired(); - - return list; - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; } } @@ -1535,14 +1500,10 @@ public final class DefaultServer implements SpiEbeanServer { try { request.initTransIfRequired(); - Set set = request.findSet(); + return request.findSet(); + + } finally { request.endTransIfRequired(); - - return set; - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; } } @@ -1551,14 +1512,10 @@ public final class DefaultServer implements SpiEbeanServer { RelationalQueryRequest request = new RelationalQueryRequest(this, relationalQueryEngine, query, t); try { request.initTransIfRequired(); - Map map = request.findMap(); + return request.findMap(); + + } finally { request.endTransIfRequired(); - - return map; - - } catch (RuntimeException ex) { - request.rollbackTransIfRequired(); - throw ex; } } diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/OrmQueryRequest.java b/src/main/java/com/avaje/ebeaninternal/server/core/OrmQueryRequest.java index d26fae2a5..dee215f35 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/OrmQueryRequest.java +++ b/src/main/java/com/avaje/ebeaninternal/server/core/OrmQueryRequest.java @@ -193,14 +193,12 @@ public final class OrmQueryRequest extends BeanRequest implements BeanQueryRe /** * Will end a locally created transaction. *

- * It ends the transaction by using a rollback() as the transaction is known - * to be readOnly. + * It ends the query only transaction. *

*/ public void endTransIfRequired() { if (createdTransaction) { - // we can rollback as readOnly transaction - transaction.rollback(); + transaction.endQueryOnly(); } } diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/RelationalQueryRequest.java b/src/main/java/com/avaje/ebeaninternal/server/core/RelationalQueryRequest.java index b8125e6d2..9af3cc758 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/RelationalQueryRequest.java +++ b/src/main/java/com/avaje/ebeaninternal/server/core/RelationalQueryRequest.java @@ -40,15 +40,6 @@ public final class RelationalQueryRequest { this.trans = (SpiTransaction) t; } - /** - * Rollback the transaction if it was created for this request. - */ - public void rollbackTransIfRequired() { - if (createdTransaction) { - trans.rollback(); - } - } - /** * Create a transaction if none currently exists. */ @@ -58,10 +49,6 @@ public final class RelationalQueryRequest { if (trans == null || !trans.isActive()) { // create a local readOnly transaction trans = ebeanServer.createServerTransaction(false, -1); - - // commented out for performance reasons... - // TODO: review performance of trans.setReadOnly(true) - // trans.setReadOnly(true); createdTransaction = true; } } @@ -72,8 +59,7 @@ public final class RelationalQueryRequest { */ public void endTransIfRequired() { if (createdTransaction) { - // we can rollback as a readOnly transaction. - trans.rollback(); + trans.endQueryOnly(); } } diff --git a/src/main/java/com/avaje/ebeaninternal/server/core/SpiOrmQueryRequest.java b/src/main/java/com/avaje/ebeaninternal/server/core/SpiOrmQueryRequest.java index 145b07798..54dc360d3 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/core/SpiOrmQueryRequest.java +++ b/src/main/java/com/avaje/ebeaninternal/server/core/SpiOrmQueryRequest.java @@ -45,8 +45,6 @@ public interface SpiOrmQueryRequest { */ public void endTransIfRequired(); - public void rollbackTransIfRequired(); - /** * Execute the query as findById. */ diff --git a/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java b/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java index 7f22618e2..2107aad29 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java +++ b/src/main/java/com/avaje/ebeaninternal/server/transaction/JdbcTransaction.java @@ -136,7 +136,7 @@ public class JdbcTransaction implements SpiTransaction { try { this.active = true; this.id = id; - this.logPrefix = deriveLogPrefix(id,null); + this.logPrefix = deriveLogPrefix(id); this.explicit = explicit; this.manager = manager; this.connection = connection; @@ -152,23 +152,17 @@ public class JdbcTransaction implements SpiTransaction { } } - private static String deriveLogPrefix(String id, String label) { + private static String deriveLogPrefix(String id) { + StringBuilder sb = new StringBuilder(); sb.append("txn["); if (id != null) { sb.append(id); } sb.append("] "); - if (label != null) { - sb.append("label[").append(label).append("] "); - } return sb.toString(); } - public void setLabel(String label) { - this.logPrefix = deriveLogPrefix(id,label); - } - public String getLogPrefix() { return logPrefix; } @@ -541,16 +535,21 @@ public class JdbcTransaction implements SpiTransaction { * Notify the transaction manager. */ protected void notifyCommit() { - if (manager == null) { - return; - } - if (queryOnly) { - manager.notifyOfQueryOnly(true, this, null); - } else { - manager.notifyOfCommit(this); + if (manager != null) { + if (queryOnly) { + manager.notifyOfQueryOnly(true, this, null); + } else { + manager.notifyOfCommit(this); + } } } + protected void notifyQueryOnly() { + if (manager != null) { + manager.notifyOfQueryOnly(true, this, null); + } + } + /** * Rollback, Commit or Close for query only transaction. *

@@ -558,7 +557,7 @@ public class JdbcTransaction implements SpiTransaction { * rollback or just close the connection for performance. *

*/ - private void commitQueryOnly() { + private void connectionEndForQueryOnly() { try { switch (onQueryOnly) { case ROLLBACK: @@ -580,6 +579,22 @@ public class JdbcTransaction implements SpiTransaction { } } + /** + * End the transaction on a query only request. + */ + public void endQueryOnly() { + if (!isActive()) { + throw new IllegalStateException(illegalStateMessage); + } + try { + connectionEndForQueryOnly(); + } finally { + // these will not throw an exception + deactivate(); + notifyQueryOnly(); + } + } + /** * Commit the transaction. */ @@ -590,7 +605,7 @@ public class JdbcTransaction implements SpiTransaction { try { if (queryOnly) { // can rollback or just close for performance - commitQueryOnly(); + connectionEndForQueryOnly(); } else { // commit if (batchControl != null && !batchControl.isEmpty()) { diff --git a/src/main/java/com/avaje/ebeaninternal/server/transaction/TransactionManager.java b/src/main/java/com/avaje/ebeaninternal/server/transaction/TransactionManager.java index d26c6c9cd..58460c3f1 100644 --- a/src/main/java/com/avaje/ebeaninternal/server/transaction/TransactionManager.java +++ b/src/main/java/com/avaje/ebeaninternal/server/transaction/TransactionManager.java @@ -91,8 +91,6 @@ public class TransactionManager { private final ClusterManager clusterManager; - //private final int commitDebugLevel; - private final String serverName; /** @@ -100,11 +98,11 @@ public class TransactionManager { */ private AtomicLong transactionCounter = new AtomicLong(1000); - private int clusterDebugLevel; - - private final BulkEventListenerMap bulkEventListenerMap; + private int clusterDebugLevel; - private TransactionEventListener[] transactionEventListeners; + private final BulkEventListenerMap bulkEventListenerMap; + + private TransactionEventListener[] transactionEventListeners; /** * Create the TransactionManager @@ -130,7 +128,7 @@ public class TransactionManager { this.prefix = GlobalProperties.get("transaction.prefix", ""); this.externalTransPrefix = GlobalProperties.get("transaction.prefix", "e"); - String value = GlobalProperties.get("transaction.onqueryonly", "ROLLBACK").toUpperCase().trim(); + String value = GlobalProperties.get("transaction.onqueryonly", "CLOSE").toUpperCase().trim(); this.onQueryOnly = getOnQueryOnly(value, dataSource); initialiseHeartbeat(); @@ -171,7 +169,6 @@ public class TransactionManager { */ private OnQueryOnly getOnQueryOnly(String onQueryOnly, DataSource ds) { - if (onQueryOnly.equals("COMMIT")){ return OnQueryOnly.COMMIT; } @@ -429,9 +426,6 @@ public class TransactionManager { logger.error(m, ex); } } - - - /** * Process a Transaction that comes from another framework or local code. diff --git a/src/test/java/com/avaje/tests/basic/TestErrorBindLog.java b/src/test/java/com/avaje/tests/basic/TestErrorBindLog.java index 03afeab5a..70abe2582 100644 --- a/src/test/java/com/avaje/tests/basic/TestErrorBindLog.java +++ b/src/test/java/com/avaje/tests/basic/TestErrorBindLog.java @@ -21,7 +21,6 @@ public class TestErrorBindLog extends BaseTestCase { } catch (PersistenceException e) { String msg = e.getMessage(); - e.printStackTrace(); Assert.assertTrue(msg.contains("Bind values:")); } } diff --git a/src/test/java/com/avaje/tests/query/TestQueryFindIterate.java b/src/test/java/com/avaje/tests/query/TestQueryFindIterate.java index aea057179..933cd85a3 100644 --- a/src/test/java/com/avaje/tests/query/TestQueryFindIterate.java +++ b/src/test/java/com/avaje/tests/query/TestQueryFindIterate.java @@ -1,5 +1,7 @@ package com.avaje.tests.query; +import javax.persistence.PersistenceException; + import org.junit.Assert; import org.junit.Test; @@ -40,4 +42,50 @@ public class TestQueryFindIterate extends BaseTestCase { Assert.assertEquals(2, count); } + + @Test(expected=PersistenceException.class) + public void testWithExceptionInQuery() { + + ResetBasicData.reset(); + + EbeanServer server = Ebean.getServer(null); + + // intentionally a query with incorrect type binding + Query query = server.find(Customer.class) + .setAutofetch(false) + .where().gt("id","JUNK_NOT_A_LONG") + .setMaxRows(2); + + // this throws an exception immediately + QueryIterator it = query.findIterate(); + it.hashCode(); + Assert.assertTrue("Never get here as exception thrown", false); + } + + + @Test(expected=IllegalStateException.class) + public void testWithExceptionInLoop() { + + ResetBasicData.reset(); + + EbeanServer server = Ebean.getServer(null); + + Query query = server.find(Customer.class) + .setAutofetch(false) + .where().gt("id", 0) + .setMaxRows(2); + + QueryIterator it = query.findIterate(); + try { + while (it.hasNext()) { + Customer customer = it.next(); + if (customer != null) { + throw new IllegalStateException("cause an exception"); + } + } + + } finally { + it.close(); + } + } } diff --git a/src/test/java/com/avaje/tests/query/TestQueryFindVisit.java b/src/test/java/com/avaje/tests/query/TestQueryFindVisit.java index 31f65fee3..6a22228bd 100644 --- a/src/test/java/com/avaje/tests/query/TestQueryFindVisit.java +++ b/src/test/java/com/avaje/tests/query/TestQueryFindVisit.java @@ -39,4 +39,34 @@ public class TestQueryFindVisit extends BaseTestCase { Assert.assertEquals(2, counter.get()); } + + /** + * Test the behaviour when an exception is thrown inside the findVisit(). + */ + @Test(expected=IllegalStateException.class) + public void testVisitThrowingException() { + + ResetBasicData.reset(); + + EbeanServer server = Ebean.getServer(null); + + Query query = server.find(Customer.class).setAutofetch(false) + .fetch("contacts", new FetchConfig().query(2)).where().gt("id", 0).orderBy("id") + .setMaxRows(2); + + final AtomicInteger counter = new AtomicInteger(0); + + query.findVisit(new QueryResultVisitor() { + + public boolean accept(Customer bean) { + counter.incrementAndGet(); + if (counter.intValue() > 0) { + throw new IllegalStateException("cause a failure"); + } + return true; + } + }); + + Assert.assertFalse("Never get here - exception thrown", true); + } }