Looks good to me.
Thanks,
Andrei
----- Original Message -----
From: Wonseok Kim
To: persistence_at_glassfish.dev.java.net
Sent: Friday, May 18, 2007 6:49 AM
Subject: Code Review for 2278 Query.executeUpdate() method doesn't throw TransactionRequiredException
Hi Tom, Andrei
Please review this really easy fix for the issue - just inserted tx check :-)
https://glassfish.dev.java.net/issues/show_bug.cgi?id=2278
To confirm I also added a new test and fixed one existing test which calls Query.executeUpdate() without tx.
Let me know if you see any problem.
Thanks,
-Wonseok
Index: entity-persistence/src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/EJBQueryImpl.java
===================================================================
RCS file: /cvs/glassfish/entity-persistence/src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/EJBQueryImpl.java,v
retrieving revision 1.37
diff -c -w -r1.37 EJBQueryImpl.java
*** entity-persistence/src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/EJBQueryImpl.java 6 May 2007 02:16:02 -0000 1.37
--- entity-persistence/src/java/oracle/toplink/essentials/internal/ejb/cmp3/base/EJBQueryImpl.java 18 May 2007 09:17:27 -0000
***************
*** 380,385 ****
--- 380,389 ----
if ( !(getDatabaseQuery() instanceof ModifyQuery) ){
throw new IllegalStateException(ExceptionLocalization.buildMessage("incorrect_query_for_execute_update"));
}
+
+ // need to throw TransactionRequiredException if there is no active transaction
+ entityManager.checkForTransaction(true);
+
//fix for bug:4288845, did not add the parameters to the query
Vector parameterValues = processParameters();
if(isFlushModeAUTO()) {
Index: entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/cmp3/advanced/EntityManagerJUnitTestSuite.java
===================================================================
RCS file: /cvs/glassfish/entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/cmp3/advanced/EntityManagerJUnitTestSuite.java,v
retrieving revision 1.49
diff -c -w -r1.49 EntityManagerJUnitTestSuite.java
*** entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/cmp3/advanced/EntityManagerJUnitTestSuite.java 15 May 2007 18:10:27 -0000 1.49
--- entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/cmp3/advanced/EntityManagerJUnitTestSuite.java 18 May 2007 09:17:29 -0000
***************
*** 2306,2311 ****
--- 2306,2312 ----
return;
}
String seqName = "testOracleSequenceDefinition";
+ em.getTransaction().begin();
try {
// first param is preallocationSize, second is startValue
// both should be positive
***************
*** 2313,2318 ****
--- 2314,2322 ----
internalTestOracleSequenceDefinition(10, 5, seqName, em);
internalTestOracleSequenceDefinition(10, 15, seqName, em);
} finally {
+ if(em.getTransaction().isActive()) {
+ em.getTransaction().rollback();
+ }
em.close();
}
}
Index: entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/ejb/ejbqltesting/JUnitEJBQLValidationTestSuite.java
===================================================================
RCS file: /cvs/glassfish/entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/ejb/ejbqltesting/JUnitEJBQLValidationTestSuite.java,v
retrieving revision 1.29
diff -c -w -r1.29 JUnitEJBQLValidationTestSuite.java
*** entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/ejb/ejbqltesting/JUnitEJBQLValidationTestSuite.java 26 Mar 2007 15:06:47 -0000 1.29
--- entity-persistence-tests/src/java/oracle/toplink/essentials/testing/tests/ejb/ejbqltesting/JUnitEJBQLValidationTestSuite.java 18 May 2007 09:17:29 -0000
***************
*** 136,141 ****
--- 136,142 ----
suite.addTest(new JUnitEJBQLValidationTestSuite("illegalArgumentExceptionTest"));
suite.addTest(new JUnitEJBQLValidationTestSuite("createNamedQueryThrowsIllegalArgumentExceptionTest"));
suite.addTest(new JUnitEJBQLValidationTestSuite("flushTxExceptionTest"));
+ suite.addTest(new JUnitEJBQLValidationTestSuite("testExecuteUpdateTxException"));
suite.addTest (new JUnitEJBQLValidationTestSuite("noResultExceptionTest"));
suite.addTest(new JUnitEJBQLValidationTestSuite("testGetSingleResultOnUpdate"));
suite.addTest(new JUnitEJBQLValidationTestSuite("testGetSingleResultOnDelete"));
***************
*** 956,961 ****
--- 957,989 ----
}
}
+ public void testExecuteUpdateTxException()
+ {
+ boolean testPass=false;
+ String ejbqlString = "DELETE FROM Employee e WHERE e.lastName=\"doesNotExist\"";
+
+ EntityManager em = createEntityManager();
+ try
+ {
+ Object result = em.createQuery(ejbqlString).executeUpdate();
+
+ //rollback for clean-up if above call does not fail, otherwise this may affect other tests
+ if(!em.getTransaction().isActive()){
+ em.getTransaction().begin();
+ }
+ em.getTransaction().rollback();
+ }
+ catch (TransactionRequiredException e)
+ {
+ testPass = true;
+ }
+ finally
+ {
+ em.close();
+ }
+ Assert.assertTrue("TransactionRequiredException is expected", testPass);
+ }
+
public void createNamedQueryThrowsIllegalArgumentExceptionTest()
{
try