fix; do not add double quotes to identifiers already double quoted fi…

…xes Issue #2223 (#2224)

* fix; do not add double quotes to identifiers already double quoted fixes Issue #2223

* add test case for quoted identifier

* Added tests for mixed case columns in the returning clause

* Add a property QUOTE_RETURNING_IDENTIFIERS which determines if we put double quotes
around identifiers that are provided in the returning array.

There are some ORMs that now quote all identifiers and if we in turn
quote them this will cause an error.

* update docs for quoteReturning

* Remove unnecessary change

* Unnecessary change

README.md

@@ -133,11 +133,12 @@ In addition to the standard connection parameters the driver supports a number o
 | reWriteBatchedInserts         | Boolean | false   | Enable optimization to rewrite and collapse compatible INSERT statements that are batched. |
 | escapeSyntaxCallMode          | String  | select  | Specifies how JDBC escape call syntax is transformed into underlying SQL (CALL/SELECT), for invoking procedures or functions (requires server version >= 11), possible values: select, callIfNoReturn, call |
 | maxResultBuffer               | String  | null    | Specifies size of result buffer in bytes, which can't be exceeded during reading result set. Can be specified as particular size (i.e. "100", "200M" "2G") or as percent of max heap memory (i.e. "10p", "20pct", "50percent") |
-| gssEncMode                    | String  | allow  | Controls the preference for using GSSAPI encryption for the connection,  values are disable, allow, prefer, and require |
+| gssEncMode                    | String  | allow   | Controls the preference for using GSSAPI encryption for the connection, values are disable, allow, prefer, and require |
 | adaptiveFetch                 | Boolean | false   | Specifies if number of rows fetched in ResultSet by each fetch iteration should be dynamic. Number of rows will be calculated by dividing maxResultBuffer size into max row size observed so far. Requires declaring maxResultBuffer and defaultRowFetchSize for first iteration. 
 | adaptiveFetchMinimum          | Integer | 0       | Specifies minimum number of rows, which can be calculated by adaptiveFetch. Number of rows used by adaptiveFetch cannot go below this value. 
 | adaptiveFetchMaximum          | Integer | -1      | Specifies maximum number of rows, which can be calculated by adaptiveFetch. Number of rows used by adaptiveFetch cannot go above this value. Any negative number set as adaptiveFetchMaximum is used by adaptiveFetch as infinity number of rows.
 | localSocketAddress            | String  | null    | Hostname or IP address given to explicitly configure the interface that the driver will bind the client side of the TCP/IP connection to when connecting.
+| quoteReturningIdentifiers     | Boolean | true    | By default we double quote returning identifiers. Some ORM's already quote them. Switch allows them to turn this off
 
 ## Contributing
 For information on how to contribute to the project see the [Contributing Guidelines](CONTRIBUTING.md)

docs/documentation/head/connect.md

@@ -575,6 +575,13 @@ Connection conn = DriverManager.getConnection(url);
 
 	By default this is set to true, server error details are propagated. This may include sensitive details such as query parameters.
 
+* **quoteReturningIdentifiers** == boolean
+
+  Quote returning columns.
+  There are some ORM's that quote everything, including returning columns
+  If we quote them, then we end up sending ""colname"" to the backend instead of "colname"
+  which will not be found.
+
 <a name="unix sockets"></a>
 ## Unix sockets
 

pgjdbc/src/main/java/org/postgresql/PGProperty.java

@@ -448,6 +448,17 @@ public enum PGProperty {
       false,
       new String[] {"3"}),
 
+  /**
+   * Quote returning columns.
+   * There are some ORM's that quote everything, including returning columns
+   * If we quote them, then we end up sending ""colname"" to the backend
+   * which will not be found
+   */
+  QUOTE_RETURNING_IDENTIFIERS(
+    "quoteReturningIdentifiers",
+    "true",
+    "Quote identifiers provided in returning array",
+      false),
   /**
    * Puts this connection in read-only mode.
    */

pgjdbc/src/main/java/org/postgresql/core/CachedQueryCreateAction.java

@@ -64,7 +64,9 @@ public CachedQuery create(Object key) throws SQLException {
 
     List<NativeQuery> queries = Parser.parseJdbcSql(parsedSql,
         queryExecutor.getStandardConformingStrings(), isParameterized, splitStatements,
-        queryExecutor.isReWriteBatchedInsertsEnabled(), returningColumns);
+        queryExecutor.isReWriteBatchedInsertsEnabled(), queryExecutor.getQuoteReturningIdentifiers(),
+        returningColumns
+        );
 
     Query query = queryExecutor.wrap(queries);
     return new CachedQuery(key, query, isFunction);

