Remove isValidIndex from SQL's Catalog interface (elastic/x-pack-elasticsearch#2195)

This moves validating that each index contains only a single type
into EsCatalog which removes a race condition and removes a method
from the Catalog interface. Removing methods from the Catalog
interface is nice because the fewer methods that the interface has
the fewer have to be secured.

Original commit: elastic/x-pack-elasticsearch@85cd089e47

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

@@ -30,6 +30,17 @@ public class AnalysisException extends SqlException {
         this.column = loc.getColumnNumber();
     }
 
+    public AnalysisException(Node<?> source, String message, Throwable cause) {
+        super(message, cause);
+
+        Location loc = Location.EMPTY;
+        if (source != null && source.location() != null) {
+            loc = source.location();
+        }
+        this.line = loc.getLineNumber();
+        this.column = loc.getColumnNumber();
+    }
+
     public int getLineNumber() {
         return line;
     }

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

@@ -1,15 +0,0 @@
-/*
- * 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.analysis;
-
-import org.elasticsearch.xpack.sql.tree.Node;
-
-public class InvalidIndexException extends AnalysisException {
-
-    public InvalidIndexException(String index, Node<?> source) {
-        super(source, "Invalid index %s; contains more than one type", index);
-    }
-}

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

@@ -5,8 +5,8 @@
  */
 package org.elasticsearch.xpack.sql.analysis.analyzer;
 
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.analysis.AnalysisException;
-import org.elasticsearch.xpack.sql.analysis.InvalidIndexException;
 import org.elasticsearch.xpack.sql.analysis.UnknownFunctionException;
 import org.elasticsearch.xpack.sql.analysis.UnknownIndexException;
 import org.elasticsearch.xpack.sql.analysis.analyzer.Verifier.Failure;
@@ -241,10 +241,12 @@ public class Analyzer extends RuleExecutor<LogicalPlan> {
         @Override
         protected LogicalPlan rule(UnresolvedRelation plan) {
             TableIdentifier table = plan.table();
-            if (!catalog.indexIsValid(table.index())) {
+            EsIndex found;
-                throw new InvalidIndexException(table.index(), plan);
+            try {
+                found = catalog.getIndex(table.index());
+            } catch (SqlIllegalArgumentException e) {
+                throw new AnalysisException(plan, e.getMessage(), e);
             }
-            EsIndex found = catalog.getIndex(table.index());
             if (found == null) {
                 throw new UnknownIndexException(table.index(), plan);
             }

sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java

@@ -6,23 +6,19 @@
 package org.elasticsearch.xpack.sql.analysis.catalog;
 
 import org.elasticsearch.common.Nullable;
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 
 import java.util.List;
 
 
 public interface Catalog {
-    /**
-     * Check if an index is valid for sql.
-     */
-    boolean indexIsValid(String index); // NOCOMMIT should probably be merged into EsCatalog's getIndex method.
-
     /**
      * Lookup the information for a table, returning {@code null} if
      * the index is not found.
+     * @throws SqlIllegalArgumentException if the index is in some way invalid for use with sql
      */
     @Nullable
-    EsIndex getIndex(String index);
+    EsIndex getIndex(String index) throws SqlIllegalArgumentException;
 
     List<EsIndex> listIndices(String pattern);
-    // NOCOMMIT should these be renamed to getTable and listTables? That seems like a name given that this is a SQL Catalog abstraction. 
 }

sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsCatalog.java

@@ -11,6 +11,7 @@ import org.elasticsearch.cluster.metadata.IndexMetaData;
 import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
 import org.elasticsearch.cluster.metadata.MetaData;
 import org.elasticsearch.common.inject.Inject;
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 
 import java.util.ArrayList;
 import java.util.Iterator;
@@ -36,19 +37,16 @@ public class EsCatalog implements Catalog {
     }
 
     @Override
-    public EsIndex getIndex(String index) {
+    public EsIndex getIndex(String index) throws SqlIllegalArgumentException {
         MetaData metadata = metadata();
-        if (false == metadata.hasIndex(index)) {
+        IndexMetaData idx = metadata.index(index);
+        if (idx == null) {
             return null;
         }
-        return EsIndex.build(metadata.index(index));
+        if (false == indexHasOnlyOneType(idx)) {
-    }
+            throw new SqlIllegalArgumentException("index has more than one type");
-
+        }
-    @Override
+        return EsIndex.build(idx);
-    public boolean indexIsValid(String index) {
-        // NOCOMMIT there is a race condition here with indices being deleted. This should be moved into getIndex
-        IndexMetaData idx = metadata().index(index);
-        return idx == null || indexHasOnlyOneType(idx);
     }
 
     @Override

sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/FilteredCatalog.java

@@ -34,11 +34,6 @@ public class FilteredCatalog implements Catalog {
         return delegate.listIndices(pattern);
     }
 
-    @Override
-    public boolean indexIsValid(String index) {
-        return delegate.indexIsValid(index);
-    }
-
     @Override
     public EsIndex getIndex(String index) {
         // NOCOMMIT we need to think really carefully about how we deal with aliases that resolve into multiple indices.

sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/ShowColumns.java

@@ -5,6 +5,7 @@
  */
 package org.elasticsearch.xpack.sql.plan.logical.command;
 
+import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
 import org.elasticsearch.xpack.sql.analysis.catalog.Catalog;
 import org.elasticsearch.xpack.sql.analysis.catalog.EsIndex;
 import org.elasticsearch.xpack.sql.expression.Attribute;
@@ -48,7 +49,12 @@ public class ShowColumns extends Command {
     protected RowSetCursor execute(SqlSession session) {
         Catalog catalog = session.catalog();
         List<List<?>> rows = new ArrayList<>();
-        EsIndex fetched = catalog.getIndex(index);
+        EsIndex fetched;
+        try {
+            fetched = catalog.getIndex(index);
+        } catch (SqlIllegalArgumentException e) {
+            throw new IllegalArgumentException(e);
+        }
         if (fetched != null) {
             fillInRows(fetched.mapping(), null, rows);
         }

sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java

@@ -34,14 +34,8 @@ class InMemoryCatalog implements Catalog {
             .collect(Collectors.toList());
     }
 
-    @Override
-    public boolean indexIsValid(String index) {
-        return true;
-    }
-
     @Override
     public EsIndex getIndex(String index) {
         return indices.get(index);
     }
-
 }