fix: reWriteBatchedInserts=true causes syntax error with ON CONFLICT (#…

…1082)

INSERT into ... VALUES(...),(...),(...) ON CONFLICT (id) ... confused parser since it considered braces after on conflict to be a part of VALUES clause.

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

@@ -63,6 +63,7 @@
     boolean isValuesFound = false;
     int valuesBraceOpenPosition = -1;
     int valuesBraceClosePosition = -1;
+    boolean valuesBraceCloseFound = false;
     boolean isInsertPresent = false;
     boolean isReturningPresent = false;
     boolean isReturningPresentPrev = false;
@@ -103,7 +104,7 @@
 
         case ')':
           inParen--;
-          if (inParen == 0 && isValuesFound) {
+          if (inParen == 0 && isValuesFound && !valuesBraceCloseFound) {
             // If original statement is multi-values like VALUES (...), (...), ... then
             // search for the latest closing paren
             valuesBraceClosePosition = nativeSql.length() + i - fragmentStart;
@@ -168,6 +169,7 @@
               nativeSql.setLength(0);
               valuesBraceOpenPosition = -1;
               valuesBraceClosePosition = -1;
+              valuesBraceCloseFound = false;
             }
           }
           break;
@@ -184,6 +186,11 @@
           isKeyWordChar = isIdentifierStartChar(aChar);
           if (isKeyWordChar) {
             keywordStart = i;
+            if (valuesBraceOpenPosition != -1 && inParen == 0) {
+              // When the statement already has multi-values, stop looking for more of them
+              // Since values(?,?),(?,?),... should not contain keywords in the middle
+              valuesBraceCloseFound = true;
+            }
           }
           break;
       }

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

@@ -161,4 +161,23 @@ public void insertSelectReturning() throws SQLException {
     boolean returningKeywordPresent = qry.get(0).command.isReturningKeywordPresent();
     Assert.assertTrue("Query has a 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);
+    SqlCommand command = qry.get(0).getCommand();
+    Assert.assertEquals(34, command.getBatchRewriteValuesBraceOpenPosition());
+    Assert.assertEquals(44, command.getBatchRewriteValuesBraceClosePosition());
+  }
+
+  @Test
+  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);
+    SqlCommand command = qry.get(0).getCommand();
+    Assert.assertEquals(34, command.getBatchRewriteValuesBraceOpenPosition());
+    Assert.assertEquals(56, command.getBatchRewriteValuesBraceClosePosition());
+  }
 }

pgjdbc/src/test/java/org/postgresql/test/jdbc2/BaseTest4.java

@@ -25,18 +25,26 @@
     REGULAR, FORCE
   }
 
+  public enum ReWriteBatchedInserts {
+    YES, NO
+  }
+
   public enum AutoCommit {
     YES, NO
   }
 
   protected Connection con;
   private BinaryMode binaryMode;
