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

Fix tests failing due to lowering restrictions on s3 paths #19361

Merged
merged 1 commit into from
Oct 12, 2023
Merged

Fix tests failing due to lowering restrictions on s3 paths #19361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 4 additions & 3 deletions ...c/test/java/io/trino/plugin/deltalake/metastore/glue/TestDeltaS3AndGlueMetastoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import java.nio.file.Path;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import static com.google.common.collect.Iterables.getOnlyElement;
Expand Down Expand Up @@ -61,7 +62,7 @@ protected void validateDataFiles(String partitionColumn, String tableName, Strin
{
String locationDirectory = location.endsWith("/") ? location : location + "/";
String partitionPart = partitionColumn.isEmpty() ? "" : partitionColumn + "=[a-z0-9]+/";
assertThat(dataFile).matches("^" + locationDirectory + partitionPart + "[a-zA-Z0-9_-]+$");
assertThat(dataFile).matches("^" + Pattern.quote(locationDirectory) + partitionPart + "[a-zA-Z0-9_-]+$");
findepi marked this conversation as resolved.
Show resolved Hide resolved
verifyPathExist(dataFile);
});
}
Expand All @@ -72,11 +73,11 @@ protected void validateMetadataFiles(String location)
String locationDirectory = location.endsWith("/") ? location : location + "/";
getAllMetadataDataFilesFromTableDirectory(location).forEach(metadataFile ->
{
assertThat(metadataFile).matches("^" + locationDirectory + "_delta_log/[0-9]+.json$");
assertThat(metadataFile).matches("^" + Pattern.quote(locationDirectory) + "_delta_log/[0-9]+.json$");
verifyPathExist(metadataFile);
});

assertThat(getExtendedStatisticsFileFromTableDirectory(location)).matches("^" + locationDirectory + "_delta_log/_trino_meta/extended_stats.json$");
assertThat(getExtendedStatisticsFileFromTableDirectory(location)).matches("^" + Pattern.quote(locationDirectory) + "_delta_log/_trino_meta/extended_stats.json$");
}

@Override
Expand Down
4 changes: 3 additions & 1 deletion plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseS3AndGlueMetastoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L
assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_int int, col_str varchar)" + partitionQueryPart);
try (UncheckedCloseable ignoredDropTable = onClose("DROP TABLE " + qualifiedTableName)) {
// in case of regular CREATE TABLE, location has generated suffix
String expectedTableLocationPattern = (schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName + "-[a-z0-9]+";
String expectedTableLocationPattern = Pattern.quote(schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName + "-[a-z0-9]+";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not quote tableName too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think it was necessary, I may add it but it is only in our tests and currently they work without it

actualTableLocation = getTableLocation(qualifiedTableName);
assertThat(actualTableLocation).matches(expectedTableLocationPattern);

Expand Down Expand Up @@ -323,6 +323,8 @@ protected enum LocationPattern
DOUBLE_SLASH("s3://%s/%s//double_slash/%s"),
TRIPLE_SLASH("s3://%s/%s///triple_slash/%s"),
PERCENT("s3://%s/%s/a%%percent/%s"),
HASH("s3://%s/%s/a#hash/%s"),
QUESTION_MARK("s3://%s/%s/a?question_mark/%s"),
WHITESPACE("s3://%s/%s/a whitespace/%s"),
TRAILING_WHITESPACE("s3://%s/%s/trailing_whitespace/%s "),
/**/;
Expand Down
29 changes: 3 additions & 26 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveS3AndGlueMetastoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
import java.util.regex.Pattern;

import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.DOUBLE_SLASH;
import static io.trino.plugin.hive.BaseS3AndGlueMetastoreTest.LocationPattern.TRIPLE_SLASH;
Expand Down Expand Up @@ -89,7 +90,7 @@ protected void validateDataFiles(String partitionColumn, String tableName, Strin
{
String locationDirectory = location.endsWith("/") ? location : location + "/";
String partitionPart = partitionColumn.isEmpty() ? "" : partitionColumn + "=[a-z0-9]+/";
assertThat(dataFile).matches("^" + locationDirectory + partitionPart + "[a-zA-Z0-9_-]+$");
assertThat(dataFile).matches("^" + Pattern.quote(locationDirectory) + partitionPart + "[a-zA-Z0-9_-]+$");
findepi marked this conversation as resolved.
Show resolved Hide resolved
verifyPathExist(dataFile);
});
}
Expand Down Expand Up @@ -186,7 +187,7 @@ public void testBasicOperationsWithProvidedSchemaLocation(boolean partitioned, L

assertUpdate("CREATE TABLE " + qualifiedTableName + "(col_str varchar, col_int int)" + partitionQueryPart);
try (UncheckedCloseable ignoredDropTable = onClose("DROP TABLE " + qualifiedTableName)) {
String expectedTableLocation = ((schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName)
String expectedTableLocation = Pattern.quote((schemaLocation.endsWith("/") ? schemaLocation : schemaLocation + "/") + tableName)
// Hive normalizes repeated slashes
.replaceAll("(?<!(s3:))/+", "/");

Expand Down Expand Up @@ -278,30 +279,6 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPa
}
}

@Test
public void testCreateTableWithIncorrectLocation()
{
String tableName = "test_create_table_with_incorrect_location_" + randomNameSuffix();
String location = "s3://%s/%s/a#hash/%s".formatted(bucketName, schemaName, tableName);

assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + tableName + "(col_str varchar, col_int integer) WITH (external_location = '" + location + "')"))
.hasMessageContaining("External location is not a valid file system URI")
.hasStackTraceContaining("Fragment is not allowed in a file system location");
}

@Test
public void testCtasWithIncorrectLocation()
{
String tableName = "test_ctas_with_incorrect_location_" + randomNameSuffix();
String location = "s3://%s/%s/a#hash/%s".formatted(bucketName, schemaName, tableName);

assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + tableName + "(col_str, col_int)" +
" WITH (external_location = '" + location + "')" +
" AS VALUES ('str1', 1)"))
.hasMessageContaining("External location is not a valid file system URI")
.hasStackTraceContaining("Fragment is not allowed in a file system location");
}

