From b31bc786d3d1ec17388024ffe0a20009e30e03ec Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 8 Aug 2017 15:42:47 -0400 Subject: [PATCH] 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@85cd089e4710cc11b4f9174b5f8570e5adb1a0a0 --- .../xpack/sql/analysis/AnalysisException.java | 11 +++++++++++ .../sql/analysis/InvalidIndexException.java | 15 --------------- .../xpack/sql/analysis/analyzer/Analyzer.java | 10 ++++++---- .../xpack/sql/analysis/catalog/Catalog.java | 10 +++------- .../xpack/sql/analysis/catalog/EsCatalog.java | 18 ++++++++---------- .../sql/analysis/catalog/FilteredCatalog.java | 5 ----- .../sql/plan/logical/command/ShowColumns.java | 8 +++++++- .../sql/analysis/catalog/InMemoryCatalog.java | 6 ------ 8 files changed, 35 insertions(+), 48 deletions(-) delete mode 100644 sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/InvalidIndexException.java diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java index 2eea190b682..dab5134114b 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/AnalysisException.java +++ b/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; } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/InvalidIndexException.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/InvalidIndexException.java deleted file mode 100644 index eab23307595..00000000000 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/InvalidIndexException.java +++ /dev/null @@ -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); - } -} diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java index 41d7b700d32..331ea514098 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/analyzer/Analyzer.java +++ b/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 { @Override protected LogicalPlan rule(UnresolvedRelation plan) { TableIdentifier table = plan.table(); - if (!catalog.indexIsValid(table.index())) { - throw new InvalidIndexException(table.index(), plan); + EsIndex found; + 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); } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java index 6d628a73544..793d2c5b72b 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/Catalog.java +++ b/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 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. } diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsCatalog.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsCatalog.java index 7bc854590bf..a407f5c6186 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/EsCatalog.java +++ b/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)); - } - - @Override - 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); + if (false == indexHasOnlyOneType(idx)) { + throw new SqlIllegalArgumentException("index has more than one type"); + } + return EsIndex.build(idx); } @Override diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/FilteredCatalog.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/FilteredCatalog.java index 7b8c4623d39..aab8b1bc5ec 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/analysis/catalog/FilteredCatalog.java +++ b/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. diff --git a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/ShowColumns.java b/sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/ShowColumns.java index 90d278cec7e..fdd5853bfe5 100644 --- a/sql/server/src/main/java/org/elasticsearch/xpack/sql/plan/logical/command/ShowColumns.java +++ b/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> 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); } diff --git a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java b/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java index 0f913b2854e..e75721d3706 100644 --- a/sql/server/src/test/java/org/elasticsearch/xpack/sql/analysis/catalog/InMemoryCatalog.java +++ b/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); } - } \ No newline at end of file