Fix missing columns to be a 400 error (elastic/x-pack-elasticsearch#2123)

1. We talked about doing 422 but that doesn't work because the netty plugin
maps 422 -> 400 and I didn't think it was worth changing that right now.
2. Missing columns in the `WHERE` clause still cause a 500 response because
we don't resolve the `SELECT` part if there is an error in the `WHERE`
clause.

Original commit: elastic/x-pack-elasticsearch@355d8292e5

sql/server/src/main/java/org/elasticsearch/xpack/sql/SqlException.java

@@ -5,16 +5,13 @@
  */
 package org.elasticsearch.xpack.sql;
 
+import org.elasticsearch.ElasticsearchException;
+
 import java.util.Locale;
 
 import static java.lang.String.format;
 
-public class SqlException extends RuntimeException {
+public class SqlException extends ElasticsearchException {
-
-    public SqlException() {
-        super();
-    }
-
     public SqlException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
         super(message, cause, enableSuppression, writableStackTrace);
     }

sql/server/src/main/java/org/elasticsearch/xpack/sql/SqlIllegalArgumentException.java

@@ -6,11 +6,6 @@
 package org.elasticsearch.xpack.sql;
 
 public class SqlIllegalArgumentException extends SqlException {
-
-    public SqlIllegalArgumentException() {
-        super();
-    }
-
     public SqlIllegalArgumentException(String message, Throwable cause, boolean enableSuppression, boolean writableStackTrace) {
         super(message, cause, enableSuppression, writableStackTrace);
     }

sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java

@@ -7,6 +7,7 @@ package org.elasticsearch.xpack.sql.analysis;
 
 import java.util.Locale;
 
+import org.elasticsearch.rest.RestStatus;
 import org.elasticsearch.xpack.sql.SqlException;
 import org.elasticsearch.xpack.sql.tree.Location;
 import org.elasticsearch.xpack.sql.tree.Node;
@@ -37,6 +38,11 @@ public class AnalysisException extends SqlException {
         return column;
     }
 
+    @Override
+    public RestStatus status() {
+        return RestStatus.BAD_REQUEST;
+    }
+
     @Override
     public String getMessage() {
         return format(Locale.ROOT, "line %s:%s: %s", getLineNumber(), getColumnNumber(), super.getMessage());

sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Verifier.java

@@ -108,7 +108,7 @@ abstract class Verifier {
                 }
             }));
 
-            // consider only nodes that are by themselves unresolved (to avoid unresolved dependees)
+            // consider only nodes that are by themselves unresolved (to avoid unresolved dependencies)
             if (p.childrenResolved() && p.expressionsResolved() && !p.resolved()) {
                 localFailures.add(fail(p, "Unresolved item '%s'", p.nodeString()));
             }

sql/server/src/main/java/org/elasticsearch/xpack/sql/capabilities/UnresolvedException.java

@@ -11,8 +11,11 @@ import org.elasticsearch.xpack.sql.SqlException;
 
 import static java.lang.String.format;
 
+/**
+ * Thrown when we accidentally attempt to resolve something on on an unresolved entity. Throwing this
+ * is always a bug.
+ */
 public class UnresolvedException extends SqlException {
-
     public UnresolvedException(String action, Object target) {
         super(format(Locale.ROOT, "Invalid call to %s on an unresolved object %s", action, target));
     }

sql/server/src/main/java/org/elasticsearch/xpack/sql/rule/RuleExecutor.java

@@ -34,6 +34,8 @@ public abstract class RuleExecutor<TreeType extends Node<TreeType>> {
         }
 
         private boolean reached(int runs, long duration) {
+            // NOCOMMIT we should probably be throwing exceptions rather than stopping
+            // NOCOMMIT including time here seems like it'd cause very unpredictable behavior
             return runs >= this.runs || duration >= this.duration;
         }
     }

sql/server/src/main/java/org/elasticsearch/xpack/sql/util/Assert.java

@@ -8,6 +8,7 @@ package org.elasticsearch.xpack.sql.util;
 import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 
 public abstract class Assert {
+    // NOCOMMIT we should investigate using java assertions like the rest of the code base
 
     public static void isTrue(boolean expression, String message, Object... values) {
         if (!expression) {

sql/server/src/test/java/org/elasticsearch/xpack/sql/server/RestSqlIT.java

@@ -24,6 +24,7 @@ import java.util.Map;
 
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
+import static org.hamcrest.Matchers.both;
 import static org.hamcrest.Matchers.containsString;
 
 public class RestSqlIT extends ESRestTestCase {
@@ -70,20 +71,25 @@ public class RestSqlIT extends ESRestTestCase {
         client().performRequest("POST", "/test/test/_bulk", singletonMap("refresh", "true"),
                 new StringEntity(bulk.toString(), ContentType.APPLICATION_JSON));
 
-        // NOCOMMIT test the error messages
+        // NOCOMMIT "unresolved" should probably be changed to something users will understand like "missing"
-        expectSqlThrows(() -> runSql("SELECT foo FROM test.test"), containsString("500"));
+        expectBadRequest(() -> runSql("SELECT foo FROM test.test"), containsString("1:8: Unresolved item 'foo'"));
-        expectSqlThrows(() -> runSql("SELECT DAY_OF_YEAR(foo) FROM test.test"), containsString("500"));
+        // NOCOMMIT the ones below one should include (foo) but it looks like the function is missing
-        expectSqlThrows(() -> runSql("SELECT foo, * FROM test.test GROUP BY DAY_OF_YEAR(foo)"), containsString("500"));
+        expectBadRequest(() -> runSql("SELECT DAY_OF_YEAR(foo) FROM test.test"), containsString("1:20: Unresolved item 'DAY_OF_YEAR'"));
-        expectSqlThrows(() -> runSql("SELECT * FROM test.test WHERE foo = 1"), containsString("500"));
+        expectBadRequest(() -> runSql("SELECT foo, * FROM test.test GROUP BY DAY_OF_YEAR(foo)"),
-        expectSqlThrows(() -> runSql("SELECT * FROM test.test WHERE DAY_OF_YEAR(foo) = 1"), containsString("500"));
+                both(containsString("1:8: Unresolved item 'foo'"))
-        expectSqlThrows(() -> runSql("SELECT * FROM test.test ORDER BY foo"), containsString("500"));
+                    .and(containsString("1:51: Unresolved item 'DAY_OF_YEAR'")));
-        expectSqlThrows(() -> runSql("SELECT * FROM test.test ORDER BY DAY_OF_YEAR(foo)"), containsString("500"));
+        // NOCOMMIT broken because we bail on the resolution phase if we can't resolve something in a previous phase
+//        expectBadRequest(() -> runSql("SELECT * FROM test.test WHERE foo = 1"), containsString("500"));
+//        expectBadRequest(() -> runSql("SELECT * FROM test.test WHERE DAY_OF_YEAR(foo) = 1"), containsString("500"));
+        // NOCOMMIT this should point to the column, no the (incorrectly capitalized) start or ORDER BY
+        expectBadRequest(() -> runSql("SELECT * FROM test.test ORDER BY foo"), containsString("line 1:34: Unresolved item 'Order'"));
+        expectBadRequest(() -> runSql("SELECT * FROM test.test ORDER BY DAY_OF_YEAR(foo)"),
+                containsString("line 1:46: Unresolved item 'Order'"));
     }
 
-    private void expectSqlThrows(ThrowingRunnable code, Matcher<String> errorMessageMatcher) {
+    private void expectBadRequest(ThrowingRunnable code, Matcher<String> errorMessageMatcher) {
         ResponseException e = expectThrows(ResponseException.class, code);
-        assertThat(e.getMessage(), containsString("500"));
+        assertEquals(400, e.getResponse().getStatusLine().getStatusCode());
-        // NOCOMMIT This should return a 400 or 422
         assertThat(e.getMessage(), errorMessageMatcher);
     }