Replace exception catching with fancy returns (elastic/x-pack-elasticsearch#2454)

Instead of throwing and catching an exception for invalid
indices this returns *why* they are invalid in a convenient
object form that can be thrown as an exception when the index
is required or the entire index can be ignored when listing
indices.

Original commit: elastic/x-pack-elasticsearch@f45cbce647
This commit is contained in:
Nik Everett 2017-09-11 14:27:50 -04:00 committed by GitHub
parent 6ef174aa19
commit 94de0d8041
12 changed files with 217 additions and 128 deletions

View File

@ -11,6 +11,7 @@ import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.xpack.security.authz.AuthorizationService;
import org.elasticsearch.xpack.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.security.authz.accesscontrol.IndicesAccessControl.IndexAccessControl;
import org.elasticsearch.xpack.sql.analysis.catalog.Catalog.GetIndexResult;
import org.elasticsearch.xpack.sql.analysis.catalog.EsIndex;
import org.elasticsearch.xpack.sql.analysis.catalog.FilteredCatalog;
import org.elasticsearch.xpack.sql.type.DataType;
@ -42,21 +43,24 @@ public class SecurityCatalogFilter implements FilteredCatalog.Filter {
}
@Override
public EsIndex filterIndex(EsIndex index) {
public GetIndexResult filterIndex(GetIndexResult delegateResult) {
if (false == licenseState.isAuthAllowed()) {
/* If security is disabled the index authorization won't be available.
* It is technically possible there to be a race between licensing
* being enabled and sql requests that might cause us to fail on those
* requests but that should be rare. */
return index;
return delegateResult;
}
EsIndex index = delegateResult.get();
IndexAccessControl permissions = getAccessControlResolver()
.apply(OPTIONS, new String[] {index.name()})
.getIndexPermissions(index.name());
/* Fetching the permissions always checks if they are granted. If they aren't
* then it throws an exception. */
* then it throws an exception. This is ok even for list requests because this
* will only ever be called on indices that are authorized because of security's
* request filtering. */
if (false == permissions.getFieldPermissions().hasFieldLevelSecurity()) {
return index;
return delegateResult;
}
Map<String, DataType> filteredMapping = new HashMap<>(index.mapping().size());
for (Map.Entry<String, DataType> entry : index.mapping().entrySet()) {
@ -64,7 +68,7 @@ public class SecurityCatalogFilter implements FilteredCatalog.Filter {
filteredMapping.put(entry.getKey(), entry.getValue());
}
}
return new EsIndex(index.name(), filteredMapping, index.aliases(), index.settings());
return GetIndexResult.valid(new EsIndex(index.name(), filteredMapping, index.aliases(), index.settings()));
}
private BiFunction<IndicesOptions, String[], IndicesAccessControl> getAccessControlResolver() {

View File

@ -124,7 +124,7 @@ public abstract class RestSqlTestCase extends ESRestTestCase {
}
public void testMissingIndex() throws IOException {
expectBadRequest(() -> runSql("SELECT foo FROM missing"), containsString("1:17: Cannot resolve index [missing]"));
expectBadRequest(() -> runSql("SELECT foo FROM missing"), containsString("1:17: index [missing] does not exist"));
}
public void testMissingField() throws IOException {

View File

@ -17,7 +17,7 @@ public class ErrorsIT extends JdbcIntegrationTestCase {
public void testSelectFromMissingTable() throws Exception {
try (Connection c = esJdbc()) {
SQLException e = expectThrows(SQLException.class, () -> c.prepareStatement("SELECT * from test").executeQuery());
assertEquals("line 1:15: Cannot resolve index [test]", e.getMessage());
assertEquals("line 1:15: index [test] does not exist", e.getMessage());
}
}
}

View File

@ -9,17 +9,84 @@ import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import java.util.Objects;
/**
* Converts from Elasticsearch's internal metadata ({@link ClusterState})
* into a representation that is compatible with SQL (@{link {@link EsIndex}).
*/
public interface Catalog {
/**
* 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
* Lookup the information for a table.
*/
@Nullable
EsIndex getIndex(String index) throws SqlIllegalArgumentException;
GetIndexResult getIndex(String index);
class GetIndexResult {
public static GetIndexResult valid(EsIndex index) {
Objects.requireNonNull(index, "index must not be null if it was found");
return new GetIndexResult(index, null);
}
public static GetIndexResult invalid(String invalid) {
Objects.requireNonNull(invalid, "invalid must not be null to signal that the index is invalid");
return new GetIndexResult(null, invalid);
}
public static GetIndexResult notFound(String name) {
Objects.requireNonNull(name, "name must not be null");
return invalid("index [" + name + "] does not exist");
}
@Nullable
private final EsIndex index;
@Nullable
private final String invalid;
private GetIndexResult(@Nullable EsIndex index, @Nullable String invalid) {
this.index = index;
this.invalid = invalid;
}
/**
* Get the {@linkplain EsIndex} built by the {@linkplain Catalog}.
* @throws SqlIllegalArgumentException if the index is invalid for
* use with sql
*/
public EsIndex get() {
if (invalid != null) {
throw new SqlIllegalArgumentException(invalid);
}
return index;
}
/**
* Is the index valid for use with sql? Returns {@code false} if the
* index wasn't found.
*/
public boolean isValid() {
return invalid == null;
}
@Override
public boolean equals(Object obj) {
if (obj == null || obj.getClass() != getClass()) {
return false;
}
GetIndexResult other = (GetIndexResult) obj;
return Objects.equals(index, other.index)
&& Objects.equals(invalid, other.invalid);
}
@Override
public int hashCode() {
return Objects.hash(index, invalid);
}
@Override
public String toString() {
if (invalid != null) {
return "GetIndexResult[" + invalid + "]";
}
return "GetIndexResult[" + index + "]";
}
}
}

View File

@ -10,13 +10,15 @@ import com.carrotsearch.hppc.cursors.ObjectObjectCursor;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.metadata.MappingMetaData;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.collect.ImmutableOpenMap;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.Types;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Map;
public class EsCatalog implements Catalog {
private final ClusterState clusterState;
@ -26,51 +28,47 @@ public class EsCatalog implements Catalog {
}
@Override
public EsIndex getIndex(String index) throws SqlIllegalArgumentException {
public GetIndexResult getIndex(String index) throws SqlIllegalArgumentException {
IndexMetaData idx = clusterState.getMetaData().index(index);
if (idx == null) {
return null;
return GetIndexResult.notFound(index);
}
if (idx.getIndex().getName().startsWith(".")) {
/* Indices that start with "." are considered internal and
* should not be available to SQL. */
return null;
}
MappingMetaData type = singleType(idx.getMappings(), idx.getIndex().getName());
if (type == null) {
return null;
}
return EsIndex.build(idx, type);
return GetIndexResult.invalid(
"[" + idx.getIndex().getName() + "] starts with [.] so it is considered internal and incompatible with sql");
}
/**
* Return the single type in the index, {@code null} if there
* are no types in the index, and throw a {@link SqlIllegalArgumentException}
* if there are multiple types in the index.
*/
@Nullable
public MappingMetaData singleType(ImmutableOpenMap<String, MappingMetaData> mappings, String name) {
// Make sure that the index contains only a single type
MappingMetaData singleType = null;
List<String> typeNames = null;
for (ObjectObjectCursor<String, MappingMetaData> type : idx.getMappings()) {
/* We actually ignore the _default_ mapping because it is still
* allowed but deprecated. */
MappingMetaData result = null;
List<String> typeNames = null;
for (ObjectObjectCursor<String, MappingMetaData> type : mappings) {
if ("_default_".equals(type.key)) {
continue;
}
if (result != null) {
if (singleType != null) {
// There are more than one types
if (typeNames == null) {
typeNames = new ArrayList<>();
typeNames.add(result.type());
typeNames.add(singleType.type());
}
typeNames.add(type.key);
}
result = type.value;
singleType = type.value;
}
if (typeNames == null) {
return result;
if (singleType == null) {
return GetIndexResult.invalid("[" + idx.getIndex().getName() + "] doesn't have any types so it is incompatible with sql");
}
if (typeNames != null) {
Collections.sort(typeNames);
throw new SqlIllegalArgumentException("[" + name + "] has more than one type " + typeNames);
return GetIndexResult.invalid(
"[" + idx.getIndex().getName() + "] contains more than one type " + typeNames + " so it is incompatible with sql");
}
Map<String, DataType> mapping = Types.fromEs(singleType.sourceAsMap());
List<String> aliases = Arrays.asList(idx.getAliases().keys().toArray(String.class));
return GetIndexResult.valid(new EsIndex(idx.getIndex().getName(), mapping, aliases, idx.getSettings()));
}
}

View File

@ -5,15 +5,10 @@
*/
package org.elasticsearch.xpack.sql.analysis.catalog;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.metadata.MappingMetaData;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.xpack.sql.type.DataType;
import org.elasticsearch.xpack.sql.type.Types;
import org.elasticsearch.xpack.sql.util.StringUtils;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
@ -53,13 +48,6 @@ public class EsIndex {
return settings;
}
static EsIndex build(IndexMetaData metadata, @Nullable MappingMetaData type) {
Map<String, DataType> mapping = type != null ? Types.fromEs(type.sourceAsMap()) : emptyMap();
List<String> aliases = Arrays.asList(metadata.getAliases().keys().toArray(String.class));
return new EsIndex(metadata.getIndex().getName(), mapping, aliases, metadata.getSettings());
}
@Override
public String toString() {
return name;

View File

@ -11,11 +11,12 @@ package org.elasticsearch.xpack.sql.analysis.catalog;
public class FilteredCatalog implements Catalog {
public interface Filter {
/**
* Filter an index. Returning {@code null} will act as though
* the index wasn't found. Will never be called with a {@code null}
* parameter.
* Filter an index. Will only be called with valid,
* found indices but gets the entire {@link GetIndexResult}
* from the delegate catalog in case it wants to return
* it unchanged.
*/
EsIndex filterIndex(EsIndex index);
GetIndexResult filterIndex(GetIndexResult delegateResult);
}
private Catalog delegate;
@ -27,10 +28,10 @@ public class FilteredCatalog implements Catalog {
}
@Override
public EsIndex getIndex(String index) {
EsIndex result = delegate.getIndex(index);
if (result == null) {
return null;
public GetIndexResult getIndex(String index) {
GetIndexResult result = delegate.getIndex(index);
if (false == result.isValid() || result.get() == null) {
return result;
}
return filter.filterIndex(result);
}

View File

@ -27,8 +27,8 @@ import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.analysis.catalog.Catalog;
import org.elasticsearch.xpack.sql.analysis.catalog.Catalog.GetIndexResult;
import org.elasticsearch.xpack.sql.analysis.catalog.EsIndex;
import java.io.IOException;
@ -162,7 +162,7 @@ public class SqlGetIndicesAction
}
public static class TransportAction extends TransportMasterNodeReadAction<Request, Response> {
private final Function<ClusterState, Catalog> catalog;
private final Function<ClusterState, Catalog> catalogSupplier;
private final SqlLicenseChecker licenseChecker;
@Inject
@ -171,7 +171,7 @@ public class SqlGetIndicesAction
IndexNameExpressionResolver indexNameExpressionResolver, CatalogHolder catalog, SqlLicenseChecker licenseChecker) {
super(settings, NAME, transportService, clusterService, threadPool, actionFilters,
indexNameExpressionResolver, Request::new);
this.catalog = catalog.catalog;
this.catalogSupplier = catalog.catalogSupplier;
this.licenseChecker = licenseChecker;
}
@ -189,7 +189,7 @@ public class SqlGetIndicesAction
@Override
protected void masterOperation(Request request, ClusterState state, ActionListener<Response> listener) {
licenseChecker.checkIfSqlAllowed();
operation(indexNameExpressionResolver, catalog, request, state, listener);
operation(indexNameExpressionResolver, catalogSupplier, request, state, listener);
}
@Override
@ -202,10 +202,10 @@ public class SqlGetIndicesAction
* Class that holds that {@link Catalog} to aid in guice binding.
*/
public static class CatalogHolder {
final Function<ClusterState, Catalog> catalog;
final Function<ClusterState, Catalog> catalogSupplier;
public CatalogHolder(Function<ClusterState, Catalog> catalog) {
this.catalog = catalog;
public CatalogHolder(Function<ClusterState, Catalog> catalogSupplier) {
this.catalogSupplier = catalogSupplier;
}
}
}
@ -219,20 +219,15 @@ public class SqlGetIndicesAction
* what the user has permission to access, and leaves an appropriate
* audit trail.
*/
public static void operation(IndexNameExpressionResolver indexNameExpressionResolver, Function<ClusterState, Catalog> catalog,
public static void operation(IndexNameExpressionResolver indexNameExpressionResolver, Function<ClusterState, Catalog> catalogSupplier,
Request request, ClusterState state, ActionListener<Response> listener) {
String[] concreteIndices = indexNameExpressionResolver.concreteIndexNames(state, request);
List<EsIndex> results = new ArrayList<>(concreteIndices.length);
Catalog catalog = catalogSupplier.apply(state);
for (String index : concreteIndices) {
EsIndex esIndex;
try {
esIndex = catalog.apply(state).getIndex(index);
} catch (SqlIllegalArgumentException e) {
assert e.getMessage().contains("has more than one type");
esIndex = null;
}
if (esIndex != null) {
results.add(esIndex);
GetIndexResult result = catalog.getIndex(index);
if (result.isValid()) {
results.add(result.get());
}
}

View File

@ -95,7 +95,7 @@ public class SqlSession {
* Get an index. Prefer not to use this method as it cannot be made to work with cross cluster search.
*/
public EsIndex getIndexSync(String index) {
return catalog.getIndex(index);
return catalog.getIndex(index).get();
}
public Planner planner() {

View File

@ -13,6 +13,7 @@ import org.elasticsearch.cluster.metadata.MetaData;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.sql.SqlIllegalArgumentException;
import org.elasticsearch.xpack.sql.analysis.catalog.Catalog.GetIndexResult;
import java.io.IOException;
@ -21,47 +22,87 @@ import static java.util.Collections.emptyMap;
public class EsCatalogTests extends ESTestCase {
public void testEmpty() {
Catalog catalog = new EsCatalog(ClusterState.builder(ClusterName.DEFAULT).build());
assertNull(catalog.getIndex("test"));
assertEquals(GetIndexResult.notFound("test"), catalog.getIndex("test"));
}
public void testIndexExists() throws IOException {
Catalog catalog = new EsCatalog(ClusterState.builder(ClusterName.DEFAULT)
.metaData(MetaData.builder()
.put(index()
.putMapping("test", "{}"))
.build())
.put(index("test")
.putMapping("test", "{}")))
.build());
assertEquals(emptyMap(), catalog.getIndex("test").mapping());
GetIndexResult result = catalog.getIndex("test");
assertTrue(result.isValid());
assertEquals(emptyMap(), result.get().mapping());
}
public void testIndexAbsent() throws IOException {
Catalog catalog = new EsCatalog(ClusterState.builder(ClusterName.DEFAULT)
.metaData(MetaData.builder()
.put(index("test")
.putMapping("test", "{}")))
.build());
GetIndexResult result = catalog.getIndex("foo");
assertFalse(result.isValid());
}
public void testIndexWithDefaultType() throws IOException {
Catalog catalog = new EsCatalog(ClusterState.builder(ClusterName.DEFAULT)
.metaData(MetaData.builder()
.put(index()
.put(index("test")
.putMapping("test", "{}")
.putMapping("_default_", "{}"))
.build())
.putMapping("_default_", "{}")))
.build());
assertEquals(emptyMap(), catalog.getIndex("test").mapping());
GetIndexResult result = catalog.getIndex("test");
assertTrue(result.isValid());
assertEquals(emptyMap(), result.get().mapping());
}
public void testIndexWithTwoTypes() throws IOException {
Catalog catalog = new EsCatalog(ClusterState.builder(ClusterName.DEFAULT)
.metaData(MetaData.builder()
.put(index()
.put(index("test")
.putMapping("first_type", "{}")
.putMapping("second_type", "{}"))
.putMapping("second_type", "{}")))
.build());
GetIndexResult result = catalog.getIndex("test");
assertFalse(result.isValid());
Exception e = expectThrows(SqlIllegalArgumentException.class, result::get);
assertEquals(e.getMessage(), "[test] contains more than one type [first_type, second_type] so it is incompatible with sql");
}
public void testIndexWithNoTypes() throws IOException {
Catalog catalog = new EsCatalog(ClusterState.builder(ClusterName.DEFAULT)
.metaData(MetaData.builder()
.put(index("test")))
.build());
GetIndexResult result = catalog.getIndex("test");
assertFalse(result.isValid());
Exception e = expectThrows(SqlIllegalArgumentException.class, result::get);
assertEquals(e.getMessage(), "[test] doesn't have any types so it is incompatible with sql");
}
public void testIndexStartsWithDot() throws IOException {
Catalog catalog = new EsCatalog(ClusterState.builder(ClusterName.DEFAULT)
.metaData(MetaData.builder()
.put(index(".security")
.putMapping("test", "{}"))
.build())
.build());
Exception e = expectThrows(SqlIllegalArgumentException.class, () -> catalog.getIndex("test"));
assertEquals(e.getMessage(), "[test] has more than one type [first_type, second_type]");
GetIndexResult result = catalog.getIndex(".security");
assertFalse(result.isValid());
Exception e = expectThrows(SqlIllegalArgumentException.class, result::get);
assertEquals(e.getMessage(), "[.security] starts with [.] so it is considered internal and incompatible with sql");
}
private IndexMetaData.Builder index() throws IOException {
return IndexMetaData.builder("test")
private IndexMetaData.Builder index(String name) throws IOException {
return IndexMetaData.builder(name)
.settings(Settings.builder()
.put("index.version.created", Version.CURRENT)
.put("index.number_of_shards", 1)

View File

@ -7,7 +7,7 @@ package org.elasticsearch.xpack.sql.analysis.catalog;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.sql.analysis.catalog.FilteredCatalog.Filter;
import org.elasticsearch.xpack.sql.analysis.catalog.Catalog.GetIndexResult;
import java.util.Arrays;
import java.util.List;
@ -19,53 +19,47 @@ import static java.util.stream.Collectors.toList;
public class FilteredCatalogTests extends ESTestCase {
public void testGetTypeNoopCatalog() {
Catalog orig = inMemoryCatalog("a", "b", "c");
Catalog filtered = new FilteredCatalog(orig, new Filter() {
@Override
public EsIndex filterIndex(EsIndex index) {
return index;
}
});
assertSame(orig.getIndex("a"), filtered.getIndex("a"));
assertSame(orig.getIndex("b"), filtered.getIndex("b"));
assertSame(orig.getIndex("c"), filtered.getIndex("c"));
assertSame(orig.getIndex("missing"), filtered.getIndex("missing"));
Catalog filtered = new FilteredCatalog(orig, delegateResult -> delegateResult);
assertEquals(orig.getIndex("a"), filtered.getIndex("a"));
assertEquals(orig.getIndex("b"), filtered.getIndex("b"));
assertEquals(orig.getIndex("c"), filtered.getIndex("c"));
assertEquals(orig.getIndex("missing"), filtered.getIndex("missing"));
}
public void testGetTypeFiltering() {
Catalog orig = inMemoryCatalog("cat", "dog");
Catalog filtered = new FilteredCatalog(orig, new Filter() {
@Override
public EsIndex filterIndex(EsIndex index) {
return index.name().equals("cat") ? index : null;
Catalog filtered = new FilteredCatalog(orig, delegateResult -> {
if (delegateResult.get().name().equals("cat")) {
return delegateResult;
}
return GetIndexResult.notFound(delegateResult.get().name());
});
assertSame(orig.getIndex("cat"), filtered.getIndex("cat"));
assertNull(filtered.getIndex("dog"));
assertEquals(orig.getIndex("cat"), filtered.getIndex("cat"));
assertEquals(GetIndexResult.notFound("dog"), filtered.getIndex("dog"));
}
public void testGetTypeModifying() {
Catalog orig = inMemoryCatalog("cat", "dog");
Catalog filtered = new FilteredCatalog(orig, new Filter() {
@Override
public EsIndex filterIndex(EsIndex index) {
Catalog filtered = new FilteredCatalog(orig, delegateResult -> {
EsIndex index = delegateResult.get();
if (index.name().equals("cat")) {
return index;
}
return new EsIndex("rat", index.mapping(), index.aliases(), index.settings());
return delegateResult;
}
return GetIndexResult.valid(new EsIndex("rat", index.mapping(), index.aliases(), index.settings()));
});
assertSame(orig.getIndex("cat"), filtered.getIndex("cat"));
assertEquals("rat", filtered.getIndex("dog").name());
assertEquals(orig.getIndex("cat"), filtered.getIndex("cat"));
assertEquals("rat", filtered.getIndex("dog").get().name());
}
public void testGetTypeFilterIgnoresNull() {
Catalog filtered = new FilteredCatalog(inMemoryCatalog(), new Filter() {
@Override
public EsIndex filterIndex(EsIndex index) {
return index.name().equals("cat") ? index : null;
public void testGetTypeFilterIgnoresMissing() {
Catalog orig = inMemoryCatalog();
Catalog filtered = new FilteredCatalog(orig, delegateResult -> {
if (delegateResult.get().name().equals("cat")) {
return delegateResult;
}
return GetIndexResult.notFound(delegateResult.get().name());
});
assertNull(filtered.getIndex("missing"));
assertEquals(orig.getIndex("missing"), filtered.getIndex("missing"));
}
private Catalog inMemoryCatalog(String... indexNames) {

View File

@ -22,7 +22,8 @@ class InMemoryCatalog implements Catalog {
}
@Override
public EsIndex getIndex(String index) {
return indices.get(index);
public GetIndexResult getIndex(String index) {
EsIndex result = indices.get(index);
return result == null ? GetIndexResult.notFound(index) : GetIndexResult.valid(result);
}
}