Forbid scheme://authority locations (without trailing slash)

lib/trino-filesystem-azure/src/test/java/io/trino/filesystem/azure/AbstractTestAzureFileSystem.java

@@ -78,7 +78,7 @@ protected void initialize(String account, String accountKey, AccountKind expecte
         checkState(accountKind == expectedAccountKind, "Expected %s account, but found %s".formatted(expectedAccountKind, accountKind));
 
         containerName = "test-%s-%s".formatted(accountKind.name().toLowerCase(ROOT), randomUUID());
-        rootLocation = Location.of("abfs://%s@%s.dfs.core.windows.net".formatted(containerName, account));
+        rootLocation = Location.of("abfs://%s@%s.dfs.core.windows.net/".formatted(containerName, account));
 
         blobContainerClient = blobServiceClient.getBlobContainerClient(containerName);
         // this will fail if the container already exists, which is what we want

lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/AbstractTestS3FileSystem.java

@@ -64,7 +64,7 @@ protected final TrinoFileSystem getFileSystem()
     @Override
     protected final Location getRootLocation()
     {
-        return Location.of("s3://" + bucket());
+        return Location.of("s3://%s/".formatted(bucket()));
     }
 
     @Override

lib/trino-filesystem-s3/src/test/java/io/trino/filesystem/s3/TestS3Location.java

@@ -24,7 +24,6 @@ public class TestS3Location
     @Test
     public void testValidUri()
     {
-        assertS3Uri("s3://abc", "abc", "");
         assertS3Uri("s3://abc/", "abc", "");
         assertS3Uri("s3://abc/x", "abc", "x");
         assertS3Uri("s3://abc/xyz/fooBAR", "abc", "xyz/fooBAR");
@@ -47,6 +46,10 @@ public void testInvalidUri()
                 .isInstanceOf(IllegalArgumentException.class)
                 .hasMessage("No bucket for S3 location: s3://");
 
+        assertThatThrownBy(() -> new S3Location(Location.of("s3://abc")))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("Path missing in file system location: s3://abc");
+
         assertThatThrownBy(() -> new S3Location(Location.of("s3:///abc")))
                 .isInstanceOf(IllegalArgumentException.class)
                 .hasMessage("No bucket for S3 location: s3:///abc");

lib/trino-filesystem/pom.xml

@@ -59,6 +59,12 @@
             <scope>runtime</scope>
         </dependency>
 
+        <dependency>
+            <groupId>org.jetbrains</groupId>
+            <artifactId>annotations</artifactId>
+            <scope>provided</scope>
+        </dependency>
+
         <!-- for testing -->
         <dependency>
             <groupId>io.trino</groupId>

lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java

@@ -96,6 +96,7 @@ public static Location of(String location)
                 }
             }
 
+            checkArgument((userInfo.isEmpty() && host.isEmpty() && port.isEmpty()) || authoritySplit.size() == 2, "Path missing in file system location: %s", location);
             String path = (authoritySplit.size() == 2) ? authoritySplit.get(1) : "";
 
             return new Location(location, Optional.of(scheme), userInfo, host, port, path);
@@ -187,14 +188,12 @@ public Location parentDirectory()
     {
         // todo should this only be allowed for file locations?
         verifyValidFileLocation();
-        checkState(!path.isEmpty(), "root location does not have parent: %s", location);
+        checkState(!path.isEmpty() && !path.equals("/"), "root location does not have parent: %s", location);
 
         int lastIndexOfSlash = path.lastIndexOf('/');
         if (lastIndexOfSlash < 0) {
             String newLocation = location.substring(0, location.length() - path.length() - 1);
-            if (newLocation.isEmpty()) {
-                newLocation = "/";
-            }
+            newLocation += "/";
             return withPath(newLocation, "");
         }
 
@@ -277,7 +276,7 @@ public void verifyValidFileLocation()
         // file path must not be empty
         checkState(!path.isEmpty() && !path.equals("/"), "File location must contain a path: %s", location);
         // file path cannot end with a slash
-        checkState(!path.endsWith("/"), "File location cannot end with '/': '%s'", location);
+        checkState(!path.endsWith("/"), "File location cannot end with '/': %s", location);
         // file path cannot end with whitespace
         checkState(!isWhitespace(path.charAt(path.length() - 1)), "File location cannot end with whitespace: %s", location);
     }

lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocations.java

@@ -157,9 +157,9 @@ public void testInvalidParent(String location, String exceptionMessageRegexp)
     @Test
     public void testDirectoryLocationEquivalence()
     {
-        assertDirectoryLocationEquivalence("scheme://authority", "scheme://authority", true);
-        assertDirectoryLocationEquivalence("scheme://authority", "scheme://authority/", true);
-        assertDirectoryLocationEquivalence("scheme://authority", "scheme://authority//", false);
+        assertDirectoryLocationEquivalence("scheme://authority/", "scheme://authority/", true);
+        assertDirectoryLocationEquivalence("scheme://authority/", "scheme://authority//", false);
+        assertDirectoryLocationEquivalence("scheme://authority/", "scheme://authority///", false);
         assertDirectoryLocationEquivalence("scheme://userInfo@host:1234/dir", "scheme://userInfo@host:1234/dir/", true);
         assertDirectoryLocationEquivalence("scheme://authority/some/path", "scheme://authority/some/path", true);
         assertDirectoryLocationEquivalence("scheme://authority/some/path", "scheme://authority/some/path/", true);

plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/file/FileHiveMetastore.java

@@ -339,15 +339,17 @@ else if (table.getTableType().equals(MANAGED_TABLE.name())) {
             }
         }
         else if (table.getTableType().equals(EXTERNAL_TABLE.name())) {
-            try {
-                Path externalLocation = new Path(table.getStorage().getLocation());
-                FileSystem externalFileSystem = hdfsEnvironment.getFileSystem(hdfsContext, externalLocation);
-                if (!externalFileSystem.isDirectory(externalLocation)) {
-                    throw new TrinoException(HIVE_METASTORE_ERROR, "External table location does not exist");
+            if (!disableLocationChecks) {
+                try {
+                    Path externalLocation = new Path(table.getStorage().getLocation());
+                    FileSystem externalFileSystem = hdfsEnvironment.getFileSystem(hdfsContext, externalLocation);
+                    if (!externalFileSystem.isDirectory(externalLocation)) {
+                        throw new TrinoException(HIVE_METASTORE_ERROR, "External table location does not exist");
+                    }
+                }
+                catch (IOException e) {
+                    throw new TrinoException(HIVE_METASTORE_ERROR, "Could not validate external location", e);
                 }
-            }
-            catch (IOException e) {
-                throw new TrinoException(HIVE_METASTORE_ERROR, "Could not validate external location", e);
             }
         }
         else if (!table.getTableType().equals(MATERIALIZED_VIEW.name())) {

plugin/trino-hive/src/test/java/io/trino/plugin/hive/s3/TestHiveS3MinioQueries.java

@@ -0,0 +1,124 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.trino.plugin.hive.s3;
+
+import com.google.common.collect.ImmutableMap;
+import io.trino.plugin.hive.HiveQueryRunner;
+import io.trino.plugin.hive.NodeVersion;
+import io.trino.plugin.hive.metastore.HiveMetastoreConfig;
+import io.trino.plugin.hive.metastore.file.FileHiveMetastore;
+import io.trino.plugin.hive.metastore.file.FileHiveMetastoreConfig;
+import io.trino.testing.AbstractTestQueryFramework;
+import io.trino.testing.DataProviders;
+import io.trino.testing.QueryRunner;
+import io.trino.testing.containers.Minio;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.Test;
+
+import java.io.File;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static com.google.common.base.Verify.verify;
+import static io.trino.plugin.hive.HiveTestUtils.HDFS_ENVIRONMENT;
+import static io.trino.testing.TestingNames.randomNameSuffix;
+import static io.trino.testing.containers.Minio.MINIO_ACCESS_KEY;
+import static io.trino.testing.containers.Minio.MINIO_SECRET_KEY;
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class TestHiveS3MinioQueries
+        extends AbstractTestQueryFramework
+{
+    private Minio minio;
+
+    @Override
+    protected QueryRunner createQueryRunner()
+            throws Exception
+    {
+        minio = closeAfterClass(Minio.builder().build());
+        minio.start();
+
+        return HiveQueryRunner.builder()
+                .setMetastore(queryRunner -> {
+                    File baseDir = queryRunner.getCoordinator().getBaseDataDir().resolve("hive_data").toFile();
+                    return new FileHiveMetastore(
+                            new NodeVersion("testversion"),
+                            HDFS_ENVIRONMENT,
+                            new HiveMetastoreConfig().isHideDeltaLakeTables(),
+                            new FileHiveMetastoreConfig()
+                                    .setCatalogDirectory(baseDir.toURI().toString())
+                                    .setDisableLocationChecks(true) // matches Glue behavior
+                                    .setMetastoreUser("test"));
+                })
+                .setHiveProperties(ImmutableMap.<String, String>builder()
+                        .put("hive.s3.aws-access-key", MINIO_ACCESS_KEY)
+                        .put("hive.s3.aws-secret-key", MINIO_SECRET_KEY)
+                        .put("hive.s3.endpoint", minio.getMinioAddress())
+                        .put("hive.s3.path-style-access", "true")
+                        .put("hive.non-managed-table-writes-enabled", "true")
+                        .buildOrThrow())
+                .build();
+    }
+
+    @AfterClass(alwaysRun = true)
+    public void cleanUp()
+    {
+        minio = null; // closed by closeAfterClass
+    }
+
+    @Test(dataProviderClass = DataProviders.class, dataProvider = "trueFalse")
+    public void testTableLocationTopOfTheBucket(boolean locationWithTrailingSlash)
+    {
+        String bucketName = "test-bucket-" + randomNameSuffix();
+        minio.createBucket(bucketName);
+        minio.writeFile("We are\nawesome at\nmultiple slashes.".getBytes(UTF_8), bucketName, "a_file");
+
+        String location = "s3://%s%s".formatted(bucketName, locationWithTrailingSlash ? "/" : "");
+        String tableName = "test_table_top_of_bucket_%s_%s".formatted(locationWithTrailingSlash, randomNameSuffix());
+        String create = "CREATE TABLE %s (a varchar) WITH (format='TEXTFILE', external_location='%s')".formatted(tableName, location);
+        if (!locationWithTrailingSlash) {
+            assertQueryFails(create, "External location is not a valid file system URI: " + location);
+            return;
+        }
+        assertUpdate(create);
+
+        // Verify location was not normalized along the way. Glue would not do that.
+        assertThat(getDeclaredTableLocation(tableName))
+                .isEqualTo(location);
+
+        assertThat(query("TABLE " + tableName))
+                .matches("VALUES VARCHAR 'We are', 'awesome at', 'multiple slashes.'");
+
+        assertUpdate("INSERT INTO " + tableName + " VALUES 'Aren''t we?'", 1);
+
+        assertThat(query("TABLE " + tableName))
+                .matches("VALUES VARCHAR 'We are', 'awesome at', 'multiple slashes.', 'Aren''t we?'");
+
+        assertUpdate("DROP TABLE " + tableName);
+    }
+
+    private String getDeclaredTableLocation(String tableName)
+    {
+        Pattern locationPattern = Pattern.compile(".*external_location = '(.*?)'.*", Pattern.DOTALL);
+        Object result = computeScalar("SHOW CREATE TABLE " + tableName);
+        Matcher matcher = locationPattern.matcher((String) result);
+        if (matcher.find()) {
+            String location = matcher.group(1);
+            verify(!matcher.find(), "Unexpected second match");
+            return location;
+        }
+        throw new IllegalStateException("Location not found in: " + result);
+    }
+}