Skip to content

Refactor appendPath helper function to use Location#appendPath #25609

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

Closed
wants to merge 2 commits into from
Closed

Refactor appendPath helper function to use Location#appendPath #25609

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
3d0a1d3
Refactor appendPath helper function to use `Location#appendPath`
chenjian2664 Mar 25, 2025
e6b3590
empty
chenjian2664 Apr 20, 2025
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
9 changes: 1 addition & 8 deletions lib/trino-filesystem/src/main/java/io/trino/filesystem/Locations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,16 +21,9 @@ public final class Locations

private Locations() {}

/**
* @deprecated use {@link Location#appendPath(String)} instead
*/
@Deprecated
public static String appendPath(String location, String path)
{
if (!location.endsWith("/")) {
location += "/";
}
return location + path;
return Location.of(location).appendPath(path).toString();
}

/**
Expand Down
66 changes: 37 additions & 29 deletions lib/trino-filesystem/src/test/java/io/trino/filesystem/TestLocations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,44 +14,52 @@
package io.trino.filesystem;

import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.stream.Stream;

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
{
private static Stream<Arguments> locations()
@Test
void testAppendPath()
{
return Stream.of(
Arguments.of("test_dir", "", "test_dir/"),
Arguments.of("", "test_file.txt", "/test_file.txt"),
Arguments.of("test_dir", "test_file.txt", "test_dir/test_file.txt"),
Arguments.of("/test_dir", "test_file.txt", "/test_dir/test_file.txt"),
Arguments.of("test_dir/", "test_file.txt", "test_dir/test_file.txt"),
Arguments.of("/test_dir/", "test_file.txt", "/test_dir/test_file.txt"),
Arguments.of("test_dir", "test_dir2/", "test_dir/test_dir2/"),
Arguments.of("test_dir/", "test_dir2/", "test_dir/test_dir2/"),
Arguments.of("s3:/test_dir", "test_file.txt", "s3:/test_dir/test_file.txt"),
Arguments.of("s3://test_dir", "test_file.txt", "s3://test_dir/test_file.txt"),
Arguments.of("s3://test_dir/", "test_file.txt", "s3://test_dir/test_file.txt"),
Arguments.of("s3://test_dir/", "location?", "s3://test_dir/location?"),
Arguments.of("s3://test_dir/", "location#", "s3://test_dir/location#"),
Arguments.of("s3://dir_with_space ", "test_file.txt", "s3://dir_with_space /test_file.txt"),
Arguments.of("s3://dir_with_double_space ", "test_file.txt", "s3://dir_with_double_space /test_file.txt"));
}
assertThat(appendPath("/test_dir", "test_file.txt")).isEqualTo("/test_dir/test_file.txt");
assertThat(appendPath("/test_dir/", "test_file.txt")).isEqualTo("/test_dir/test_file.txt");
assertThat(appendPath("/test_dir", "test_dir2/")).isEqualTo("/test_dir/test_dir2/");
assertThat(appendPath("/test_dir/", "test_dir2/")).isEqualTo("/test_dir/test_dir2/");
assertThat(appendPath("s3://test_dir/", "test_file.txt")).isEqualTo("s3://test_dir/test_file.txt");
assertThat(appendPath("s3://test_dir/", "location?")).isEqualTo("s3://test_dir/location?");
assertThat(appendPath("s3://test_dir/", "location#")).isEqualTo("s3://test_dir/location#");
assertThat(appendPath("s3://test_dir/", "test_dir2/")).isEqualTo("s3://test_dir/test_dir2/");
assertThat(appendPath("s3://dir_with_space /", "test_file.txt")).isEqualTo("s3://dir_with_space /test_file.txt");
assertThat(appendPath("s3://dir_with_space /", "location?")).isEqualTo("s3://dir_with_space /location?");
assertThat(appendPath("s3://dir_with_space /", "location#")).isEqualTo("s3://dir_with_space /location#");
assertThat(appendPath("s3://dir_with_space /", "test_dir2/")).isEqualTo("s3://dir_with_space /test_dir2/");
assertThat(appendPath("s3://dir_with_double_space /", "test_file.txt")).isEqualTo("s3://dir_with_double_space /test_file.txt");
assertThat(appendPath("s3://dir_with_double_space /", "location?")).isEqualTo("s3://dir_with_double_space /location?");
assertThat(appendPath("s3://dir_with_double_space /", "location#")).isEqualTo("s3://dir_with_double_space /location#");
assertThat(appendPath("s3://dir_with_double_space /", "test_dir2/")).isEqualTo("s3://dir_with_double_space /test_dir2/");

@ParameterizedTest
@MethodSource("locations")
@SuppressWarnings("deprecation") // we're testing a deprecated method
public void testAppendPath(String location, String path, String expected)
{
assertThat(appendPath(location, path)).isEqualTo(expected);
assertThatThrownBy(() -> appendPath("", "test_file.txt"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("location is empty");
assertThatThrownBy(() -> appendPath("test_dir", "test_file.txt"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("No scheme for file system location: test_dir");
assertThatThrownBy(() -> appendPath("s3:test_dir", "test_file.txt"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Path must begin with a '/' when no authority is present");
assertThatThrownBy(() -> appendPath("s3://test_dir", "test_file.txt"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Path missing in file system location: s3://test_dir");
assertThatThrownBy(() -> appendPath("s3://test_dir ", "test_file.txt"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Path missing in file system location: s3://test_dir ");
assertThatThrownBy(() -> appendPath("s3://user@host:invalid_port", "test_file.txt"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Invalid port in file system location: s3://user@host:invalid_port");
}

@Test
Expand Down