From 979e54a1ee2f5e4b0f5d8494ff985eed6afc2e16 Mon Sep 17 00:00:00 2001 From: Konrad Dziedzic Date: Fri, 6 Oct 2023 15:03:50 +0200 Subject: [PATCH] Allow using fragment and query characters in S3 locations --- .../java/io/trino/filesystem/Location.java | 3 -- .../java/io/trino/filesystem/Locations.java | 10 ----- .../io/trino/filesystem/TestLocation.java | 41 +++++++------------ .../io/trino/filesystem/TestLocations.java | 18 -------- .../plugin/hive/BaseHiveConnectorTest.java | 5 +-- 5 files changed, 16 insertions(+), 61 deletions(-) diff --git a/lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java b/lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java index 08cbe7ef03d..a30f57cc9f5 100644 --- a/lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java +++ b/lib/trino-filesystem/src/main/java/io/trino/filesystem/Location.java @@ -61,9 +61,6 @@ public static Location of(String location) checkArgument(!location.isEmpty(), "location is empty"); checkArgument(!location.isBlank(), "location is blank"); - checkArgument(location.indexOf('#') < 0, "Fragment is not allowed in a file system location: %s", location); - checkArgument(location.indexOf('?') < 0, "URI query component is not allowed in a file system location: %s", location); - // legacy HDFS location that is just a path if (location.startsWith("/")) { return new Location(location, Optional.empty(), Optional.empty(), Optional.empty(), OptionalInt.empty(), location.substring(1)); diff --git a/lib/trino-filesystem/src/main/java/io/trino/filesystem/Locations.java b/lib/trino-filesystem/src/main/java/io/trino/filesystem/Locations.java index 13694965dc7..0e948aa5467 100644 --- a/lib/trino-filesystem/src/main/java/io/trino/filesystem/Locations.java +++ b/lib/trino-filesystem/src/main/java/io/trino/filesystem/Locations.java @@ -13,8 +13,6 @@ */ package io.trino.filesystem; -import static com.google.common.base.Preconditions.checkArgument; - public final class Locations { private Locations() {} @@ -25,20 +23,12 @@ private Locations() {} @Deprecated public static String appendPath(String location, String path) { - validateLocation(location); - if (!location.endsWith("/")) { location += "/"; } return location + path; } - private static void validateLocation(String location) - { - checkArgument(location.indexOf('?') < 0, "location contains a query string: %s", location); - checkArgument(location.indexOf('#') < 0, "location contains a fragment: %s", location); - } - /** * Verifies whether the two provided directory location parameters point to the same actual location. */ diff --git a/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java b/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java index b11746c14e9..6d540b19512 100644 --- a/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java +++ b/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocation.java @@ -80,6 +80,15 @@ void testParse() assertLocation("scheme://host///path//", "scheme", Optional.empty(), "host", "//path//"); + assertLocationWithoutUriTesting("scheme://userInfo@host/some/path#fragment", "scheme", Optional.of("userInfo"), "host", "some/path#fragment"); + assertLocationWithoutUriTesting("scheme://userInfo@ho#st/some/path", "scheme", Optional.of("userInfo"), "ho#st", "some/path"); + assertLocationWithoutUriTesting("scheme://user#Info@host/some/path", "scheme", Optional.of("user#Info"), "host", "some/path"); + assertLocationWithoutUriTesting("sc#heme://userInfo@host/some/path", "sc#heme", Optional.of("userInfo"), "host", "some/path"); + assertLocationWithoutUriTesting("scheme://userInfo@host/some/path?fragment", "scheme", Optional.of("userInfo"), "host", "some/path?fragment"); + assertLocationWithoutUriTesting("scheme://userInfo@ho?st/some/path", "scheme", Optional.of("userInfo"), "ho?st", "some/path"); + assertLocationWithoutUriTesting("scheme://user?Info@host/some/path", "scheme", Optional.of("user?Info"), "host", "some/path"); + assertLocationWithoutUriTesting("sc?heme://userInfo@host/some/path", "sc?heme", Optional.of("userInfo"), "host", "some/path"); + // the path can be empty assertLocation("scheme://", Optional.of("scheme"), Optional.empty(), Optional.empty(), OptionalInt.empty(), "", Set.of("Expected authority at index 9: scheme://")); assertLocation("scheme://host/", "scheme", Optional.empty(), "host", ""); @@ -169,34 +178,12 @@ void testParse() assertThatThrownBy(() -> Location.of("scheme://:")) .isInstanceOf(IllegalArgumentException.class) .hasMessage("Invalid port in file system location: scheme://:"); + } - // fragment is not allowed - assertThatThrownBy(() -> Location.of("scheme://userInfo@host/some/path#fragement")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Fragment is not allowed in a file system location: scheme://userInfo@host/some/path#fragement"); - assertThatThrownBy(() -> Location.of("scheme://userInfo@ho#st/some/path")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Fragment is not allowed in a file system location: scheme://userInfo@ho#st/some/path"); - assertThatThrownBy(() -> Location.of("scheme://user#Info@host/some/path")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Fragment is not allowed in a file system location: scheme://user#Info@host/some/path"); - assertThatThrownBy(() -> Location.of("sc#heme://userInfo@host/some/path")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("Fragment is not allowed in a file system location: sc#heme://userInfo@host/some/path"); - - // query component is not allowed - assertThatThrownBy(() -> Location.of("scheme://userInfo@host/some/path?fragement")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("URI query component is not allowed in a file system location: scheme://userInfo@host/some/path?fragement"); - assertThatThrownBy(() -> Location.of("scheme://userInfo@ho?st/some/path")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("URI query component is not allowed in a file system location: scheme://userInfo@ho?st/some/path"); - assertThatThrownBy(() -> Location.of("scheme://user?Info@host/some/path")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("URI query component is not allowed in a file system location: scheme://user?Info@host/some/path"); - assertThatThrownBy(() -> Location.of("sc?heme://userInfo@host/some/path")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessage("URI query component is not allowed in a file system location: sc?heme://userInfo@host/some/path"); + private static void assertLocationWithoutUriTesting(String locationString, String scheme, Optional userInfo, String host, String path) + { + Optional expectedHost = host.isEmpty() ? Optional.empty() : Optional.of(host); + assertLocation(Location.of(locationString), locationString, Optional.of(scheme), userInfo, expectedHost, OptionalInt.empty(), path, true, Set.of("skipped")); } private static void assertLocation(String locationString, String scheme, Optional userInfo, String host, String path) diff --git a/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocations.java b/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocations.java index fa339eea5d9..6d518383674 100644 --- a/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocations.java +++ b/lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocations.java @@ -23,7 +23,6 @@ import static io.trino.filesystem.Locations.appendPath; import static io.trino.filesystem.Locations.areDirectoryLocationsEquivalent; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; public class TestLocations { @@ -53,23 +52,6 @@ public void testAppendPath(String location, String path, String expected) assertThat(appendPath(location, path)).isEqualTo(expected); } - private static Stream invalidLocations() - { - return Stream.of( - Arguments.of("location?", "location contains a query string.*"), - Arguments.of("location#", "location contains a fragment.*")); - } - - @ParameterizedTest - @MethodSource("invalidLocations") - @SuppressWarnings("deprecation") // we're testing a deprecated method - public void testInvalidLocationInAppendPath(String location, String exceptionMessageRegexp) - { - assertThatThrownBy(() -> appendPath(location, "test")) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageMatching(exceptionMessageRegexp); - } - @Test public void testDirectoryLocationEquivalence() { diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java index aba1d111836..dac1e46df05 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java @@ -625,11 +625,10 @@ public void testSchemaOperations() public void testCreateSchemaWithIncorrectLocation() { String schemaName = "test_create_schema_with_incorrect_location_" + randomNameSuffix(); - String schemaLocation = "s3://testbucket/%s/a#hash/%s".formatted(schemaName, schemaName); + String schemaLocation = "s3://bucket"; assertThatThrownBy(() -> assertUpdate("CREATE SCHEMA " + schemaName + " WITH (location = '" + schemaLocation + "')")) - .hasMessageContaining("Invalid location URI") - .hasStackTraceContaining("Fragment is not allowed in a file system location"); + .hasMessageContaining("Invalid location URI"); } @Test