Re: svn commit: r124610 - in petstore/java/org/apache/cocoon/components/flow/javascript/...

[Gunnar Brand <g....@interface-projects.de>]

I noticed (kinda late), that this is almost the same for the 
ScriptableConnection as (the still open?) bug 30326
http://issues.apache.org/bugzilla/show_bug.cgi?id=30326
which was never applied imho. The patch proposed there discards the 
exception in the try { close() } block.
The one I see here (Cocoon 2.1.7, svn web doesn't work at the time of this 
writing, so I can't check head) does throw the exception caught for close.

I think both versions are not 100% perfect.

30326 discards exceptions happening when close() bugs out but the previous 
statement worked fine. This is not good.
But it works fine if the statement failed by not throwing another exception 
in finally, shadowing the first exception.
For example the statement failed with a good reason, and the close fails 
with a not so good reason due to the statement failing. The developer will 
have a hard time finding the real reason.

The current 2.1.7 version works the opposite way.
Good if the statement worked but close() fails and bad if the statement and 
close() fail.

The only proper way I can think of would be storing the first thrown 
exception in all catch blocks in a method wide variable and throwing it 
wrapped into a javascript exception at the end of the finally block (i.e. 
store and rethrow only if it is !=null).

Alternatively a flag indicating that a excpetion has been thrown works 
quite well, too. Since this can be an exception, short ifs can be used to 
avoid extra code.

I added some pseudo code to the quoted diff below for both ideas...

Gunnar


 > --- 
cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components 
\
 >                 /flow/javascript/ScriptableConnection.java    (original)
 > +++ 
cocoon/branches/BRANCH_2_1_X/src/blocks/petstore/java/org/apache/cocoon/components 
\
 > /flow/javascript/ScriptableConnection.java    Fri Jan  7 18:53:56 2005 
@@ -18,6 +18,7 @@
 >  import java.sql.PreparedStatement;
 >  import java.sql.ResultSet;
 >  import java.sql.ResultSetMetaData;
 > +import java.sql.SQLException;
 >
 >  import org.mozilla.javascript.Context;
 >  import org.mozilla.javascript.Function;
 > @@ -66,7 +67,7 @@
 >   * A ScriptableConnection is also a wrapper around a real JDBC 
Connection and thus
 >   * provides all of methods of Connection as well
 >   *
 > - * @version CVS $Id: ScriptableConnection.java,v 1.6 2004/03/05 
13:02:03 bdelacretaz \
 > Exp $ + * @version CVS $Id$
 >   */
 >  public class ScriptableConnection extends ScriptableObject {
 >
 > @@ -206,9 +207,11 @@
 >      public Object jsFunction_query(String sql, Object params,
 >                                     int startRow, int maxRows,
 >                                     Object funObj)
 > -        throws JavaScriptException {
 > +            throws JavaScriptException {
 > +
              Exception firstException = null;
 > +        PreparedStatement stmt = null;
 >          try {
 > -            PreparedStatement stmt = connection.prepareStatement(sql);
 > +            stmt = connection.prepareStatement(sql);
 >              Scriptable array = (Scriptable)params;
 >              if (array != Undefined.instance) {
 >                  int len = (int)
 > @@ -272,13 +275,22 @@
 >              throw e;
 >          } catch (Exception e) {
 >              throw new JavaScriptException(e);
//              ^^^ remove
                 firstException = e;
 > +        } finally {
 > +            try {
 > +                if (stmt != null) {
 > +                    stmt.close();
 > +                }
 > +            } catch (SQLException sqle) {
 > +                throw new JavaScriptException(sqle);
//                   ^^^ remove
                      if ( firstException!=null ) firstException = sqle;
 > +            }
                  if ( firstException!=null ) throw new 
JavaScriptException(firstException);
 >          }
 >      }
 >

The next method needs these changes, too...

 >      public int jsFunction_update(String sql, Object params)
 >          throws JavaScriptException {
              Exception thrownException = null;
 > +        PreparedStatement stmt = null;
 >          try {
 > -            PreparedStatement stmt = connection.prepareStatement(sql);
 > +            stmt = connection.prepareStatement(sql);
 >              Scriptable array = (Scriptable)params;
 >              if (array != Undefined.instance) {
 >                  int len = (int)
 > @@ -298,6 +310,14 @@
 >              return stmt.getUpdateCount();
 >          } catch (Exception e) {
 >              throw new JavaScriptException(e);
//              ^^^ remove
                 throw new JavaScriptException(thrownException = e);   // 
first throw, no check necessary
 > +        } finally {
 > +            try {
 > +                if (stmt != null) {
 > +                    stmt.close();
 > +                }
 > +            } catch (SQLException sqle) {
 > +                throw new JavaScriptException(sqle);
//                   ^^^ remove
 > +                throw new JavaScriptException(thrownException = 
thrownException!=null ? thrownException : sqle);
 > +            }
 >          }
 >      }
 >




-- 
G. Brand - interface projects GmbH
Tolkewitzer Strasse 49
D-01277 Dresden
mail:   g.brand@interface-projects.de
tel:    ++49-351-3 18 09 - 41

Ein Unternehmen der interface:business-Gruppe