Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict dropping non-empty schema #8660

Merged
merged 2 commits into from
Jul 27, 2021
Merged

Restrict dropping non-empty schema #8660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
51a7bd4
Test if random schema name could be created
wendigo Jul 27, 2021
9f888c5
Restrict dropping non-empty schema
wendigo Jul 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions core/trino-main/src/main/java/io/trino/execution/DropSchemaTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import io.trino.Session;
import io.trino.execution.warnings.WarningCollector;
import io.trino.metadata.Metadata;
import io.trino.metadata.QualifiedTablePrefix;
import io.trino.security.AccessControl;
import io.trino.spi.TrinoException;
import io.trino.spi.connector.CatalogSchemaName;
Expand All @@ -30,6 +31,7 @@
import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.metadata.MetadataUtil.createCatalogSchemaName;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.spi.StandardErrorCode.SCHEMA_NOT_EMPTY;
import static io.trino.spi.StandardErrorCode.SCHEMA_NOT_FOUND;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;

Expand Down Expand Up @@ -72,10 +74,34 @@ public ListenableFuture<Void> execute(
return immediateVoidFuture();
}

if (!isSchemaEmpty(session, schema, metadata)) {
throw semanticException(SCHEMA_NOT_EMPTY, statement, "Cannot drop non-empty schema '%s'", schema.getSchemaName());
}

accessControl.checkCanDropSchema(session.toSecurityContext(), schema);

metadata.dropSchema(session, schema);

return immediateVoidFuture();
}

private static boolean isSchemaEmpty(Session session, CatalogSchemaName schema, Metadata metadata)
{
QualifiedTablePrefix tablePrefix = new QualifiedTablePrefix(schema.getCatalogName(), schema.getSchemaName());

// These are best efforts checks that don't provide any guarantees against concurrent DDL operations
if (!metadata.listTables(session, tablePrefix).isEmpty()) {
wendigo marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

if (!metadata.listViews(session, tablePrefix).isEmpty()) {
return false;
}

if (!metadata.listMaterializedViews(session, tablePrefix).isEmpty()) {
return false;
}

return true;
}
}
10 changes: 0 additions & 10 deletions ...rino-clickhouse/src/test/java/io/trino/plugin/clickhouse/TestClickHouseConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@
import static io.trino.testing.assertions.Assert.assertEquals;
import static io.trino.testing.sql.TestTable.randomTableSuffix;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;

Expand Down Expand Up @@ -81,15 +80,6 @@ protected boolean hasBehavior(TestingConnectorBehavior connectorBehavior)
}
}

@Override
public void testDropNonEmptySchema()
{
assertThatThrownBy(super::testDropNonEmptySchema)
.isInstanceOf(AssertionError.class)
.hasMessageStartingWith("Expected query to fail: DROP SCHEMA ");
throw new SkipException("DROP SCHEMA erases all tables in ClickHouse connector"); // TODO (https://github.com/trinodb/trino/issues/8634)
}

@Override
@Test(dataProvider = "testColumnNameDataProvider")
public void testColumnName(String columnName)
Expand Down
6 changes: 0 additions & 6 deletions plugin/trino-hive/src/main/java/io/trino/plugin/hive/HiveMetadata.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,6 @@
import static io.trino.spi.StandardErrorCode.INVALID_SCHEMA_PROPERTY;
import static io.trino.spi.StandardErrorCode.INVALID_TABLE_PROPERTY;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.spi.StandardErrorCode.SCHEMA_NOT_EMPTY;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.spi.predicate.TupleDomain.withColumnDomains;
import static io.trino.spi.statistics.TableStatisticType.ROW_COUNT;
Expand Down Expand Up @@ -767,11 +766,6 @@ public void createSchema(ConnectorSession session, String schemaName, Map<String
@Override
public void dropSchema(ConnectorSession session, String schemaName)
{
// basic sanity check to provide a better error message
if (!listTables(session, Optional.of(schemaName)).isEmpty() ||
!listViews(session, Optional.of(schemaName)).isEmpty()) {
throw new TrinoException(SCHEMA_NOT_EMPTY, "Schema not empty: " + schemaName);
}
metastore.dropDatabase(new HiveIdentity(session), schemaName);
}

Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ public void testSchemaOperations()

assertUpdate(session, "CREATE TABLE new_schema.test (x bigint)");

assertQueryFails(session, "DROP SCHEMA new_schema", "Schema not empty: new_schema");
assertQueryFails(session, "DROP SCHEMA new_schema", ".*Cannot drop non-empty schema 'new_schema'");

assertUpdate(session, "DROP TABLE new_schema.test");

Expand Down
10 changes: 0 additions & 10 deletions plugin/trino-memsql/src/test/java/io/trino/plugin/memsql/TestMemSqlConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
import static java.util.stream.Collectors.joining;
import static java.util.stream.IntStream.range;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;

Expand Down Expand Up @@ -161,15 +160,6 @@ public void testInsertUnicode()
throw new SkipException("MemSQL doesn't support utf8mb4");
}

@Override
public void testDropNonEmptySchema()
{
assertThatThrownBy(super::testDropNonEmptySchema)
.isInstanceOf(AssertionError.class)
.hasMessageStartingWith("Expected query to fail: DROP SCHEMA ");
throw new SkipException("DROP SCHEMA erases all tables in MemSQL connector"); // TODO (https://github.com/trinodb/trino/issues/8634)
}