pgjdbc/src/main/java/org/postgresql/core/Parser.java

@@ -40,13 +40,15 @@ public class Parser {
    * @param withParameters            whether to replace ?, ? with $1, $2, etc
    * @param splitStatements           whether to split statements by semicolon
    * @param isBatchedReWriteConfigured whether re-write optimization is enabled
+   * @param quoteReturningIdentifiers whether to quote identifiers returned using returning clause
    * @param returningColumnNames      for simple insert, update, delete add returning with given column names
    * @return list of native queries
    * @throws SQLException if unable to add returning clause (invalid column names)
    */
   public static List<NativeQuery> parseJdbcSql(String query, boolean standardConformingStrings,
       boolean withParameters, boolean splitStatements,
       boolean isBatchedReWriteConfigured,
+      boolean quoteReturningIdentifiers,
       String... returningColumnNames) throws SQLException {
     if (!withParameters && !splitStatements
         && returningColumnNames != null && returningColumnNames.length == 0) {
@@ -145,7 +147,7 @@ public static List<NativeQuery> parseJdbcSql(String query, boolean standardConfo
             }
             fragmentStart = i + 1;
             if (nativeSql.length() > 0) {
-              if (addReturning(nativeSql, currentCommandType, returningColumnNames, isReturningPresent)) {
+              if (addReturning(nativeSql, currentCommandType, returningColumnNames, isReturningPresent, quoteReturningIdentifiers)) {
                 isReturningPresent = true;
               }
 
@@ -284,7 +286,7 @@ public static List<NativeQuery> parseJdbcSql(String query, boolean standardConfo
       return nativeQueries != null ? nativeQueries : Collections.<NativeQuery>emptyList();
     }
 
-    if (addReturning(nativeSql, currentCommandType, returningColumnNames, isReturningPresent)) {
+    if (addReturning(nativeSql, currentCommandType, returningColumnNames, isReturningPresent, quoteReturningIdentifiers)) {
       isReturningPresent = true;
     }
 
@@ -346,7 +348,7 @@ public static List<NativeQuery> parseJdbcSql(String query, boolean standardConfo
   }
 
   private static boolean addReturning(StringBuilder nativeSql, SqlCommandType currentCommandType,
-      String[] returningColumnNames, boolean isReturningPresent) throws SQLException {
+      String[] returningColumnNames, boolean isReturningPresent, boolean quoteReturningIdentifiers) throws SQLException {
     if (isReturningPresent || returningColumnNames.length == 0) {
       return false;
     }
@@ -367,7 +369,14 @@ private static boolean addReturning(StringBuilder nativeSql, SqlCommandType curr
       if (col > 0) {
         nativeSql.append(", ");
       }
-      Utils.escapeIdentifier(nativeSql, columnName);
+      /*
+      If the client quotes identifiers then doing so again would create an error
+       */
+      if (quoteReturningIdentifiers) {
+        Utils.escapeIdentifier(nativeSql, columnName);
+      } else {
+        nativeSql.append( columnName );
+      }
     }
     return true;
   }

pgjdbc/src/main/java/org/postgresql/core/QueryExecutor.java

@@ -461,6 +461,12 @@ Object createQueryKey(String sql, boolean escapeProcessing, boolean isParameteri
    */
   boolean getStandardConformingStrings();
 
+  /**
+   *
+   * @return true if we are going to quote identifier provided in the returning array default is true
+   */
+  boolean getQuoteReturningIdentifiers();
+
   /**
    * Returns backend timezone in java format.
    * @return backend timezone in java format.

pgjdbc/src/main/java/org/postgresql/core/QueryExecutorBase.java

@@ -47,6 +47,7 @@ public abstract class QueryExecutorBase implements QueryExecutor {
   private final boolean reWriteBatchedInserts;
   private final boolean columnSanitiserDisabled;
   private final EscapeSyntaxCallMode escapeSyntaxCallMode;
+  private final boolean quoteReturningIdentifiers;
   private final PreferQueryMode preferQueryMode;
   private AutoSave autoSave;
   private boolean flushCacheOnDeallocate = true;
@@ -76,6 +77,7 @@ protected QueryExecutorBase(PGStream pgStream, String user,
     this.columnSanitiserDisabled = PGProperty.DISABLE_COLUMN_SANITISER.getBoolean(info);
     String callMode = PGProperty.ESCAPE_SYNTAX_CALL_MODE.get(info);
     this.escapeSyntaxCallMode = EscapeSyntaxCallMode.of(callMode);
+    this.quoteReturningIdentifiers = PGProperty.QUOTE_RETURNING_IDENTIFIERS.getBoolean(info);
     String preferMode = PGProperty.PREFER_QUERY_MODE.get(info);
     this.preferQueryMode = PreferQueryMode.of(preferMode);
     this.autoSave = AutoSave.of(PGProperty.AUTOSAVE.get(info));
@@ -269,6 +271,11 @@ public synchronized boolean getStandardConformingStrings() {
     return standardConformingStrings;
   }
 
+  @Override
+  public boolean getQuoteReturningIdentifiers() {
+    return quoteReturningIdentifiers;
+  }
+
   @Override
   public synchronized TransactionState getTransactionState() {
     return transactionState;

pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java

@@ -237,7 +237,7 @@ boolean hasLock(@Nullable Object holder) {
   public Query createSimpleQuery(String sql) throws SQLException {
     List<NativeQuery> queries = Parser.parseJdbcSql(sql,
         getStandardConformingStrings(), false, true,
-        isReWriteBatchedInsertsEnabled());
+        isReWriteBatchedInsertsEnabled(), getQuoteReturningIdentifiers());
     return wrap(queries);
   }
 

pgjdbc/src/main/java/org/postgresql/ds/common/BaseDataSource.java

@@ -378,6 +378,22 @@ public void setProtocolVersion(int protocolVersion) {
     }
   }
 
+  /**
+   * @return quoteReturningIdentifiers
+   * @see PGProperty#QUOTE_RETURNING_IDENTIFIERS
+   */
+  public boolean getQuoteReturningIdentifiers() {
+    return PGProperty.QUOTE_RETURNING_IDENTIFIERS.getBoolean(properties);
+  }
+
+  /**
+   * @param quoteIdentifiers indicate whether to quote identifiers
+   * @see PGProperty#QUOTE_RETURNING_IDENTIFIERS
+   */
+  public void setQuoteReturningIdentifiers(boolean quoteIdentifiers) {
+    PGProperty.QUOTE_RETURNING_IDENTIFIERS.set(properties, quoteIdentifiers);
+  }
+
   /**
    * @return receive buffer size
    * @see PGProperty#RECEIVE_BUFFER_SIZE

pgjdbc/src/test/java/org/postgresql/core/ParserTest.java

@@ -168,7 +168,7 @@ public void insertSelectFakeReturning() throws SQLException {
         "insert test(id, name) select 1, 'value' as RETURNING from test2";
     List<NativeQuery> qry =
         Parser.parseJdbcSql(
-            query, true, true, true, true);
+            query, true, true, true, true, true);
     boolean returningKeywordPresent = qry.get(0).command.isReturningKeywordPresent();
     Assert.assertFalse("Query does not have returning clause " + query, returningKeywordPresent);
   }
@@ -179,7 +179,7 @@ public void insertSelectReturning() throws SQLException {
         "insert test(id, name) select 1, 'value' from test2 RETURNING id";
     List<NativeQuery> qry =
         Parser.parseJdbcSql(
-            query, true, true, true, true);
+            query, true, true, true, true, true);
     boolean returningKeywordPresent = qry.get(0).command.isReturningKeywordPresent();
     Assert.assertTrue("Query has a returning clause " + query, returningKeywordPresent);
   }
@@ -190,15 +190,15 @@ public void insertReturningInWith() throws SQLException {
         "with x as (insert into mytab(x) values(1) returning x) insert test(id, name) select 1, 'value' from test2";
     List<NativeQuery> qry =
         Parser.parseJdbcSql(
-            query, true, true, true, true);
+            query, true, true, true, true, true);
     boolean returningKeywordPresent = qry.get(0).command.isReturningKeywordPresent();
     Assert.assertFalse("There's no top-level <<returning>> clause " + query, returningKeywordPresent);
   }
 
   @Test
   public void insertBatchedReWriteOnConflict() throws SQLException {
     String query = "insert into test(id, name) values (:id,:name) ON CONFLICT (id) DO NOTHING";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertEquals(34, command.getBatchRewriteValuesBraceOpenPosition());
     Assert.assertEquals(44, command.getBatchRewriteValuesBraceClosePosition());
@@ -207,15 +207,15 @@ public void insertBatchedReWriteOnConflict() throws SQLException {
   @Test
   public void insertBatchedReWriteOnConflictUpdateBind() throws SQLException {
     String query = "insert into test(id, name) values (?,?) ON CONFLICT (id) UPDATE SET name=?";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertFalse("update set name=? is NOT compatible with insert rewrite", command.isBatchedReWriteCompatible());
   }
 
   @Test
   public void insertBatchedReWriteOnConflictUpdateConstant() throws SQLException {
     String query = "insert into test(id, name) values (?,?) ON CONFLICT (id) UPDATE SET name='default'";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertTrue("update set name='default' is compatible with insert rewrite", command.isBatchedReWriteCompatible());
   }
@@ -224,7 +224,7 @@ public void insertBatchedReWriteOnConflictUpdateConstant() throws SQLException {
   public void insertMultiInsert() throws SQLException {
     String query =
         "insert into test(id, name) values (:id,:name),(:id,:name) ON CONFLICT (id) DO NOTHING";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertEquals(34, command.getBatchRewriteValuesBraceOpenPosition());
     Assert.assertEquals(56, command.getBatchRewriteValuesBraceClosePosition());
@@ -233,13 +233,13 @@ public void insertMultiInsert() throws SQLException {
   @Test
   public void valuesTableParse() throws SQLException {
     String query = "insert into values_table (id, name) values (?,?)";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertEquals(43,command.getBatchRewriteValuesBraceOpenPosition());
     Assert.assertEquals(49,command.getBatchRewriteValuesBraceClosePosition());
 
     query = "insert into table_values (id, name) values (?,?)";
-    qry = Parser.parseJdbcSql(query, true, true, true, true);
+    qry = Parser.parseJdbcSql(query, true, true, true, true, true);
     command = qry.get(0).getCommand();
     Assert.assertEquals(43,command.getBatchRewriteValuesBraceOpenPosition());
     Assert.assertEquals(49,command.getBatchRewriteValuesBraceClosePosition());
@@ -249,7 +249,7 @@ public void valuesTableParse() throws SQLException {
   public void createTableParseWithOnDeleteClause() throws SQLException {
     String[] returningColumns = {"*"};
     String query = "create table \"testTable\" (\"id\" INT SERIAL NOT NULL PRIMARY KEY, \"foreignId\" INT REFERENCES \"otherTable\" (\"id\") ON DELETE NO ACTION)";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, returningColumns);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true, returningColumns);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertFalse("No returning keyword should be present", command.isReturningKeywordPresent());
     Assert.assertEquals(SqlCommandType.CREATE, command.getType());
@@ -259,7 +259,7 @@ public void createTableParseWithOnDeleteClause() throws SQLException {
   public void createTableParseWithOnUpdateClause() throws SQLException {
     String[] returningColumns = {"*"};
     String query = "create table \"testTable\" (\"id\" INT SERIAL NOT NULL PRIMARY KEY, \"foreignId\" INT REFERENCES \"otherTable\" (\"id\")) ON UPDATE NO ACTION";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, returningColumns);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true, returningColumns);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertFalse("No returning keyword should be present", command.isReturningKeywordPresent());
     Assert.assertEquals(SqlCommandType.CREATE, command.getType());
@@ -269,7 +269,7 @@ public void createTableParseWithOnUpdateClause() throws SQLException {
   public void alterTableParseWithOnDeleteClause() throws SQLException {
     String[] returningColumns = {"*"};
     String query = "alter table \"testTable\" ADD \"foreignId\" INT REFERENCES \"otherTable\" (\"id\") ON DELETE NO ACTION";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, returningColumns);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true, returningColumns);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertFalse("No returning keyword should be present", command.isReturningKeywordPresent());
     Assert.assertEquals(SqlCommandType.ALTER, command.getType());
@@ -279,7 +279,7 @@ public void alterTableParseWithOnDeleteClause() throws SQLException {
   public void alterTableParseWithOnUpdateClause() throws SQLException {
     String[] returningColumns = {"*"};
     String query = "alter table \"testTable\" ADD \"foreignId\" INT REFERENCES \"otherTable\" (\"id\") ON UPDATE RESTRICT";
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, returningColumns);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true, returningColumns);
     SqlCommand command = qry.get(0).getCommand();
     Assert.assertFalse("No returning keyword should be present", command.isReturningKeywordPresent());
     Assert.assertEquals(SqlCommandType.ALTER, command.getType());

pgjdbc/src/test/java/org/postgresql/core/ReturningParserTest.java

@@ -52,7 +52,7 @@ public void test() throws SQLException {
     String query =
         "insert into\"prep\"(a, " + prefix + columnName + suffix + ")values(1,2)" + prefix
             + returning + suffix;
-    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true);
+    List<NativeQuery> qry = Parser.parseJdbcSql(query, true, true, true, true, true);
     boolean returningKeywordPresent = qry.get(0).command.isReturningKeywordPresent();
 
     boolean expectedReturning = this.returning.equalsIgnoreCase("returning")