From f5af60c7cfbbf42210ea89edd1b7ee017e818b74 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 14 Dec 2017 09:56:36 -0500 Subject: [PATCH] SQL: Fix error message on bad index (elastic/x-pack-elasticsearch#3312) Fixes the error message that SQL produces when it sees unsupported indexes. It was always returning all broken indexes as "unknown" even though we have much better error messages. It was just throwing them away. I caught this originally when backporting to 6.x where we had a test that we produced a useful error message when the user attempted to run SQL on an index with more than one type. We couldn't run that in the feature/sql branch because it is inside the full cluster restart tests and the "old" version of Elasticsearch used in those tests in feature/sql is 6.x which doesn't allow indexes with multiple types. When I backported to 6.x the test failed because it hadn't been run before. In addition to fixing that test and the problem, this adds another test that will reveal the problem when run in the feature/sql and master branch. Original commit: elastic/x-pack-elasticsearch@c7b787baeef1abce832383c36a2c4b8c00ef350a --- .../xpack/restart/FullClusterRestartIT.java | 3 +- .../xpack/qa/sql/ErrorsTestCase.java | 1 + .../xpack/qa/sql/cli/ErrorsTestCase.java | 13 ++++++ .../xpack/qa/sql/jdbc/ErrorsTestCase.java | 15 +++++++ .../xpack/qa/sql/rest/RestSqlTestCase.java | 10 +++++ .../sql/analysis/index/GetIndexResult.java | 2 +- .../sql/plan/logical/UnresolvedRelation.java | 13 +++--- .../plan/logical/UnresolvedRelationTests.java | 40 +++++++++++++++++++ 8 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 sql/server/src/test/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelationTests.java diff --git a/qa/full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java b/qa/full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java index 23c81d13fc5..7aa6ce89777 100644 --- a/qa/full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java +++ b/qa/full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java @@ -290,7 +290,8 @@ public class FullClusterRestartIT extends ESRestTestCase { ResponseException e = expectThrows(ResponseException.class, () -> client().performRequest("POST", "/_xpack/sql", emptyMap(), new StringEntity("{\"query\":\"SELECT * FROM testsqlfailsonindexwithtwotypes\"}", ContentType.APPLICATION_JSON))); assertEquals(400, e.getResponse().getStatusLine().getStatusCode()); - assertThat(e.getMessage(), containsString("Invalid index testsqlfailsonindexwithtwotypes; contains more than one type")); + assertThat(e.getMessage(), containsString( + "[testsqlfailsonindexwithtwotypes] contains more than one type [type1, type2] so it is incompatible with sql")); } private String loadWatch(String watch) throws IOException { diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/ErrorsTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/ErrorsTestCase.java index 8c31b7a6445..a08d2682b3c 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/ErrorsTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/ErrorsTestCase.java @@ -13,6 +13,7 @@ package org.elasticsearch.xpack.qa.sql; public interface ErrorsTestCase { void testSelectInvalidSql() throws Exception; void testSelectFromMissingIndex() throws Exception; + void testSelectFromIndexWithoutTypes() throws Exception; void testSelectMissingField() throws Exception; void testSelectMissingFunction() throws Exception; } diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java index f25bca75b75..3adc49ff0ff 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/cli/ErrorsTestCase.java @@ -6,6 +6,10 @@ package org.elasticsearch.xpack.qa.sql.cli; import java.io.IOException; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; + +import static java.util.Collections.emptyMap; /** * Tests for error messages. @@ -23,6 +27,15 @@ public abstract class ErrorsTestCase extends CliIntegrationTestCase implements o assertEquals("line 1:15: Unknown index [test][1;23;31m][0m", readLine()); } + @Override + public void testSelectFromIndexWithoutTypes() throws Exception { + // Create an index without any types + client().performRequest("PUT", "/test", emptyMap(), new StringEntity("{}", ContentType.APPLICATION_JSON)); + + assertEquals("[1;31mBad request [[22;3;33mFound 1 problem(s)", command("SELECT * FROM test")); + assertEquals("line 1:15: [test] doesn't have any types so it is incompatible with sql[1;23;31m][0m", readLine()); + } + @Override public void testSelectMissingField() throws IOException { index("test", body -> body.field("test", "test")); diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ErrorsTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ErrorsTestCase.java index 7e995e69363..8729a4a50bb 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ErrorsTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/jdbc/ErrorsTestCase.java @@ -7,6 +7,10 @@ package org.elasticsearch.xpack.qa.sql.jdbc; import java.sql.Connection; import java.sql.SQLException; +import org.apache.http.entity.ContentType; +import org.apache.http.entity.StringEntity; + +import static java.util.Collections.emptyMap; /** * Tests for exceptions and their messages. @@ -28,6 +32,17 @@ public class ErrorsTestCase extends JdbcIntegrationTestCase implements org.elast } } + @Override + public void testSelectFromIndexWithoutTypes() throws Exception { + // Create an index without any types + client().performRequest("PUT", "/test", emptyMap(), new StringEntity("{}", ContentType.APPLICATION_JSON)); + + try (Connection c = esJdbc()) { + SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * FROM test").executeQuery()); + assertEquals("Found 1 problem(s)\nline 1:15: [test] doesn't have any types so it is incompatible with sql", e.getMessage()); + } + } + @Override public void testSelectMissingField() throws Exception { index("test", body -> body.field("test", "test")); diff --git a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java index 5f390b0d73e..3f299c372cd 100644 --- a/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java +++ b/qa/sql/src/main/java/org/elasticsearch/xpack/qa/sql/rest/RestSqlTestCase.java @@ -30,6 +30,7 @@ import java.util.Map; import java.util.TreeMap; import static java.util.Collections.emptyList; +import static java.util.Collections.emptyMap; import static java.util.Collections.singletonList; import static java.util.Collections.singletonMap; import static java.util.Collections.unmodifiableMap; @@ -137,6 +138,15 @@ public abstract class RestSqlTestCase extends ESRestTestCase implements ErrorsTe expectBadRequest(() -> runSql("SELECT * FROM missing"), containsString("1:15: Unknown index [missing]")); } + @Override + public void testSelectFromIndexWithoutTypes() throws Exception { + // Create an index without any types + client().performRequest("PUT", "/test", emptyMap(), new StringEntity("{}", ContentType.APPLICATION_JSON)); + + expectBadRequest(() -> runSql("SELECT * FROM test"), + containsString("1:15: [test] doesn't have any types so it is incompatible with sql")); + } + @Override public void testSelectMissingField() throws IOException { StringBuilder bulk = new StringBuilder(); diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/GetIndexResult.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/GetIndexResult.java index 5b84030a34f..743e587a83a 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/GetIndexResult.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/index/GetIndexResult.java @@ -20,7 +20,7 @@ public final class GetIndexResult { } public static GetIndexResult notFound(String name) { Objects.requireNonNull(name, "name must not be null"); - return invalid("Index '" + name + "' does not exist"); + return invalid("Unknown index [" + name + "]"); } private final EsIndex index; diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelation.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelation.java index eacd5475821..f748bd41354 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelation.java +++ b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelation.java @@ -61,7 +61,7 @@ public class UnresolvedRelation extends LeafPlan implements Unresolvable { @Override public int hashCode() { - return Objects.hash(table); + return Objects.hash(location(), table, alias, unresolvedMsg); } @Override @@ -69,12 +69,15 @@ public class UnresolvedRelation extends LeafPlan implements Unresolvable { if (this == obj) { return true; } - + if (obj == null || getClass() != obj.getClass()) { return false; } - + UnresolvedRelation other = (UnresolvedRelation) obj; - return Objects.equals(table, other.table); + return location().equals(other.location()) + && table.equals(other.table) + && Objects.equals(alias, other.alias) + && unresolvedMsg.equals(other.unresolvedMsg); } -} \ No newline at end of file +} diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelationTests.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelationTests.java new file mode 100644 index 00000000000..402a35f7dd6 --- /dev/null +++ b/sql/server/src/test/java/org/elasticsearch/xpack/sql/plan/logical/UnresolvedRelationTests.java @@ -0,0 +1,40 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.sql.plan.logical; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.sql.plan.TableIdentifier; +import org.elasticsearch.xpack.sql.tree.Location; +import java.util.ArrayList; +import java.util.List; +import java.util.function.Function; + +import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; + +public class UnresolvedRelationTests extends ESTestCase { + public void testEqualsAndHashCode() { + Location location = new Location(between(1, 1000), between(1, 1000)); + TableIdentifier table = new TableIdentifier(location, randomAlphaOfLength(5)); + String alias = randomBoolean() ? null : randomAlphaOfLength(5); + String unresolvedMessage = randomAlphaOfLength(5); + UnresolvedRelation relation = new UnresolvedRelation(location, table, alias, unresolvedMessage); + List> mutators = new ArrayList<>(); + mutators.add(r -> new UnresolvedRelation( + new Location(r.location().getLineNumber() + 1, r.location().getColumnNumber()), r.table(), r.alias(), r.unresolvedMessage())); + mutators.add(r -> new UnresolvedRelation( + r.location(), new TableIdentifier(r.location(), r.table().index() + "m"), r.alias(), r.unresolvedMessage())); + mutators.add(r -> new UnresolvedRelation( + r.location(), + r.table(), + randomValueOtherThan(r.alias(), () -> randomBoolean() ? null : randomAlphaOfLength(5)), + r.unresolvedMessage())); + mutators.add(r -> new UnresolvedRelation( + r.location(), r.table(), r.alias(), randomValueOtherThan(r.unresolvedMessage(), () -> randomAlphaOfLength(5)))); + checkEqualsAndHashCode(relation, + r -> new UnresolvedRelation(r.location(), r.table(), r.alias(), r.unresolvedMessage()), + r -> randomFrom(mutators).apply(r)); + } +}