Skip to content

HADOOP-18928: S3AFileSystem URL encodes twice where Path has trailing / #6167

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 1 commit into from
+16 −1
Closed

HADOOP-18928: S3AFileSystem URL encodes twice where Path has trailing / #6167

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
8 changes: 7 additions & 1 deletion hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3AFileSystem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.io.InterruptedIOException;
import java.io.UncheckedIOException;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.file.AccessDeniedException;
import java.text.DateFormat;
import java.text.SimpleDateFormat;
Expand Down Expand Up @@ -1652,7 +1653,12 @@ public Path makeQualified(final Path path) {
LOG.debug("Stripping trailing '/' from {}", q);
Copy link
Contributor

@steveloughran steveloughran Oct 25, 2023

Choose a reason for hiding this comment

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

how about pulling out everything in this method after the super.makeQualified(path); call into a static method in S3AUtils? that way it could be tested in a unit test rather than waiting until someone runs the integration tests. faster and with yetus coverage

// deal with an empty "/" at the end by mapping to the parent and
// creating a new path from it
q = new Path(urlString.substring(0, urlString.length() - 1));
try {
q = new Path(new URI(urlString.substring(0, urlString.length() - 1)));
} catch (URISyntaxException e) {
LOG.error(String.format("Error removing trailing / from path %s", path.toString()));
Copy link
Contributor

@steveloughran steveloughran Oct 25, 2023

Choose a reason for hiding this comment

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

  1. We'd want a LogExactlyOnce so if there is a problem it only ever gets logged once, not every time a path was loaded.
  2. it should be at warn, not error
  3. +use slf4j {} expansions.
    thanks

return q;
}
}
}
if (!q.isRoot() && q.getName().isEmpty()) {
Expand Down
9 changes: 9 additions & 0 deletions hadoop-tools/hadoop-aws/src/test/java/org/apache/hadoop/fs/s3a/ITestS3AMiscOperations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,15 @@ public void testPathFixup() throws Throwable {
fs.makeQualified(pathFromTrailingURI));
}

@Test
public void testQualifyPathWithWhitespace() {
final S3AFileSystem fs = getFileSystem();
Path pathWithSpace = fs.makeQualified(new Path("path with space"));
Path pathWithSpaceTrailingSlash = fs.makeQualified(new Path("path with space/"));
// removing trailing / should not recreate the Path object in a URL encoded way
assertEquals(pathWithSpace, pathWithSpaceTrailingSlash);
}

/**
* Verify that paths with a trailing "//" are fixed up.
*/
Expand Down