bug: Improved error message for reserved keywords (#6166)

* adds support for 0x, X'', x'' type hex in udf:encode

* added one line of comment

* added some more tests for odd length hex and corrupted hex

* remove input.json from pr

* changed error message

* added tests for non-reserved keywords

* added tests for non-reserved keywords 2

* added tests for non-reserved keywords 3

* removed files from commit

* undeleted files'
'

* added comments

* added comments 2

* added comments 3

* fix: changed the error message

* removed hardcoding of keywords and added tests

* refactored code

* not a good time to mess up git

* cleaned up imports

* cleaning up

* cleaned up imports more

* changed javadoc for ParserKeywordValidatorUtil.java

* removed useless import

* changed comment to contrapositive in ParserUtil.java

* added better error message

* should build

* should build1

ksqldb-engine/src/main/java/io/confluent/ksql/function/InternalFunctionRegistry.java

@@ -24,6 +24,7 @@
 import io.confluent.ksql.name.FunctionName;
 import io.confluent.ksql.schema.ksql.types.SqlType;
 import io.confluent.ksql.util.KsqlException;
+import io.confluent.ksql.util.ParserKeywordValidatorUtil;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -38,7 +39,7 @@ public class InternalFunctionRegistry implements MutableFunctionRegistry {
   private final Map<String, UdfFactory> udfs = new HashMap<>();
   private final Map<String, AggregateFunctionFactory> udafs = new HashMap<>();
   private final Map<String, TableFunctionFactory> udtfs = new HashMap<>();
-  private final FunctionNameValidator functionNameValidator = new FunctionNameValidator();
+  private final ParserKeywordValidatorUtil functionNameValidator = new ParserKeywordValidatorUtil();
 
   public InternalFunctionRegistry() {
     new BuiltInInitializer(this).init();

ksqldb-parser/src/main/java/io/confluent/ksql/parser/DefaultKsqlParser.java

@@ -19,8 +19,10 @@
 import io.confluent.ksql.parser.SqlBaseParser.SingleStatementContext;
 import io.confluent.ksql.parser.exception.ParseFailedException;
 import io.confluent.ksql.parser.tree.Statement;
+import io.confluent.ksql.util.ParserUtil;
 import java.util.List;
 import java.util.function.Function;
+import java.util.regex.Matcher;
 import java.util.stream.Collectors;
 import org.antlr.v4.runtime.BaseErrorListener;
 import org.antlr.v4.runtime.CharStream;
@@ -29,6 +31,7 @@
 import org.antlr.v4.runtime.ParserRuleContext;
 import org.antlr.v4.runtime.RecognitionException;
 import org.antlr.v4.runtime.Recognizer;
+import org.antlr.v4.runtime.Token;
 import org.antlr.v4.runtime.atn.PredictionMode;
 import org.antlr.v4.runtime.misc.Interval;
 import org.antlr.v4.runtime.misc.ParseCancellationException;
@@ -47,7 +50,17 @@ public void syntaxError(
         final String message,
         final RecognitionException e
     ) {
-      throw new ParsingException(message, e, line, charPositionInLine);
+      if (offendingSymbol instanceof Token && isKeywordError(
+              message, ((Token) offendingSymbol).getText())) {
+        //Checks if the error is a reserved keyword error
+        final String tokenName = ((Token) offendingSymbol).getText();
+        final String newMessage =
+                "\"" + tokenName + "\" is a reserved keyword and it can't be used as an identifier."
+                + " You can use it as an identifier by escaping it as \'" + tokenName + "\' ";
+        throw new ParsingException(newMessage, e, line, charPositionInLine);
+      } else {
+        throw new ParsingException(message, e, line, charPositionInLine);
+      }
     }
   };
 
@@ -130,4 +143,16 @@ private static String getStatementString(final SingleStatementContext singleStat
         singleStatementContext.stop.getStopIndex()
     ));
   }
+
+  /**
+   * checks if the error is a reserved keyword error by checking the message and offendingSymbol
+   * @param message the error message
+   * @param offendingSymbol the symbol that caused the error
+   * @return
+   */
+  private static boolean isKeywordError(final String message, final String offendingSymbol) {
+    final Matcher m = ParserUtil.EXTRANEOUS_INPUT_PATTERN.matcher(message);
+
+    return  m.find() && ParserUtil.isReserved(offendingSymbol);
+  }
 }

ksqldb-engine/src/main/java/io/confluent/ksql/function/FunctionNameValidator.java → ksqldb-parser/src/main/java/io/confluent/ksql/util/ParserKeywordValidatorUtil.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2018 Confluent Inc.
+ * 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
@@ -13,7 +13,7 @@
  * specific language governing permissions and limitations under the License.
  */
 