+  private ReWriteBatchedInserts reWriteBatchedInserts;
   protected PreferQueryMode preferQueryMode;
 
   protected void updateProperties(Properties props) {
     if (binaryMode == BinaryMode.FORCE) {
       forceBinary(props);
     }
+    if (reWriteBatchedInserts == ReWriteBatchedInserts.YES) {
+      PGProperty.REWRITE_BATCHED_INSERTS.set(props, true);
+    }
   }
 
   protected void forceBinary(Properties props) {
@@ -47,6 +55,11 @@ public final void setBinaryMode(BinaryMode binaryMode) {
     this.binaryMode = binaryMode;
   }
 
+  public void setReWriteBatchedInserts(
+      ReWriteBatchedInserts reWriteBatchedInserts) {
+    this.reWriteBatchedInserts = reWriteBatchedInserts;
+  }
+
   @Before
   public void setUp() throws Exception {
     Properties props = new Properties();

pgjdbc/src/test/java/org/postgresql/test/jdbc2/BatchedInsertReWriteEnabledTest.java

@@ -26,15 +26,19 @@
 public class BatchedInsertReWriteEnabledTest extends BaseTest4 {
   private final AutoCommit autoCommit;
 
-  public BatchedInsertReWriteEnabledTest(AutoCommit autoCommit) {
+  public BatchedInsertReWriteEnabledTest(AutoCommit autoCommit,
+      BinaryMode binaryMode) {
     this.autoCommit = autoCommit;
+    setBinaryMode(binaryMode);
   }
 
-  @Parameterized.Parameters(name = "{index}: autoCommit={0}")
+  @Parameterized.Parameters(name = "{index}: autoCommit={0}, binary={1}")
   public static Iterable<Object[]> data() {
     Collection<Object[]> ids = new ArrayList<Object[]>();
     for (AutoCommit autoCommit : AutoCommit.values()) {
-      ids.add(new Object[]{autoCommit});
+      for (BinaryMode binaryMode : BinaryMode.values()) {
+        ids.add(new Object[]{autoCommit, binaryMode});
+      }
     }
     return ids;
   }

pgjdbc/src/test/java/org/postgresql/test/jdbc2/UpsertTest.java

@@ -10,11 +10,13 @@
 import org.postgresql.core.ServerVersion;
 import org.postgresql.test.TestUtil;
 
+import org.junit.Assert;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
 import java.sql.PreparedStatement;
+import java.sql.ResultSet;
 import java.sql.SQLException;
 import java.sql.Statement;
 import java.util.ArrayList;
@@ -26,15 +28,18 @@
  */
 @RunWith(Parameterized.class)
 public class UpsertTest extends BaseTest4 {
-  public UpsertTest(BinaryMode binaryMode) {
+  public UpsertTest(BinaryMode binaryMode, ReWriteBatchedInserts rewrite) {
     setBinaryMode(binaryMode);
+    setReWriteBatchedInserts(rewrite);
   }
 
-  @Parameterized.Parameters(name = "binary = {0}")
+  @Parameterized.Parameters(name = "binary = {0}, reWriteBatchedInserts = {1}")
   public static Iterable<Object[]> data() {
     Collection<Object[]> ids = new ArrayList<Object[]>();
     for (BinaryMode binaryMode : BinaryMode.values()) {
-      ids.add(new Object[]{binaryMode});
+      for (ReWriteBatchedInserts rewrite : ReWriteBatchedInserts.values()) {
+        ids.add(new Object[]{binaryMode, rewrite});
+      }
     }
     return ids;
   }
@@ -94,4 +99,53 @@ public void testUpsertDoUpdateNoConflict() throws SQLException {
     assertEquals("insert on conflict do update should report 1 modified row on plain insert",
         1, count);
   }
+
+  @Test
+  public void testSingleValuedUpsertBatch() throws SQLException {
+    PreparedStatement ps = null;
+    try {
+      ps = con.prepareStatement(
+          "insert into test_statement(i, t) values (?,?) ON CONFLICT (i) DO NOTHING");
+      ps.setInt(1, 50);
+      ps.setString(2, "50");
+      ps.addBatch();
+      ps.setInt(1, 53);
+      ps.setString(2, "53");
+      ps.addBatch();
+      int[] actual = ps.executeBatch();
+      BatchExecuteTest.assertSimpleInsertBatch(2, actual);
+    } finally {
+      TestUtil.closeQuietly(ps);
+    }
+  }
+
+  @Test
+  public void testMultiValuedUpsertBatch() throws SQLException {
+    PreparedStatement ps = null;
+    try {
+      ps = con.prepareStatement(
+          "insert into test_statement(i, t) values (?,?),(?,?) ON CONFLICT (i) DO NOTHING");
+      ps.setInt(1, 50);
+      ps.setString(2, "50");
+      ps.setInt(3, 51);
+      ps.setString(4, "51");
+      ps.addBatch();
+      ps.setInt(1, 52);
+      ps.setString(2, "52");
+      ps.setInt(3, 53);
+      ps.setString(4, "53");
+      ps.addBatch();
+      int[] actual = ps.executeBatch();
+
+      BatchExecuteTest.assertBatchResult("2 batched rows, 2-values each", new int[]{2, 2}, actual);
+
+      Statement st = con.createStatement();
+      ResultSet rs =
+          st.executeQuery("select count(*) from test_statement where i between 50 and 53");
+      rs.next();
+      Assert.assertEquals("test_statement should have 4 rows with 'i' of 50..53", 4, rs.getInt(1));
+    } finally {
+      TestUtil.closeQuietly(ps);
+    }
+  }
 }