This is an automated email from the ASF dual-hosted git repository. srdo pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/storm.git
The following commit(s) were added to refs/heads/master by this push: new a694d87 STORM-3472: Add tests missing for STORM-3411, make the download file name generat… (#3091) a694d87 is described below commit a694d873f93616474430a337d7819e174f9e9b5b Author: Stig Døssing <s...@apache.org> AuthorDate: Fri Jul 26 18:40:29 2019 +0200 STORM-3472: Add tests missing for STORM-3411, make the download file name generat… (#3091) * STORM-3472: Add tests missing for STORM-3411, make the download file name generation code less error prone --- .../daemon/logviewer/handler/LogviewerProfileHandler.java | 10 ++++++---- .../storm/daemon/logviewer/utils/LogFileDownloader.java | 13 ++++++++++++- .../daemon/logviewer/utils/LogviewerResponseBuilder.java | 15 ++++----------- .../storm/daemon/logviewer/webapp/LogviewerResource.java | 7 +++---- .../handler/LogviewerLogDownloadHandlerTest.java | 9 ++++++++- .../logviewer/handler/LogviewerProfileHandlerTest.java | 15 +++++++++------ 6 files changed, 42 insertions(+), 27 deletions(-) diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java index 58b883a..c68ab3c 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandler.java @@ -102,7 +102,6 @@ public class LogviewerProfileHandler { /** * Download a dump file. * - * @param host host address * @param topologyId topology ID * @param hostPort host and port of worker * @param fileName dump file name @@ -110,8 +109,10 @@ public class LogviewerProfileHandler { * @return a Response which lets browsers download that file. * @see {@link org.apache.storm.daemon.logviewer.utils.LogFileDownloader#downloadFile(String, String, String, boolean)} */ - public Response downloadDumpFile(String host, String topologyId, String hostPort, String fileName, String user) throws IOException { - String portStr = hostPort.split(":")[1]; + public Response downloadDumpFile(String topologyId, String hostPort, String fileName, String user) throws IOException { + String[] hostPortSplit = hostPort.split(":"); + String host = hostPortSplit[0]; + String portStr = hostPortSplit[1]; Path rawFile = logRoot.resolve(topologyId).resolve(portStr).resolve(fileName); Path absFile = rawFile.toAbsolutePath().normalize(); if (!absFile.startsWith(logRoot) || !rawFile.normalize().toString().equals(rawFile.toString())) { @@ -122,7 +123,8 @@ public class LogviewerProfileHandler { if (absFile.toFile().exists()) { String workerFileRelativePath = String.join(File.separator, topologyId, portStr, WORKER_LOG_FILENAME); if (resourceAuthorizer.isUserAllowedToAccessFile(user, workerFileRelativePath)) { - return LogviewerResponseBuilder.buildDownloadFile(host, absFile.toFile(), numFileDownloadExceptions); + String downloadedFileName = host + "-" + topologyId + "-" + portStr + "-" + absFile.getFileName(); + return LogviewerResponseBuilder.buildDownloadFile(downloadedFileName, absFile.toFile(), numFileDownloadExceptions); } else { return LogviewerResponseBuilder.buildResponseUnauthorizedUser(user); } diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java index 17eb10f..22ce298 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogFileDownloader.java @@ -80,7 +80,18 @@ public class LogFileDownloader { if (file.toFile().exists()) { if (isDaemon || resourceAuthorizer.isUserAllowedToAccessFile(user, fileName)) { fileDownloadSizeDistMb.update(Math.round((double) file.toFile().length() / FileUtils.ONE_MB)); - return LogviewerResponseBuilder.buildDownloadFile(host, file.toFile(), numFileDownloadExceptions); + String downloadedFileName; + Path pathRelativeToRootDir = rootDir.relativize(file); + if (isDaemon || pathRelativeToRootDir.getNameCount() != 3) { + downloadedFileName = host + "-" + rawFile.getFileName(); + } else { + //host-topoId-port-fileName + downloadedFileName = host + "-" + + pathRelativeToRootDir.getName(0) + "-" + + pathRelativeToRootDir.getName(1) + "-" + + pathRelativeToRootDir.getName(2); + } + return LogviewerResponseBuilder.buildDownloadFile(downloadedFileName, file.toFile(), numFileDownloadExceptions); } else { return LogviewerResponseBuilder.buildResponseUnauthorizedUser(user); } diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java index a53be71..a44c825 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/LogviewerResponseBuilder.java @@ -75,25 +75,18 @@ public class LogviewerResponseBuilder { /** * Build a Response object representing download a file. * - * @param host host address + * @param contentDispositionName The name to set in the Content-Disposition header * @param file file to download */ - public static Response buildDownloadFile(String host, File file, Meter numFileDownloadExceptions) throws IOException { - String fname; - try { - String topoInfo = file.getParentFile().getParentFile().getName(); - String port = file.getParentFile().getName(); - fname = String.format("%s-%s-%s-%s", host, port, topoInfo, file.getName()); - } catch (NullPointerException e) { - fname = file.getName(); - } + public static Response buildDownloadFile(String contentDispositionName, + File file, Meter numFileDownloadExceptions) throws IOException { try { // do not close this InputStream in method: it will be used from jetty server InputStream is = Files.newInputStream(file.toPath()); return Response.status(OK) .entity(wrapWithStreamingOutput(is)) .type(MediaType.APPLICATION_OCTET_STREAM_TYPE) - .header("Content-Disposition", "attachment; filename=\"" + fname + "\"") + .header("Content-Disposition", "attachment; filename=\"" + contentDispositionName + "\"") .build(); } catch (IOException e) { numFileDownloadExceptions.mark(); diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/webapp/LogviewerResource.java b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/webapp/LogviewerResource.java index fb85911..d5a42ac 100644 --- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/webapp/LogviewerResource.java +++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/webapp/LogviewerResource.java @@ -233,8 +233,7 @@ public class LogviewerResource { String user = httpCredsHandler.getUserName(request); try { - String host = InetAddress.getLocalHost().getCanonicalHostName(); - return profileHandler.downloadDumpFile(host, topologyId, hostPort, fileName, user); + return profileHandler.downloadDumpFile(topologyId, hostPort, fileName, user); } catch (IOException e) { numDownloadDumpExceptions.mark(); throw e; @@ -252,7 +251,7 @@ public class LogviewerResource { String file = request.getParameter("file"); String decodedFileName = Utils.urlDecodeUtf8(file); try { - String host = InetAddress.getLocalHost().getCanonicalHostName(); + String host = Utils.hostname(); return logDownloadHandler.downloadLogFile(host, decodedFileName, user); } catch (IOException e) { numDownloadLogExceptions.mark(); @@ -271,7 +270,7 @@ public class LogviewerResource { String file = request.getParameter("file"); String decodedFileName = Utils.urlDecodeUtf8(file); try { - String host = InetAddress.getLocalHost().getCanonicalHostName(); + String host = Utils.hostname(); return logDownloadHandler.downloadDaemonLogFile(host, decodedFileName, user); } catch (IOException e) { numDownloadDaemonLogExceptions.mark(); diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java index a21ab5a..b09501f 100644 --- a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerLogDownloadHandlerTest.java @@ -19,13 +19,14 @@ package org.apache.storm.daemon.logviewer.handler; +import static org.hamcrest.CoreMatchers.containsString; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.nullValue; import static org.junit.Assert.assertThat; +import com.google.common.net.HttpHeaders; import java.io.IOException; -import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.Map; @@ -52,8 +53,12 @@ public class LogviewerLogDownloadHandlerTest { assertThat(topoAResponse.getStatus(), is(Response.Status.OK.getStatusCode())); assertThat(topoAResponse.getEntity(), not(nullValue())); + String topoAContentDisposition = topoAResponse.getHeaderString(HttpHeaders.CONTENT_DISPOSITION); + assertThat(topoAContentDisposition, containsString("host-topoA-1111-worker.log")); assertThat(topoBResponse.getStatus(), is(Response.Status.OK.getStatusCode())); assertThat(topoBResponse.getEntity(), not(nullValue())); + String topoBContentDisposition = topoBResponse.getHeaderString(HttpHeaders.CONTENT_DISPOSITION); + assertThat(topoBContentDisposition, containsString("host-topoB-1111-worker.log")); } } @@ -83,6 +88,8 @@ public class LogviewerLogDownloadHandlerTest { assertThat(response.getStatus(), is(Response.Status.OK.getStatusCode())); assertThat(response.getEntity(), not(nullValue())); + String contentDisposition = response.getHeaderString(HttpHeaders.CONTENT_DISPOSITION); + assertThat(contentDisposition, containsString("host-nimbus.log")); } } diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java index 02744ac..0318268 100644 --- a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java +++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/handler/LogviewerProfileHandlerTest.java @@ -25,14 +25,13 @@ import static org.hamcrest.CoreMatchers.not; import static org.hamcrest.CoreMatchers.nullValue; import static org.junit.Assert.assertThat; +import com.google.common.net.HttpHeaders; import java.io.IOException; -import java.io.InputStream; import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.Map; import javax.ws.rs.core.Response; -import org.apache.commons.io.IOUtils; import org.apache.storm.daemon.logviewer.utils.ResourceAuthorizer; import org.apache.storm.metric.StormMetricsRegistry; import org.apache.storm.testing.TmpPath; @@ -98,15 +97,19 @@ public class LogviewerProfileHandlerTest { LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); - Response topoAResponse = handler.downloadDumpFile("host","topoA", "localhost:1111", "worker.jfr", "user"); - Response topoBResponse = handler.downloadDumpFile("host","topoB", "localhost:1111", "worker.txt", "user"); + Response topoAResponse = handler.downloadDumpFile("topoA", "localhost:1111", "worker.jfr", "user"); + Response topoBResponse = handler.downloadDumpFile("topoB", "localhost:1111", "worker.txt", "user"); Utils.forceDelete(rootPath.toString()); assertThat(topoAResponse.getStatus(), is(Response.Status.OK.getStatusCode())); assertThat(topoAResponse.getEntity(), not(nullValue())); + String topoAContentDisposition = topoAResponse.getHeaderString(HttpHeaders.CONTENT_DISPOSITION); + assertThat(topoAContentDisposition, containsString("localhost-topoA-1111-worker.jfr")); assertThat(topoBResponse.getStatus(), is(Response.Status.OK.getStatusCode())); assertThat(topoBResponse.getEntity(), not(nullValue())); + String topoBContentDisposition = topoBResponse.getHeaderString(HttpHeaders.CONTENT_DISPOSITION); + assertThat(topoBContentDisposition, containsString("localhost-topoB-1111-worker.txt")); } } @@ -116,7 +119,7 @@ public class LogviewerProfileHandlerTest { LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); - Response topoAResponse = handler.downloadDumpFile("host","../../", "localhost:logs", "daemon-dump.bin", "user"); + Response topoAResponse = handler.downloadDumpFile("../../", "localhost:logs", "daemon-dump.bin", "user"); Utils.forceDelete(rootPath.toString()); @@ -130,7 +133,7 @@ public class LogviewerProfileHandlerTest { LogviewerProfileHandler handler = createHandlerTraversalTests(rootPath.getFile().toPath()); - Response topoAResponse = handler.downloadDumpFile("host","../", "localhost:../logs", "daemon-dump.bin", "user"); + Response topoAResponse = handler.downloadDumpFile("../", "localhost:../logs", "daemon-dump.bin", "user"); Utils.forceDelete(rootPath.toString());