-package io.confluent.ksql.function;
+package io.confluent.ksql.util;
 
 import com.google.common.collect.ImmutableSet;
 import io.confluent.ksql.parser.SqlBaseParser;
@@ -24,11 +24,14 @@
 import org.antlr.v4.runtime.Vocabulary;
 
 /**
- * Check that a function name is valid. It is valid if it is not a Java reserved word
- * and not a ksql reserved word and is a valid java identifier.
+ * This Class provides methods for checking whether a function name is valid.
+ * It is valid if it is not a Java reserved word and not a ksql reserved word
+ * and is a valid java identifier.
+ * It also provides utils for checking if keywords are reserved or not as
+ * defined in SqlBase.g4
  */
 @ThreadSafe
-class FunctionNameValidator implements Predicate<String> {
+public class ParserKeywordValidatorUtil implements Predicate<String> {
   private static final Set<String> JAVA_RESERVED_WORDS
       = ImmutableSet.<String>builder()
       .add("abstract").add("assert").add("boolean").add("break").add("byte").add("case")
@@ -90,4 +93,8 @@ private static Set<String> createFromVocabulary() {
     }
     return builder.build();
   }
+
+  public static Set<String> getKsqlReservedWords() {
+    return KSQL_RESERVED_WORDS;
+  }
 }

ksqldb-parser/src/main/java/io/confluent/ksql/util/ParserUtil.java

@@ -24,8 +24,10 @@
 import io.confluent.ksql.execution.expression.tree.Literal;
 import io.confluent.ksql.execution.expression.tree.LongLiteral;
 import io.confluent.ksql.name.SourceName;
+import io.confluent.ksql.parser.CaseInsensitiveStream;
 import io.confluent.ksql.parser.NodeLocation;
 import io.confluent.ksql.parser.ParsingException;
+import io.confluent.ksql.parser.SqlBaseLexer;
 import io.confluent.ksql.parser.SqlBaseParser;
 import io.confluent.ksql.parser.SqlBaseParser.FloatLiteralContext;
 import io.confluent.ksql.parser.SqlBaseParser.IntegerLiteralContext;
@@ -37,13 +39,17 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.regex.Pattern;
+import org.antlr.v4.runtime.CharStreams;
+import org.antlr.v4.runtime.CommonTokenStream;
 import org.antlr.v4.runtime.ParserRuleContext;
 import org.antlr.v4.runtime.Token;
 import org.antlr.v4.runtime.tree.TerminalNode;
 
+// CHECKSTYLE_RULES.OFF: ClassDataAbstractionCoupling
 public final class ParserUtil {
-
+  // CHECKSTYLE_RULES.ON: ClassDataAbstractionCoupling
   /**
    * Source names must adhere to the kafka topic naming convention. We restrict
    * it here instead of as a parser rule to allow for a more descriptive error
@@ -52,6 +58,8 @@ public final class ParserUtil {
    * @see org.apache.kafka.streams.state.StoreBuilder#name
    */
   private static final Pattern VALID_SOURCE_NAMES = Pattern.compile("[a-zA-Z0-9_-]*");
+  public static final Pattern EXTRANEOUS_INPUT_PATTERN = Pattern.compile(
+          "extraneous input.*expecting.*");
 
   private ParserUtil() {
   }
@@ -212,4 +220,32 @@ public static JsonObject convertJsonFieldCase(final JsonObject obj) {
     }
     return new JsonObject(convertedMap);
   }
+
+  /**
+   * Checks if the token is a reserved keyword or not
+   * @param token the String that caused the parsing error
+   * @return true if the token is a reserved keyword according to SqlBase.g4
+   *         false otherwise
+   */
+  public static boolean isReserved(final String token) {
+
+    final SqlBaseLexer sqlBaseLexer = new SqlBaseLexer(
+            new CaseInsensitiveStream(CharStreams.fromString(token)));
+    final CommonTokenStream tokenStream = new CommonTokenStream(sqlBaseLexer);
+    final SqlBaseParser sqlBaseParser = new SqlBaseParser(tokenStream);
+
+    sqlBaseParser.removeErrorListeners();
+
+    final SqlBaseParser.NonReservedContext nonReservedContext = sqlBaseParser.nonReserved();
+    if (nonReservedContext.exception == null) {
+      // If we call nonReservedWord, and if it successfully parses,
+      // then we just parsed through a nonReserved word as defined in SqlBase.g4
+      // and we return false
+      return false;
+    }
+
+    final Set<String> allVocab = ParserKeywordValidatorUtil.getKsqlReservedWords();
+
+    return allVocab.contains(token.toLowerCase());
+  }
 }

