From 10dfd360308bc340a7cb72169471bee6238ecf4f Mon Sep 17 00:00:00 2001 From: Romain Manni-Bucau Date: Mon, 30 Mar 2020 13:11:46 +0200 Subject: [PATCH] making DefaultHttpDataFactory able to configure basedir and deleteonexit (#10146) Motivation: currently (http) disk based attributes or uploads are globally configured in a single directory and can also only globally be deleted on exit or not. it does not fit well multiple cases, in particular the case you have multiple servers in the same JVM. Modification: make it configurable per attribute/fileupload. Result: This PR duplicates Disk* constructor to add basedir and deleteonexit parameters and wires it in default http daa factory. --- .../multipart/DefaultHttpDataFactory.java | 42 +++++++++++++++---- .../codec/http/multipart/DiskAttribute.java | 41 ++++++++++++++++-- .../codec/http/multipart/DiskFileUpload.java | 21 ++++++++-- .../codec/http/multipart/MixedAttribute.java | 37 +++++++++++++--- .../codec/http/multipart/MixedFileUpload.java | 21 ++++++++-- .../multipart/DefaultHttpDataFactoryTest.java | 16 +++++++ .../http/multipart/DiskFileUploadTest.java | 16 +++++++ 7 files changed, 168 insertions(+), 26 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java index 9f92325bf4..857a0140e1 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactory.java @@ -19,6 +19,7 @@ import io.netty.handler.codec.http.DefaultHttpRequest; import io.netty.handler.codec.http.HttpConstants; import io.netty.handler.codec.http.HttpRequest; +import java.io.File; import java.io.IOException; import java.nio.charset.Charset; import java.util.ArrayList; @@ -59,6 +60,10 @@ public class DefaultHttpDataFactory implements HttpDataFactory { private Charset charset = HttpConstants.DEFAULT_CHARSET; + private String baseDir; + + private boolean deleteOnExit; // false is a good default cause true leaks + /** * Keep all {@link HttpData}s until cleaning methods are called. * We need to use {@link IdentityHashMap} because different requests may be equal. @@ -111,6 +116,25 @@ public class DefaultHttpDataFactory implements HttpDataFactory { this.charset = charset; } + /** + * Override global {@link DiskAttribute#baseDirectory} and {@link DiskFileUpload#baseDirectory} values. + * + * @param baseDir directory path where to store disk attributes and file uploads. + */ + public void setBaseDir(String baseDir) { + this.baseDir = baseDir; + } + + /** + * Override global {@link DiskAttribute#deleteOnExitTemporaryFile} and + * {@link DiskFileUpload#deleteOnExitTemporaryFile} values. + * + * @param deleteOnExit true if temporary files should be deleted with the JVM, false otherwise. + */ + public void setDeleteOnExit(boolean deleteOnExit) { + this.deleteOnExit = deleteOnExit; + } + @Override public void setMaxLimit(long maxSize) { this.maxSize = maxSize; @@ -131,14 +155,14 @@ public class DefaultHttpDataFactory implements HttpDataFactory { @Override public Attribute createAttribute(HttpRequest request, String name) { if (useDisk) { - Attribute attribute = new DiskAttribute(name, charset); + Attribute attribute = new DiskAttribute(name, charset, baseDir, deleteOnExit); attribute.setMaxSize(maxSize); List list = getList(request); list.add(attribute); return attribute; } if (checkSize) { - Attribute attribute = new MixedAttribute(name, minSize, charset); + Attribute attribute = new MixedAttribute(name, minSize, charset, baseDir, deleteOnExit); attribute.setMaxSize(maxSize); List list = getList(request); list.add(attribute); @@ -152,14 +176,14 @@ public class DefaultHttpDataFactory implements HttpDataFactory { @Override public Attribute createAttribute(HttpRequest request, String name, long definedSize) { if (useDisk) { - Attribute attribute = new DiskAttribute(name, definedSize, charset); + Attribute attribute = new DiskAttribute(name, definedSize, charset, baseDir, deleteOnExit); attribute.setMaxSize(maxSize); List list = getList(request); list.add(attribute); return attribute; } if (checkSize) { - Attribute attribute = new MixedAttribute(name, definedSize, minSize, charset); + Attribute attribute = new MixedAttribute(name, definedSize, minSize, charset, baseDir, deleteOnExit); attribute.setMaxSize(maxSize); List list = getList(request); list.add(attribute); @@ -186,11 +210,11 @@ public class DefaultHttpDataFactory implements HttpDataFactory { if (useDisk) { Attribute attribute; try { - attribute = new DiskAttribute(name, value, charset); + attribute = new DiskAttribute(name, value, charset, baseDir, deleteOnExit); attribute.setMaxSize(maxSize); } catch (IOException e) { // revert to Mixed mode - attribute = new MixedAttribute(name, value, minSize, charset); + attribute = new MixedAttribute(name, value, minSize, charset, baseDir, deleteOnExit); attribute.setMaxSize(maxSize); } checkHttpDataSize(attribute); @@ -199,7 +223,7 @@ public class DefaultHttpDataFactory implements HttpDataFactory { return attribute; } if (checkSize) { - Attribute attribute = new MixedAttribute(name, value, minSize, charset); + Attribute attribute = new MixedAttribute(name, value, minSize, charset, baseDir, deleteOnExit); attribute.setMaxSize(maxSize); checkHttpDataSize(attribute); List list = getList(request); @@ -222,7 +246,7 @@ public class DefaultHttpDataFactory implements HttpDataFactory { long size) { if (useDisk) { FileUpload fileUpload = new DiskFileUpload(name, filename, contentType, - contentTransferEncoding, charset, size); + contentTransferEncoding, charset, size, baseDir, deleteOnExit); fileUpload.setMaxSize(maxSize); checkHttpDataSize(fileUpload); List list = getList(request); @@ -231,7 +255,7 @@ public class DefaultHttpDataFactory implements HttpDataFactory { } if (checkSize) { FileUpload fileUpload = new MixedFileUpload(name, filename, contentType, - contentTransferEncoding, charset, size, minSize); + contentTransferEncoding, charset, size, minSize, baseDir, deleteOnExit); fileUpload.setMaxSize(maxSize); checkHttpDataSize(fileUpload); List list = getList(request); diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java index d70732ea02..8835b7e400 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskAttribute.java @@ -37,6 +37,10 @@ public class DiskAttribute extends AbstractDiskHttpData implements Attribute { public static final String postfix = ".att"; + private String baseDir; + + private boolean deleteOnExit; + /** * Constructor used for huge Attribute */ @@ -44,16 +48,40 @@ public class DiskAttribute extends AbstractDiskHttpData implements Attribute { this(name, HttpConstants.DEFAULT_CHARSET); } + public DiskAttribute(String name, String baseDir, boolean deleteOnExit) { + this(name, HttpConstants.DEFAULT_CHARSET); + this.baseDir = baseDir == null ? baseDirectory : baseDir; + this.deleteOnExit = deleteOnExit; + } + public DiskAttribute(String name, long definedSize) { + this(name, definedSize, HttpConstants.DEFAULT_CHARSET, baseDirectory, deleteOnExitTemporaryFile); + } + + public DiskAttribute(String name, long definedSize, String baseDir, boolean deleteOnExit) { this(name, definedSize, HttpConstants.DEFAULT_CHARSET); + this.baseDir = baseDir == null ? baseDirectory : baseDir; + this.deleteOnExit = deleteOnExit; } public DiskAttribute(String name, Charset charset) { + this(name, charset, baseDirectory, deleteOnExitTemporaryFile); + } + + public DiskAttribute(String name, Charset charset, String baseDir, boolean deleteOnExit) { super(name, charset, 0); + this.baseDir = baseDir == null ? baseDirectory : baseDir; + this.deleteOnExit = deleteOnExit; } public DiskAttribute(String name, long definedSize, Charset charset) { + this(name, definedSize, charset, baseDirectory, deleteOnExitTemporaryFile); + } + + public DiskAttribute(String name, long definedSize, Charset charset, String baseDir, boolean deleteOnExit) { super(name, charset, definedSize); + this.baseDir = baseDir == null ? baseDirectory : baseDir; + this.deleteOnExit = deleteOnExit; } public DiskAttribute(String name, String value) throws IOException { @@ -61,8 +89,15 @@ public class DiskAttribute extends AbstractDiskHttpData implements Attribute { } public DiskAttribute(String name, String value, Charset charset) throws IOException { + this(name, value, charset, baseDirectory, deleteOnExitTemporaryFile); + } + + public DiskAttribute(String name, String value, Charset charset, + String baseDir, boolean deleteOnExit) throws IOException { super(name, charset, 0); // Attribute have no default size setValue(value); + this.baseDir = baseDir == null ? baseDirectory : baseDir; + this.deleteOnExit = deleteOnExit; } @Override @@ -136,12 +171,12 @@ public class DiskAttribute extends AbstractDiskHttpData implements Attribute { @Override protected boolean deleteOnExit() { - return deleteOnExitTemporaryFile; + return deleteOnExit; } @Override protected String getBaseDirectory() { - return baseDirectory; + return baseDir; } @Override @@ -193,7 +228,7 @@ public class DiskAttribute extends AbstractDiskHttpData implements Attribute { @Override public Attribute replace(ByteBuf content) { - DiskAttribute attr = new DiskAttribute(getName()); + DiskAttribute attr = new DiskAttribute(getName(), baseDir, deleteOnExit); attr.setCharset(getCharset()); if (content != null) { try { diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java index 345f42e119..fdd6187b20 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/DiskFileUpload.java @@ -38,6 +38,10 @@ public class DiskFileUpload extends AbstractDiskHttpData implements FileUpload { public static final String postfix = ".tmp"; + private final String baseDir; + + private final boolean deleteOnExit; + private String filename; private String contentType; @@ -45,11 +49,19 @@ public class DiskFileUpload extends AbstractDiskHttpData implements FileUpload { private String contentTransferEncoding; public DiskFileUpload(String name, String filename, String contentType, - String contentTransferEncoding, Charset charset, long size) { + String contentTransferEncoding, Charset charset, long size, String baseDir, boolean deleteOnExit) { super(name, charset, size); setFilename(filename); setContentType(contentType); setContentTransferEncoding(contentTransferEncoding); + this.baseDir = baseDir == null ? baseDirectory : baseDir; + this.deleteOnExit = deleteOnExit; + } + + public DiskFileUpload(String name, String filename, String contentType, + String contentTransferEncoding, Charset charset, long size) { + this(name, filename, contentType, contentTransferEncoding, + charset, size, baseDirectory, deleteOnExitTemporaryFile); } @Override @@ -135,12 +147,12 @@ public class DiskFileUpload extends AbstractDiskHttpData implements FileUpload { @Override protected boolean deleteOnExit() { - return deleteOnExitTemporaryFile; + return deleteOnExit; } @Override protected String getBaseDirectory() { - return baseDirectory; + return baseDir; } @Override @@ -193,7 +205,8 @@ public class DiskFileUpload extends AbstractDiskHttpData implements FileUpload { @Override public FileUpload replace(ByteBuf content) { DiskFileUpload upload = new DiskFileUpload( - getName(), getFilename(), getContentType(), getContentTransferEncoding(), getCharset(), size); + getName(), getFilename(), getContentType(), getContentTransferEncoding(), getCharset(), size, + baseDir, deleteOnExit); if (content != null) { try { upload.setContent(content); diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java index 90b7a4af62..aa91264e7f 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedAttribute.java @@ -27,6 +27,8 @@ import java.nio.charset.Charset; * Mixed implementation using both in Memory and in File with a limit of size */ public class MixedAttribute implements Attribute { + private String baseDir; + private boolean deleteOnExit; private Attribute attribute; private final long limitSize; @@ -41,24 +43,45 @@ public class MixedAttribute implements Attribute { } public MixedAttribute(String name, long limitSize, Charset charset) { + this(name, limitSize, charset, DiskAttribute.baseDirectory, DiskAttribute.deleteOnExitTemporaryFile); + } + + public MixedAttribute(String name, long limitSize, Charset charset, String baseDir, boolean deleteOnExit) { this.limitSize = limitSize; attribute = new MemoryAttribute(name, charset); + this.baseDir = baseDir; + this.deleteOnExit = deleteOnExit; } public MixedAttribute(String name, long definedSize, long limitSize, Charset charset) { + this(name, definedSize, limitSize, charset, + DiskAttribute.baseDirectory, DiskAttribute.deleteOnExitTemporaryFile); + } + + public MixedAttribute(String name, long definedSize, long limitSize, Charset charset, + String baseDir, boolean deleteOnExit) { this.limitSize = limitSize; attribute = new MemoryAttribute(name, definedSize, charset); + this.baseDir = baseDir; + this.deleteOnExit = deleteOnExit; } public MixedAttribute(String name, String value, long limitSize) { - this(name, value, limitSize, HttpConstants.DEFAULT_CHARSET); + this(name, value, limitSize, HttpConstants.DEFAULT_CHARSET, + DiskAttribute.baseDirectory, DiskFileUpload.deleteOnExitTemporaryFile); } public MixedAttribute(String name, String value, long limitSize, Charset charset) { + this(name, value, limitSize, charset, + DiskAttribute.baseDirectory, DiskFileUpload.deleteOnExitTemporaryFile); + } + + public MixedAttribute(String name, String value, long limitSize, Charset charset, + String baseDir, boolean deleteOnExit) { this.limitSize = limitSize; if (value.length() > this.limitSize) { try { - attribute = new DiskAttribute(name, value, charset); + attribute = new DiskAttribute(name, value, charset, baseDir, deleteOnExit); } catch (IOException e) { // revert to Memory mode try { @@ -74,6 +97,8 @@ public class MixedAttribute implements Attribute { throw new IllegalArgumentException(e); } } + this.baseDir = baseDir; + this.deleteOnExit = deleteOnExit; } @Override @@ -100,7 +125,7 @@ public class MixedAttribute implements Attribute { checkSize(attribute.length() + buffer.readableBytes()); if (attribute.length() + buffer.readableBytes() > limitSize) { DiskAttribute diskAttribute = new DiskAttribute(attribute - .getName(), attribute.definedLength()); + .getName(), attribute.definedLength(), baseDir, deleteOnExit); diskAttribute.setMaxSize(maxSize); if (((MemoryAttribute) attribute).getByteBuf() != null) { diskAttribute.addContent(((MemoryAttribute) attribute) @@ -178,7 +203,7 @@ public class MixedAttribute implements Attribute { if (buffer.readableBytes() > limitSize) { if (attribute instanceof MemoryAttribute) { // change to Disk - attribute = new DiskAttribute(attribute.getName(), attribute.definedLength()); + attribute = new DiskAttribute(attribute.getName(), attribute.definedLength(), baseDir, deleteOnExit); attribute.setMaxSize(maxSize); } } @@ -191,7 +216,7 @@ public class MixedAttribute implements Attribute { if (file.length() > limitSize) { if (attribute instanceof MemoryAttribute) { // change to Disk - attribute = new DiskAttribute(attribute.getName(), attribute.definedLength()); + attribute = new DiskAttribute(attribute.getName(), attribute.definedLength(), baseDir, deleteOnExit); attribute.setMaxSize(maxSize); } } @@ -202,7 +227,7 @@ public class MixedAttribute implements Attribute { public void setContent(InputStream inputStream) throws IOException { if (attribute instanceof MemoryAttribute) { // change to Disk even if we don't know the size - attribute = new DiskAttribute(attribute.getName(), attribute.definedLength()); + attribute = new DiskAttribute(attribute.getName(), attribute.definedLength(), baseDir, deleteOnExit); attribute.setMaxSize(maxSize); } attribute.setContent(inputStream); diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java index b80b892359..c63060d64a 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/MixedFileUpload.java @@ -27,6 +27,10 @@ import java.nio.charset.Charset; */ public class MixedFileUpload implements FileUpload { + private final String baseDir; + + private final boolean deleteOnExit; + private FileUpload fileUpload; private final long limitSize; @@ -37,6 +41,13 @@ public class MixedFileUpload implements FileUpload { public MixedFileUpload(String name, String filename, String contentType, String contentTransferEncoding, Charset charset, long size, long limitSize) { + this(name, filename, contentType, contentTransferEncoding, + charset, size, limitSize, DiskFileUpload.baseDirectory, DiskFileUpload.deleteOnExitTemporaryFile); + } + + public MixedFileUpload(String name, String filename, String contentType, + String contentTransferEncoding, Charset charset, long size, + long limitSize, String baseDir, boolean deleteOnExit) { this.limitSize = limitSize; if (size > this.limitSize) { fileUpload = new DiskFileUpload(name, filename, contentType, @@ -46,6 +57,8 @@ public class MixedFileUpload implements FileUpload { contentTransferEncoding, charset, size); } definedSize = size; + this.baseDir = baseDir; + this.deleteOnExit = deleteOnExit; } @Override @@ -76,7 +89,7 @@ public class MixedFileUpload implements FileUpload { .getName(), fileUpload.getFilename(), fileUpload .getContentType(), fileUpload .getContentTransferEncoding(), fileUpload.getCharset(), - definedSize); + definedSize, baseDir, deleteOnExit); diskFileUpload.setMaxSize(maxSize); ByteBuf data = fileUpload.getByteBuf(); if (data != null && data.isReadable()) { @@ -177,7 +190,7 @@ public class MixedFileUpload implements FileUpload { .getName(), memoryUpload.getFilename(), memoryUpload .getContentType(), memoryUpload .getContentTransferEncoding(), memoryUpload.getCharset(), - definedSize); + definedSize, baseDir, deleteOnExit); fileUpload.setMaxSize(maxSize); // release old upload @@ -199,7 +212,7 @@ public class MixedFileUpload implements FileUpload { .getName(), memoryUpload.getFilename(), memoryUpload .getContentType(), memoryUpload .getContentTransferEncoding(), memoryUpload.getCharset(), - definedSize); + definedSize, baseDir, deleteOnExit); fileUpload.setMaxSize(maxSize); // release old upload @@ -219,7 +232,7 @@ public class MixedFileUpload implements FileUpload { .getName(), fileUpload.getFilename(), fileUpload .getContentType(), fileUpload .getContentTransferEncoding(), fileUpload.getCharset(), - definedSize); + definedSize, baseDir, deleteOnExit); fileUpload.setMaxSize(maxSize); // release old upload diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactoryTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactoryTest.java index 872686922d..9640142b31 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactoryTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DefaultHttpDataFactoryTest.java @@ -29,6 +29,7 @@ import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1; import static io.netty.handler.codec.http.multipart.HttpPostBodyUtil.DEFAULT_TEXT_CONTENT_TYPE; import static io.netty.util.CharsetUtil.UTF_8; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; @@ -57,6 +58,21 @@ public class DefaultHttpDataFactoryTest { factory.cleanAllHttpData(); } + @Test + public void customBaseDirAndDeleteOnExit() { + final DefaultHttpDataFactory defaultHttpDataFactory = new DefaultHttpDataFactory(true); + final String dir = "target/DefaultHttpDataFactoryTest/customBaseDirAndDeleteOnExit"; + defaultHttpDataFactory.setBaseDir(dir); + defaultHttpDataFactory.setDeleteOnExit(true); + final Attribute attr = defaultHttpDataFactory.createAttribute(req1, "attribute1"); + final FileUpload fu = defaultHttpDataFactory.createFileUpload( + req1, "attribute1", "f.txt", "text/plain", null, null, 0); + assertEquals(dir, DiskAttribute.class.cast(attr).getBaseDirectory()); + assertEquals(dir, DiskFileUpload.class.cast(fu).getBaseDirectory()); + assertTrue(DiskAttribute.class.cast(attr).deleteOnExit()); + assertTrue(DiskFileUpload.class.cast(fu).deleteOnExit()); + } + @Test public void cleanRequestHttpDataShouldIdentifiesRequestsByTheirIdentities() throws Exception { // Create some data belonging to req1 and req2 diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java index b4b7866e6d..1f1170ba3b 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/DiskFileUploadTest.java @@ -18,12 +18,28 @@ package io.netty.handler.codec.http.multipart; import io.netty.buffer.Unpooled; import org.junit.Test; +import java.io.File; import java.io.IOException; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertTrue; public class DiskFileUploadTest { + @Test + public void testSpecificCustomBaseDir() throws IOException { + File baseDir = new File("target/DiskFileUploadTest/testSpecificCustomBaseDir"); + baseDir.mkdirs(); // we don't need to clean it since it is in volatile files anyway + DiskFileUpload f = + new DiskFileUpload("d1", "d1", "application/json", null, null, 100, + baseDir.getAbsolutePath(), false); + + f.setContent(Unpooled.EMPTY_BUFFER); + + assertTrue(f.getFile().getAbsolutePath().startsWith(baseDir.getAbsolutePath())); + assertTrue(f.getFile().exists()); + assertEquals(0, f.getFile().length()); + f.delete(); + } @Test public final void testDiskFileUploadEquals() {