Cocoon
  1. Cocoon
  2. COCOON-719

[PATCH] Support for transactions in SQLTransformer

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.0.4
    • Fix Version/s: None
    • Component/s: - Components: Sitemap
    • Labels:
      None
    • Environment:
      Operating System: All
      Platform: All

      Description

      I asked about transaction support in the SQLTransformer and got some good
      suggestions about using some DB specific SQL. I might have gone that way (we use
      Oracle 9i), but I wanted something more portable.

      Daniel Fagerstrom posted about a patch he wrote that allowed the SQLTransformer
      to share a connection among top level queries. Great, half way there! (Here's
      the link for that patch... http://issues.apache.org/bugzilla/show_bug.cgi?id=16718).

      I added in code to support setting a parameter like this;

                     <map:transform type="sql">
                         <map:parameter name="use-connection" value="mbrdb"/>
                         <map:parameter name="enable-transaction" value="true"/>
                     </map:transform>

      I put the code in the SQLTransformer to turn off the auto-commit flag and to
      issue the appropriate commit/rollback calls (on error). It works for a couple
      inserts I threw together. If I put an error in the second insert, the first is
      rolled back. If both are good, they both are commited. :-)
      I'm attaching the revised SQLTransformer. I know you guys like to see patches,
      so I attached that also. For now, this code has Daniel's patch applied (onto the
      2.0.4 version) and my additions are bounded by comments (i.e. // DAK:
      Transaction and // DAK)
      Is this something that can be put into the scratchpad? We could give it a
      different package so people could just choose it in the sitemap.xmap
      Here is the context diff with the 2.0.4 SQLTransformer (I added comments
      surrounding my new code to make it easy to spot
      ....
      *** SQLTransformer.java Sun May 18 17:48:50 2003
      --- SQLTransformer.java.orig Tue May 13 22:20:36 2003
      ***************
      *** 102,110 ****
            public static final String MAGIC_PASSWORD = "password";
            public static final String MAGIC_NR_OF_ROWS = "show-nr-of-rows";
            public static final String MAGIC_QUERY = "query";
      - // DAK: Transaction
      - public static final String MAGIC_TRANSACTION = "enable-transaction";
      - // DAK
            public static final String MAGIC_VALUE = "value";
            public static final String MAGIC_DOC_ELEMENT = "doc-element";
            public static final String MAGIC_ROW_ELEMENT = "row-element";
      --- 102,107 ----
      ***************
      *** 186,194 ****
            protected XMLDeserializer interpreter;
            protected Parser parser;
        
      - /** The connection used by all top level queries */
      - protected Connection conn;
      -
            /**
             * Constructor
             */
      --- 183,188 ----
      ***************
      *** 220,237 ****
             */
            public void recycle() {
                super.recycle();
      - try {
      - // Close the connection used by all top level queries
      - if (this.conn != null) {
      - // DAK: Transaction
      - this.conn.commit();
      - // DAK
      - this.conn.close();
      - this.conn = null;
      - }
      - } catch ( SQLException e ) {
      - getLogger().warn( "Could not close the connection", e );
      - }
                this.queries.clear();
                this.outUri = null;
                this.outPrefix = null;
      --- 214,219 ----
      ***************
      *** 292,300 ****
                    getLogger().debug( "ROW-ELEMENT: " + parameters.getParameter(
      SQLTransformer.MAGIC_ROW_ELEMENT, "row" ) );
                    getLogger().debug( "NS-URI: " + parameters.getParameter(
      SQLTransformer.MAGIC_NS_URI_ELEMENT, NAMESPACE ) );
                    getLogger().debug( "NS-PREFIX: " + parameters.getParameter(
      SQLTransformer.MAGIC_NS_PREFIX_ELEMENT, "" ) );
      - // DAK: Transaction
      - getLogger().debug( "TRANSACTION: " + parameters.getParameter(
      SQLTransformer.MAGIC_TRANSACTION, "" ) );
      - // DAK
                }
           }
        
      --- 274,279 ----
      ***************
      *** 317,335 ****
                AttributesImpl attr = new AttributesImpl();
                Query query = (Query) queries.elementAt( index );
                boolean query_failure = false;
      - Connection conn = null;
                try {
                    try {
      - if (index == 0) {
      - if (this.conn == null) // The first top level execute-query
      - this.conn = query.getConnection();
      - // reuse the global connection for all top level queries
      - conn = this.conn;
      - }
      - else // index > 0, sub queries are always executed in an own connection
      - conn = query.getConnection();
      -
      - query.setConnection(conn);
                        query.execute();
                    } catch ( SQLException e ) {
                        if (getLogger().isDebugEnabled()) {
      --- 296,303 ----
      ***************
      *** 372,403 ****
        
                        this.end( query.rowset_name );
                    }
      - // DAK: Transaction
      - else {
      - if (conn.getAutoCommit() == false)
      - conn.rollback();
      - }
      - // DAK
                } catch ( SQLException e ) {
                    if (getLogger().isDebugEnabled()) {
                        getLogger().debug( "SQLTransformer.executeQuery()", e );
                    }
      - // DAK: Transaction
      - try {
      - if (conn.getAutoCommit() == false)
      - conn.rollback();
      - } catch (SQLException ex) {
      - if (getLogger().isDebugEnabled()) {
      - getLogger().debug( "SQLTransformer.executeQuery()", e );
      - }
      - }
      - // DAK
                    throw new SAXException( e );
                } finally {
                    try {
                        query.close();
      - if (index > 0) // close the connection used by a sub query
      - conn.close();
                    } catch ( SQLException e ) {
                        getLogger().warn( "SQLTransformer: Could not close JDBC
      connection", e );
                    }
      --- 340,353 ----
      ***************
      *** 1018,1027 ****
                    }
                }
        
      - protected void setConnection(Connection conn) {
      - this.conn = conn;
      - }
      -
                /** Get a Connection. Made this a separate method to separate the
      logic from the actual execution. */
                protected Connection getConnection() throws SQLException {
                    Connection result = null;
      --- 968,973 ----
      ***************
      *** 1074,1088 ****
                                                                      password );
                            }
                        }
      - // DAK: Transaction
      - String transaction = properties.getParameter(
      SQLTransformer.MAGIC_TRANSACTION, null );
      - if (transaction != null || transaction.trim().toLowerCase().equals("true")) {
      - result.setAutoCommit(false);
      - if (getLogger().isDebugEnabled()) {
      - getLogger().debug("So, someone fetched a connection and transactions are
      enabled...");
      - }
      - }
      - // DAK
                    } catch ( SQLException e ) {
                        transformer.getTheLogger().error( "Caught a SQLException", e );
                        throw e;
      --- 1020,1025 ----
      ***************
      *** 1092,1101 ****
                }
        
                protected void execute() throws SQLException {
      - if (this.conn == null) {
      - throw new SQLException("A connection must be set before executing a query");
      - }
      -
                    this.rowset_name = properties.getParameter(
      SQLTransformer.MAGIC_DOC_ELEMENT, "rowset" );
                    this.row_name = properties.getParameter(
      SQLTransformer.MAGIC_ROW_ELEMENT, "row" );
        
      --- 1029,1034 ----
      ***************
      *** 1123,1128 ****
      --- 1056,1063 ----
                        transformer.getTheLogger().debug( "EXECUTING " + query );
                    }
        
      + conn = getConnection();
      +
                    try {
                        if ( !isstoredprocedure ) {
                            if ( oldDriver ) {
      ***************
      *** 1155,1160 ****
      --- 1090,1098 ----
                    } catch ( SQLException e ) {
                        transformer.getTheLogger().error( "Caught a SQLException", e );
                        throw e;
      + } finally {
      + conn.close();
      + conn = null; // To make sure we don't use this
      connection again.
                    }
                }
        
      ***************
      *** 1233,1238 ****
      --- 1171,1178 ----
                            cst.close();
                        cst = null; // Prevent using cst again.
                    } finally {
      + if ( conn != null )
      + conn.close();
                        conn = null;
                    }
                }

        Activity

        Hide
        David Kavanagh added a comment -
        Created an attachment (id=6729)
        modified SQLTransformer (in full)
        Show
        David Kavanagh added a comment - Created an attachment (id=6729) modified SQLTransformer (in full)
        Hide
        David Kavanagh added a comment -
        Created an attachment (id=7591)
        fixed a bug. Just change the package to replace the standard transformer
        Show
        David Kavanagh added a comment - Created an attachment (id=7591) fixed a bug. Just change the package to replace the standard transformer
        Hide
        David Kavanagh added a comment -
        A co-worker suggested having the "enable-transactions" flag as something that
        can be set in the incomming XML also. That would allow more generic pipelines
        since the query XML would turn the transactions on. (not implemented yet)
        Show
        David Kavanagh added a comment - A co-worker suggested having the "enable-transactions" flag as something that can be set in the incomming XML also. That would allow more generic pipelines since the query XML would turn the transactions on. (not implemented yet)
        Hide
        Jörg Heinicke added a comment -
        25 votes (!) really makes it necessary to set the priority to high.

        David, I hope you are still listening. I want to ask you if you can provide this
        patch again for the recent 2.1 version (which has changed partly since your
        patch date) and provide it in diff form. Daniel's patch (bug #16718) was applied
        some time ago. Now your's can be applied as well, we only need an update on it
        (at least it would simplify committer's life as we had to get the changes from
        your old patch below by hand otherwise).

        Doing it additionally for 2.0 is not a must (Daniel's patch was not applied
        there) as its development was stopped, but if you want to do this as well, no
        one will stop you. We decided to release a last 2.0 release in near future, so
        it would not be useless.

        BTW, you don't need to attach the complete Java file. And we will not change the
        package as we want to avoid duplicate as far as possible.

        Thanks for your understanding,

        Joerg
        Show
        Jörg Heinicke added a comment - 25 votes (!) really makes it necessary to set the priority to high. David, I hope you are still listening. I want to ask you if you can provide this patch again for the recent 2.1 version (which has changed partly since your patch date) and provide it in diff form. Daniel's patch (bug #16718) was applied some time ago. Now your's can be applied as well, we only need an update on it (at least it would simplify committer's life as we had to get the changes from your old patch below by hand otherwise). Doing it additionally for 2.0 is not a must (Daniel's patch was not applied there) as its development was stopped, but if you want to do this as well, no one will stop you. We decided to release a last 2.0 release in near future, so it would not be useless. BTW, you don't need to attach the complete Java file. And we will not change the package as we want to avoid duplicate as far as possible. Thanks for your understanding, Joerg

          People

          • Assignee:
            Unassigned
            Reporter:
            David Kavanagh
          • Votes:
            9 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:

              Development