ksqldb-parser/src/test/java/io/confluent/ksql/parser/KsqlParserTest.java

@@ -1128,6 +1128,50 @@ public void testSelectWithMissingComma() {
     assertThat(e.getMessage(), containsString("line 1:12: extraneous input 'C' expecting"));
   }
 
+  @Test
+  public void testReservedKeywordSyntaxError() {
+    // Given:
+    final String simpleQuery = "CREATE STREAM ORDERS (c1 VARCHAR, size INTEGER)\n" +
+            "  WITH (kafka_topic='ords', value_format='json');";
+
+    // When:
+    final Exception e = assertThrows(
+            ParseFailedException.class,
+            () -> KsqlParserTestUtil.buildSingleAst(simpleQuery, metaStore)
+    );
+
+    // Then:
+    assertThat(e.getMessage(), containsString("\"size\" is a reserved keyword and it can't be used as an identifier"));
+  }
+
+  @Test
+  public void testReservedKeywordSyntaxErrorCaseInsensitive() {
+    // Given:
+    final String simpleQuery = "CREATE STREAM ORDERS (Load VARCHAR, size INTEGER)\n" +
+            "  WITH (kafka_topic='ords', value_format='json');";
+
+    // When:
+    final Exception e = assertThrows(
+            ParseFailedException.class,
+            () -> KsqlParserTestUtil.buildSingleAst(simpleQuery, metaStore)
+    );
+
+    // Then:
+    assertThat(e.getMessage(), containsString("\"Load\" is a reserved keyword and it can't be used as an identifier"));
+  }
+
+  @Test
+  public void testNonReservedKeywordShouldNotThrowException() {
+    // 'sink' is a keyword but is non-reserved. should not throw an exception
+    final CreateStream result = (CreateStream) KsqlParserTestUtil.buildSingleAst("CREATE STREAM ORDERS" +
+            " (place VARCHAR, Sink INTEGER)\n" +
+            " WITH (kafka_topic='orders_topic', value_format='json');", metaStore).getStatement();
+
+    // Then:
+    assertThat(result.getName(), equalTo(SourceName.of("ORDERS")));
+    assertThat(result.getProperties().getKafkaTopic(), equalTo("orders_topic"));
+  }
+
   @Test
   public void testSelectWithMultipleFroms() {
     // Given:

ksqldb-engine/src/test/java/io/confluent/ksql/function/FunctionNameValidatorTest.java → ksqldb-parser/src/test/java/io/confluent/ksql/util/ParserKeywordValidatorUtilTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2018 Confluent Inc.
+ * 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
@@ -13,7 +13,7 @@
  * specific language governing permissions and limitations under the License.
  */
 
-package io.confluent.ksql.function;
+package io.confluent.ksql.util;
 
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
@@ -22,9 +22,9 @@
 import org.antlr.v4.runtime.Vocabulary;
 import org.junit.Test;
 
-public class FunctionNameValidatorTest {
+public class ParserKeywordValidatorUtilTest {
 
-  private final FunctionNameValidator validator = new FunctionNameValidator();
+  private final ParserKeywordValidatorUtil validator = new ParserKeywordValidatorUtil();
 
   @Test
   public void shouldNotAllowJavaReservedWords() {

ksqldb-parser/src/test/java/io/confluent/ksql/util/ParserUtilTest.java

@@ -0,0 +1,43 @@
+package io.confluent.ksql.util;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+public class ParserUtilTest {
+
+    @Test
+    public void shouldBeReserved() {
+        // Given:
+        final String[] keywords = new String[]{
+                "size",      // reserved word
+                "load",      // reserved word
+                "SIZE",      //upper case
+                "Load"       //case insensitive
+        };
+
+        // Then:
+        for (final String keyword : keywords) {
+            assertEquals(true, ParserUtil.isReserved(keyword));
+        }
+    }
+
+    @Test
+    public void shouldNotBeReserved() {
+        // Given:
+        final String[] keywords = new String[]{
+                "source",      // non-reserved keyword
+                "sink",        // non-reserved keyword
+                "MAP",         //upper case
+                "Array",        //case insensitive
+                "ASSERT",
+                "foo",
+                "bAR"
+        };
+
+        // Then:
+        for (final String keyword : keywords) {
+            assertEquals(false, ParserUtil.isReserved(keyword));
+        }
+    }
+}