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
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 @@
/**
* 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 @@
* @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 @@
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 @@
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 @@
/**
* 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 @@
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 @@
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 @@
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 @@
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 @@
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.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 @@
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 @@
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 @@
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());
