JDBC-556 Fix named savepoints in dialect 1

- Use QuoteStrategy for escaping savepoint name
- Also fix QuoteStrategy not escaping double quotes

src/documentation/release_notes.md

@@ -227,6 +227,7 @@ The following has been changed or fixed since Jaybird 3.0.5
     only allows next states \[CLOSING, PREPARED, ERROR], received EXECUTING"_ ([JDBC-531](http://tracker.firebirdsql.org/browse/JDBC-531))
 -   New feature: Support for Firebird 3 case sensitive user names ([JDBC-549](http://tracker.firebirdsql.org/browse/JDBC-549))  
     See [Case sensitive user names] for more information.
+-   Fixed: Savepoints did not work in connection dialect 1 as savepoint names were always quoted ([JDBC-556](http://tracker.firebirdsql.org/browse/JDBC-556))  
 
 ### Known issues in Jaybird 3.0.6
 

src/main/org/firebirdsql/jdbc/FBConnection.java

@@ -1289,7 +1289,8 @@ public void setTypeMap(Map<String,Class<?>> map) throws SQLException {
     private int getNextSavepointCounter() {
         return savepointCounter.getAndIncrement();
     }
-
+
+    @Override
     public Savepoint setSavepoint() throws SQLException {
         synchronized (getSynchronizationObject()) {
             checkValidity();
@@ -1320,10 +1321,38 @@ private void setSavepoint(FBSavepoint savepoint) throws SQLException {
 
         txCoordinator.ensureTransaction();
 
-        getGDSHelper().executeImmediate("SAVEPOINT " + savepoint.getServerSavepointId());
+        StringBuilder setSavepoint = new StringBuilder("SAVEPOINT ");
+        getQuoteStrategy().appendQuoted(savepoint.getServerSavepointId(), setSavepoint);
+
+        getGDSHelper().executeImmediate(setSavepoint.toString());
         savepoints.add(savepoint);
     }
 
+    /**
+     * Creates a named savepoint.
+     * <p>
+     * Savepoint names need to be valid Firebird identifiers, and the maximum length is restricted to the maximum
+     * identifier length (see {@link DatabaseMetaData#getMaxColumnNameLength()}. The implementation will take care of
+     * quoting the savepoint name appropriately for the connection dialect. The {@code name} should be passed unquoted.
+     * </p>
+     * <p>
+     * With connection dialect 1, the name is restricted to the rules for unquoted identifier names, that is, its
+     * characters are restricted to {@code A-Za-z0-9$_} and handled case insensitive.
+     * </p>
+     * <p>
+     * For dialect 2 and 3, the name is restricted to the rules for Firebird quoted identifiers (essentially any
+     * printable character and space is valid), and the name is handled case sensitive.
+     * </p>
+     *
+     * @param name
+     *         Savepoint name
+     * @return Savepoint object
+     * @throws SQLException
+     *         if a database access error occurs, this method is called while participating in a distributed
+     *         transaction, this method is called on a closed connection or this {@code Connection} object is currently
+     *         in auto-commit mode
+     */
+    @Override
     public Savepoint setSavepoint(String name) throws SQLException {
         synchronized (getSynchronizationObject()) {
             checkValidity();
@@ -1333,7 +1362,8 @@ public Savepoint setSavepoint(String name) throws SQLException {
             return savepoint;
         }
     }
-
+
+    @Override
     public void rollback(Savepoint savepoint) throws SQLException {
         synchronized (getSynchronizationObject()) {
             checkValidity();
@@ -1354,13 +1384,17 @@ public void rollback(Savepoint savepoint) throws SQLException {
 
             FBSavepoint fbSavepoint = (FBSavepoint) savepoint;
 
-            if (!fbSavepoint.isValid())
+            if (!fbSavepoint.isValid()) {
                 throw new SQLException("Savepoint is no longer valid.");
+            }
 
-            getGDSHelper().executeImmediate("ROLLBACK TO " + fbSavepoint.getServerSavepointId());
+            StringBuilder rollbackSavepoint = new StringBuilder("ROLLBACK TO ");
+            getQuoteStrategy().appendQuoted(fbSavepoint.getServerSavepointId(), rollbackSavepoint);
+            getGDSHelper().executeImmediate(rollbackSavepoint.toString());
         }
     }
 
+    @Override
     public void releaseSavepoint(Savepoint savepoint) throws SQLException {
         synchronized (getSynchronizationObject()) {
             checkValidity();
@@ -1380,7 +1414,9 @@ public void releaseSavepoint(Savepoint savepoint) throws SQLException {
                 throw new SQLException("Savepoint is no longer valid.");
             }
 
-            getGDSHelper().executeImmediate("RELEASE SAVEPOINT " + fbSavepoint.getServerSavepointId() + " ONLY");
+            StringBuilder rollbackSavepoint = new StringBuilder("RELEASE SAVEPOINT ");
+            getQuoteStrategy().appendQuoted(fbSavepoint.getServerSavepointId(), rollbackSavepoint).append(" ONLY");
+            getGDSHelper().executeImmediate(rollbackSavepoint.toString());
 
             fbSavepoint.invalidate();
 
@@ -1651,6 +1687,10 @@ public final Object getSynchronizationObject() {
         }
     }
 
+    private QuoteStrategy getQuoteStrategy() throws SQLException {
+        return QuoteStrategy.forDialect(getGDSHelper().getDialect());
+    }
+
     protected class GeneratedKeysQuery extends AbstractGeneratedKeysQuery {
         protected GeneratedKeysQuery(String sql, int autoGeneratedKeys) throws SQLException {
             super(sql, autoGeneratedKeys);

src/main/org/firebirdsql/jdbc/FBSavepoint.java

@@ -1,7 +1,5 @@
 /*
- * $Id$
- * 
- * Firebird Open Source J2ee connector - jdbc driver
+ * Firebird Open Source JavaEE Connector - JDBC Driver
  *
  * Distributable under LGPL license.
  * You may obtain a copy of the License at http://www.gnu.org/copyleft/lgpl.html
@@ -14,7 +12,7 @@
  * This file was created by members of the firebird development team.
  * All individual contributions remain the Copyright (C) of those
  * individuals.  Contributors to this file are either listed here or
- * can be obtained from a CVS history command.
+ * can be obtained from a source control history command.
  *
  * All rights reserved.
  */
@@ -24,154 +22,124 @@
 
 /**
  * Savepoint implementation.
- * 
+ *
  * @author <a href="mailto:rrokytskyy@users.sourceforge.net">Roman Rokytskyy</a>
  */
 public class FBSavepoint implements FirebirdSavepoint {
-    public static final String SAVEPOINT_ID_PREFIX = "svpt";
 
+    private static final String SAVEPOINT_ID_PREFIX = "SVPT";
+
+    private final boolean named;
+    private final int savepointId;
+    private final String name;
     private boolean valid = true;
-    private int savepointId;
-    private String name;
-    private String serverId;
-
+
     /**
      * Create instance of this class.
-     * 
-     * @param id ID of the savepoint.
+     *
+     * @param savepointId
+     *         ID of the savepoint.
      */
-    public FBSavepoint(int id) {
-        savepointId = id;
-        serverId = getSavepointServerId(id);
+    public FBSavepoint(int savepointId) {
+        this(false, savepointId, getSavepointServerId(savepointId));
     }
-    
+
     /**
      * Create instance of this class for the specified name.
-     * 
-     * @param name name of the savepoint.
+     *
+     * @param name
+     *         name of the savepoint.
      */
     public FBSavepoint(String name) {
-        this.name = name;
-        serverId = getSavepointServerId(name);
+        this(true, -1, name);
     }
-
-    /**
-     * Generate a savepoint ID for the specified savepoint counter.
-     *  
-     * @param counter savepoint counter.
-     * @return valid savepoint ID.
-     */
-    private String getSavepointServerId(int counter) {
-        return SAVEPOINT_ID_PREFIX + counter;
+
+    private FBSavepoint(boolean named, int savepointId, String name) {
+        this.named = named;
+        this.savepointId = savepointId;
+        this.name = name;
     }
-    
+
     /**
-     * Generate a savepoint ID for the specified name.
-     * 
-     * @param name name of the savepoint.
-     * 
-     * @return valid savepoint ID.
+     * Generate a savepoint name for the specified savepoint id.
+     *
+     * @param savePointId
+     *         savepoint id.
+     * @return valid savepoint name.
      */
-    private String getSavepointServerId(String name) {
-        StringBuilder sb = new StringBuilder();
-
-        sb.append('"');
-
-        for (int i = 0; i < name.length(); i++) {
-            char currentChar = name.charAt(i);
-            // we have to double quote quotes
-            if (currentChar == '"') {
-            	sb.append('"');
-            }
-
-            sb.append(currentChar);
+    private static String getSavepointServerId(int savePointId) {
+        if (savePointId >= 0) {
+            return SAVEPOINT_ID_PREFIX + savePointId;
         }
-
-        sb.append('"');
-
-        return sb.toString();
+        return SAVEPOINT_ID_PREFIX + '_' + Math.abs(savePointId);
     }
-    
+
     /**
-     * Get SQL server savepoint ID. This method generates correct ID for the 
-     * savepoint that can be directly used in the SQL statement.
-     * 
-     * @return valid server-side ID for the savepoint.
+     * Get server savepoint name.
+     * <p>
+     * This method generates correct name for the savepoint that can be used in the SQL statement after
+     * dialect-appropriate quoting.
+     * </p>
+     *
+     * @return valid server-side name for the savepoint.
      */
     String getServerSavepointId() {
-        return serverId;
+        return name;
     }
-
-    /**
-     * Get ID of the savepoint.
-     */
+
+    @Override
     public int getSavepointId() throws SQLException {
-        if (name == null)
-            return savepointId;
-        else
+        if (named) {
             throw new SQLException("Savepoint is named.");
+        }
+        return savepointId;
     }
 
-    /**
-     * Get name of the savepoint.
-     */
+    @Override
     public String getSavepointName() throws SQLException {
-        if (name == null)
+        if (!named) {
             throw new SQLException("Savepoint is unnamed.");
-        else
-            return name;
-    }
-
-    /**
-     * Check if this savepoint is named. This method is used internally to avoid
-     * unnecessary exception throwing.
-     * 
-     * @return <code>true</code> if savepoint is named.
-     */
-    boolean isNamed() {
-        return name == null;
+        }
+        return name;
     }
-    
+
     /**
      * Check if the savepoint is valid.
-     * 
-     * @return <code>true</code> if savepoint is valid.
+     *
+     * @return {@code true} if savepoint is valid.
      */
     boolean isValid() {
         return valid;
     }
-    
+
     /**
-     * Make this savepoint invalid. 
+     * Make this savepoint invalid.
      */
     void invalidate() {
         this.valid = false;
     }
-    
+
     /**
-     * Check if objects are equal. For unnamed savepoints their IDs are checked,
-     * otherwise their names.
-     * 
-     * @param obj object to test.
-     * 
-     * @return <code>true</code> if <code>obj</code> is equal to this object.
+     * Check if objects are equal. For unnamed savepoints their IDs are checked, otherwise their names.
+     *
+     * @param obj
+     *         object to test.
+     * @return {@code true} if {@code obj} is equal to this object.
      */
+    @Override
     public boolean equals(Object obj) {
         if (obj == this) return true;
         if (!(obj instanceof FBSavepoint)) return false;
-
-        FBSavepoint that = (FBSavepoint)obj;
-
-        return this.name == null ? 
-            this.savepointId == that.savepointId : 
-            this.name.equals(that.name); 
+
+        FBSavepoint that = (FBSavepoint) obj;
+
+        return this.named == that.named &&
+                this.name.equals(that.name);
     }
-
-    /**
-     * Get hash code of this instance.
-     */
+
+    @Override
     public int hashCode() {
-        return name == null ? savepointId : name.hashCode();
+        return name.hashCode();
     }
 
 }