@Test
public void testDropTable()
{
Expand Down
11 changes: 0 additions & 11 deletions plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import io.trino.testing.TestingConnectorBehavior;
import io.trino.testing.sql.TestTable;
import org.intellij.lang.annotations.Language;
import org.testng.SkipException;
import org.testng.annotations.Test;

import java.util.Optional;
Expand All @@ -34,7 +33,6 @@
import static io.trino.testing.assertions.Assert.assertEquals;
import static java.lang.String.format;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.testng.Assert.assertFalse;
import static org.testng.Assert.assertTrue;

Expand Down Expand Up @@ -180,15 +178,6 @@ public void testShowCreateTable()
")");
}

@Override
public void testDropNonEmptySchema()
{
assertThatThrownBy(super::testDropNonEmptySchema)
.isInstanceOf(AssertionError.class)
.hasMessageStartingWith("Expected query to fail: DROP SCHEMA ");
throw new SkipException("DROP SCHEMA erases all tables in MySQL connector"); // TODO (https://github.com/trinodb/trino/issues/8634)
}

@Test
public void testDropTable()
{
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-phoenix/src/test/java/io/trino/plugin/phoenix/TestPhoenixConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public void testSchemaOperations()

assertThatThrownBy(() -> getQueryRunner().execute("DROP SCHEMA new_schema"))
.isInstanceOf(RuntimeException.class)
.hasMessage("ERROR 723 (43M06): Cannot mutate schema as schema has existing tables schemaName=NEW_SCHEMA");
.hasMessageContaining("Cannot drop non-empty schema 'new_schema'");

assertUpdate("DROP TABLE new_schema.test");
assertUpdate("DROP SCHEMA new_schema");
Expand Down
2 changes: 1 addition & 1 deletion plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ public void testSchemaOperations()

assertThatThrownBy(() -> getQueryRunner().execute("DROP SCHEMA new_schema"))
.isInstanceOf(RuntimeException.class)
.hasMessage("ERROR 723 (43M06): Cannot mutate schema as schema has existing tables schemaName=NEW_SCHEMA");
.hasMessageContaining("Cannot drop non-empty schema 'new_schema'");

assertUpdate("DROP TABLE new_schema.test");
assertUpdate("DROP SCHEMA new_schema");
Expand Down
2 changes: 1 addition & 1 deletion ...g/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestCreateDropSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public void testCreateDropSchema()

onTrino().executeQuery("CREATE TABLE test_drop_schema.test_drop (col1 int)");
assertThat(() -> query("DROP SCHEMA test_drop_schema"))
.failsWithMessage("Schema not empty: test_drop_schema");
.failsWithMessage("line 1:1: Cannot drop non-empty schema 'test_drop_schema'");

onTrino().executeQuery("DROP TABLE test_drop_schema.test_drop");

Expand Down
11 changes: 3 additions & 8 deletions testing/trino-testing/src/main/java/io/trino/testing/AbstractTestDistributedQueries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1135,19 +1135,14 @@ public void testCreateSchema()
@Test
public void testDropNonEmptySchema()
wendigo marked this conversation as resolved.
Show resolved Hide resolved
{
if (!supportsCreateSchema()) {
assertQueryFails("DROP SCHEMA " + getSession().getSchema().orElseThrow(), "This connector does not support dropping schemas");
String schemaName = "test_drop_non_empty_schema_" + randomTableSuffix();
if (!supportsDropSchema()) {
return;
}

String schemaName = "test_drop_non_empty_schema_" + randomTableSuffix();
assertUpdate("CREATE SCHEMA " + schemaName);
assertUpdate("CREATE TABLE " + schemaName + ".t(x int)");
assertQueryFails("DROP SCHEMA " + schemaName, "(?si).*" +
"(Schema not empty|" +
"Cannot drop schema|" +
"schema has (\\w+ )?tables|" +
"Cannot drop .{0,3}\\Q" + schemaName + "\\E).*");
assertQueryFails("DROP SCHEMA " + schemaName, ".*Cannot drop non-empty schema '\\Q" + schemaName + "\\E'");
assertUpdate("DROP TABLE " + schemaName + ".t");
assertUpdate("DROP SCHEMA " + schemaName);
}
Expand Down
6 changes: 2 additions & 4 deletions testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,14 +223,12 @@ public void testRowLevelDelete()
@Test
public void testCreateSchema()
{
String schemaName = "test_schema_create_" + randomTableSuffix();
wendigo marked this conversation as resolved.
Show resolved Hide resolved
wendigo marked this conversation as resolved.
Show resolved Hide resolved
if (!hasBehavior(SUPPORTS_CREATE_SCHEMA)) {
assertQueryFails("CREATE SCHEMA xxxxxx", "This connector does not support creating schemas");
getSession().getSchema().ifPresent(
s -> assertQueryFails("DROP SCHEMA " + s, "This connector does not support dropping schemas"));
assertQueryFails("CREATE SCHEMA " + schemaName, "This connector does not support creating schemas");
return;
}

String schemaName = "test_schema_create_" + randomTableSuffix();
assertUpdate("CREATE SCHEMA " + schemaName);
assertThat(query("SHOW SCHEMAS"))
.skippingTypesCheck()
Expand Down