fix: Allows unclosed quote characters to exist in comments (#4993)

* fix: Allows quote characters to exist in comments, e.g. SELECT -- It's a comment

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/JLineReader.java

@@ -91,7 +91,6 @@ private static LineReader build(
   ) {
     final DefaultParser parser = new DefaultParser();
     parser.setEofOnEscapedNewLine(true);
-    parser.setEofOnUnclosedQuote(true);
     parser.setQuoteChars(new char[]{'\''});
     parser.setEscapeChars(new char[]{'\\'});
 

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/KsqlLineParser.java

@@ -47,6 +47,10 @@ public ParsedLine parse(final String line, final int cursor, final ParseContext
       return parsed;
     }
 
+    if (UnclosedQuoteChecker.isUnclosedQuote(line)) {
+      throw new EOFError(-1, -1, "Missing end quote", "end quote char");
+    }
+
     final String bare = CommentStripper.strip(parsed.line());
     if (bare.isEmpty()) {
       return parsed;

ksqldb-cli/src/main/java/io/confluent/ksql/cli/console/UnclosedQuoteChecker.java

@@ -0,0 +1,81 @@
+/*
+ * Copyright 2020 Confluent Inc.
+ *
+ * Licensed under the Confluent Community License (the "License"); you may not use
+ * this file except in compliance with the License.  You may obtain a copy of the
+ * License at
+ *
+ * http://www.confluent.io/confluent-community-license
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OF ANY KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+
+package io.confluent.ksql.cli.console;
+
+/**
+ * Checks to see if the line is in the middle of a quote.  Unlike
+ * org.jline.reader.impl.DefaultParser, this is comment aware.
+ */
+public final class UnclosedQuoteChecker {
+  private static final String COMMENT = "--";
+
+  private UnclosedQuoteChecker() {}
+
+  // CHECKSTYLE_RULES.OFF: CyclomaticComplexity
+  public static boolean isUnclosedQuote(final String line) {
+    // CHECKSTYLE_RULES.ON: CyclomaticComplexity
+    int quoteStart = -1;
+    for (int i = 0; i < line.length(); ++i) {
+      if (quoteStart < 0 && isQuoteChar(line, i)) {
+        quoteStart = i;
+      } else if (quoteStart >= 0 && isTwoQuoteStart(line, i) && !isEscaped(line, i)) {
+        // Together, two quotes are effectively an escaped quote and don't act as a quote character.
+        // Skip the next quote char, since it's coupled with the first.
+        i++;
+      } else if (quoteStart >= 0 && isQuoteChar(line, i) && !isEscaped(line, i)) {
+        quoteStart = -1;
+      }
+    }
+    final int commentInd = line.indexOf(COMMENT);
+    if (commentInd < 0) {
+      return quoteStart >= 0;
+    } else if (quoteStart < 0) {
+      return false;
+    } else {
+      return commentInd > quoteStart;
+    }
+  }
+
+  private static boolean isQuoteChar(final String line, final int ind) {
+    final char c = line.charAt(ind);
+    if (c == '\'') {
+      return true;
+    }
+    return false;
+  }
+
+  private static boolean isEscaped(final String line, final int ind) {
+    if (ind == 0) {
+      return false;
+    }
+    final char c = line.charAt(ind - 1);
+    if (c == '\\' && !isEscaped(line, ind - 1)) {
+      return true;
+    }
+    return false;
+  }
+
+  // Technically, it is sufficient to ensure that quotes are paired up to answer the "unclosed"
+  // question.  It's still clearer to differentiate between the two-quote escaped quote and two
+  // back-to-back strings (e.g. 'ab''cd' is really evaluated as "ab'cd" rather than 'ab' and 'cd'),
+  // so we explicitly check for that case here.
+  private static boolean isTwoQuoteStart(final String line, final int ind) {
+    if (ind + 1 >= line.length()) {
+      return false;
+    }
+    return isQuoteChar(line, ind) && isQuoteChar(line, ind + 1);
+  }
+}

ksqldb-cli/src/main/java/io/confluent/ksql/util/CmdLineUtil.java

@@ -70,13 +70,17 @@ private static final class WhitespaceParser {
 
     private boolean inQuotes;
     private boolean inWhitespace;
+    private boolean inComment;
+    private char prevChar;
     private String input;
     private ImmutableList.Builder<String> output;
     private final StringBuilder currentToken = new StringBuilder();
 
     private List<String> parse(final String s) {
       inQuotes = false;
       inWhitespace = false;
+      inComment = false;
+      prevChar = '\0';
       input = s.trim();
       currentToken.setLength(0);
       output = ImmutableList.builder();
@@ -93,14 +97,17 @@ private List<String> parse(final String s) {
       return output.build();
     }
 
+    // CHECKSTYLE_RULES.OFF: CyclomaticComplexity
     private int processCharAt(final int pos) {
+      // CHECKSTYLE_RULES.ON: CyclomaticComplexity
       final char c = input.charAt(pos);
       int returnPos = pos;
 
       if (!Character.isWhitespace(c)) {
         inWhitespace = false;
-      } else if (!inQuotes && !inWhitespace) {
+      } else if (!inQuotes && !inWhitespace && !(inComment && c != '\n')) {
         inWhitespace = true;
+        inComment = false;
 
         output.add(currentToken.toString());
         currentToken.setLength(0);
@@ -109,8 +116,11 @@ private int processCharAt(final int pos) {
       if (!inWhitespace) {
         currentToken.append(c);
       }
+      if (isLineComment(prevChar, c)) {
+        inComment = true;
+      }
 
-      if (c == '\'') {
+      if (c == '\'' && !inComment) {
         if (!inQuotes) {
           inQuotes = true;
         } else if (isEscapedChar(input, pos)) {
@@ -120,10 +130,15 @@ private int processCharAt(final int pos) {
           inQuotes = false;
         }
       }
+      prevChar = c;
       return returnPos;
     }
   }
 
+  private static boolean isLineComment(final char prevChar, final char current) {
+    return prevChar == '-' && current == '-';
+  }
+
   private static final class QuoteRemover {
     private static final int NOT_SET = -1;
     private int quoteStart;

ksqldb-cli/src/test/java/io/confluent/ksql/cli/console/UnclosedQuoteCheckerTest.java

@@ -0,0 +1,195 @@
+/*
+ * Copyright 2020 Confluent Inc.
+ *
+ * Licensed under the Confluent Community License (the "License"); you may not use
+ * this file except in compliance with the License.  You may obtain a copy of the
+ * License at
+ *
+ * http://www.confluent.io/confluent-community-license
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OF ANY KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+
+package io.confluent.ksql.cli.console;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
+
+import org.junit.Test;
+
+public class UnclosedQuoteCheckerTest {
+
+  @Test
+  public void shouldFindUnclosedQuote() {
+    // Given:
+    final String line = "some line 'this is in a quote";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(true));
+  }
+
+  @Test
+  public void shouldFindUnclosedQuote_commentCharsInside() {
+    // Given:
+    final String line = "some line 'this is in a quote -- not a comment";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(true));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_commentCharsInside() {
+    // Given:
+    final String line = "some line 'this is in a quote -- not a comment'";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldFindUnclosedQuote_escaped() {
+    // Given:
+    final String line = "some line 'this is in a quote\\'";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(true));
+  }
+
+  @Test
+  public void shouldFindUnclosedQuote_escapedEscape() {
+    // Given:
+    final String line = "some line 'this is in a quote\\\\'";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldFindUnclosedQuote_escapedThree() {
+    // Given:
+    final String line = "some line 'this is in a quote\\\'";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(true));
+  }
+
+  @Test
+  public void shouldFindUnclosedQuote_twoQuote() {
+    // Given:
+    final String line = "some line 'this is in a quote''";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(true));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_endsQuote() {
+    // Given:
+    final String line = "some line 'this is in a quote'";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_endsNonQuote() {
+    // Given:
+    final String line = "some line 'this is in a quote' more";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_containsDoubleQuote() {
+    // Given:
+    final String line = "some line 'this is \"in\" a quote'";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_containsUnclosedDoubleQuote() {
+    // Given:
+    final String line = "some line 'this is \"in a quote'";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_escaped() {
+    // Given:
+    final String line = "some line 'this is in a quote\\''";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_twoQuote() {
+    // Given:
+    final String line = "some line 'this is in a quote'''";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldFindUnclosedQuote_manyQuote() {
+    // Given:
+    final String line = "some line 'this is in a quote''''";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(true));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_manyQuote() {
+    // Given:
+    final String line = "some line 'this is in a quote'''''";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_escapedAndMultipleQuotes() {
+    // Given:
+    final String line = "some line 'this is in a quote\\''''";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_inComment() {
+    // Given:
+    final String line = "some line -- 'this is in a comment";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_onlyComment() {
+    // Given:
+    final String line = "some line -- this is a comment";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+
+  @Test
+  public void shouldNotFindUnclosedQuote_commentAfterQuote() {
+    // Given:
+    final String line = "some line 'quoted text' -- this is a comment";
+
+    // Then:
+    assertThat(UnclosedQuoteChecker.isUnclosedQuote(line), is(false));
+  }
+}

ksqldb-cli/src/test/java/io/confluent/ksql/util/CmdLineUtilTest.java

@@ -108,6 +108,14 @@ public void shouldCheckSplitJavaDocExamplesAreRight() {
     assertSplit("'escaped '' quote'", "'escaped '' quote'");
   }
 
+  @Test
+  public void shouldCheckSplitIgnoreCommentedQuote() {
+    assertSplit("-- '\nSHOW TABLES;", "-- '", "SHOW", "TABLES;");
+
+    assertSplit("COMMAND -- '\nSHOW TABLES;", "COMMAND", "-- '", "SHOW", "TABLES;");
+    assertSplit("COMMAND-- '\nSHOW TABLES;", "COMMAND-- '", "SHOW", "TABLES;");
+  }
+
   @Test
   public void shouldRemoveQuotesFromUnQuotedString() {
     assertMatchedQuotesRemoved(" some input ", " some input ");