@Test
public void testSchemaNameEscape()
{
Expand Down
28 changes: 3 additions & 25 deletions ...src/test/java/io/trino/plugin/iceberg/catalog/glue/TestIcebergS3AndGlueMetastoreTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@

import java.nio.file.Path;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import static io.trino.plugin.hive.metastore.glue.GlueHiveMetastore.createTestingGlueHiveMetastore;
import static io.trino.testing.TestingNames.randomNameSuffix;
import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

public class TestIcebergS3AndGlueMetastoreTest
extends BaseS3AndGlueMetastoreTest
Expand Down Expand Up @@ -60,7 +60,7 @@ protected void validateDataFiles(String partitionColumn, String tableName, Strin
{
String locationDirectory = location.endsWith("/") ? location : location + "/";
String partitionPart = partitionColumn.isEmpty() ? "" : partitionColumn + "=[a-z0-9]+/";
assertThat(dataFile).matches("^" + locationDirectory + "data/" + partitionPart + "[a-zA-Z0-9_-]+.(orc|parquet)$");
assertThat(dataFile).matches("^" + Pattern.quote(locationDirectory) + "data/" + partitionPart + "[a-zA-Z0-9_-]+.(orc|parquet)$");
findepi marked this conversation as resolved.
Show resolved Hide resolved
verifyPathExist(dataFile);
});
}
Expand All @@ -71,7 +71,7 @@ protected void validateMetadataFiles(String location)
getAllMetadataDataFilesFromTableDirectory(location).forEach(metadataFile ->
{
String locationDirectory = location.endsWith("/") ? location : location + "/";
assertThat(metadataFile).matches("^" + locationDirectory + "metadata/[a-zA-Z0-9_-]+.(avro|metadata.json|stats)$");
assertThat(metadataFile).matches("^" + Pattern.quote(locationDirectory) + "metadata/[a-zA-Z0-9_-]+.(avro|metadata.json|stats)$");
verifyPathExist(metadataFile);
});
}
Expand Down Expand Up @@ -134,26 +134,4 @@ public void testAnalyzeWithProvidedTableLocation(boolean partitioned, LocationPa
assertQuery("SHOW STATS FOR " + tableName, expectedStatistics);
}
}

@Test
public void testCreateTableWithIncorrectLocation()
{
String tableName = "test_create_table_with_incorrect_location_" + randomNameSuffix();
String location = "s3://%s/%s/a#hash/%s".formatted(bucketName, schemaName, tableName);

assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + tableName + " (key integer, value varchar) WITH (location = '" + location + "')"))
.hasMessageContaining("Fragment is not allowed in a file system location");
}

@Test
public void testCtasWithIncorrectLocation()
{
String tableName = "test_create_table_with_incorrect_location_" + randomNameSuffix();
String location = "s3://%s/%s/a#hash/%s".formatted(bucketName, schemaName, tableName);

assertThatThrownBy(() -> assertUpdate("CREATE TABLE " + tableName +
" WITH (location = '" + location + "')" +
" AS SELECT * FROM tpch.tiny.nation"))
.hasMessageContaining("Fragment is not allowed in a file system location");
}
}