Ensure the content copy file is always removed

- in order to prevent out of disk space errors (#650) - code tidy up Fixes #650
paulcwarren · Oct 22, 2021 · 5b6b397 · 5b6b397
1 parent 3b42171
commit 5b6b397
Show file tree

Hide file tree

Showing 4 changed files with 150 additions and 39 deletions.
diff --git a/.../main/java/internal/org/springframework/content/commons/repository/factory/StoreImpl.java b/.../main/java/internal/org/springframework/content/commons/repository/factory/StoreImpl.java
@@ -9,9 +9,11 @@
 import java.io.OutputStream;
 import java.io.Serializable;
 import java.nio.file.Files;
+import java.nio.file.Path;
 
-import org.springframework.content.commons.io.FileRemover;
-import org.springframework.content.commons.io.ObservableInputStream;
+import org.apache.commons.io.IOUtils;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
 import org.springframework.content.commons.property.PropertyPath;
 import org.springframework.content.commons.repository.ContentStore;
 import org.springframework.content.commons.repository.events.AfterAssociateEvent;
@@ -33,32 +35,37 @@
 
 public class StoreImpl implements ContentStore<Object, Serializable> {
 
+    private static Log logger = LogFactory.getLog(StoreImpl.class);
+
     private final ContentStore<Object, Serializable> delegate;
     private final ApplicationEventPublisher publisher;
+    private final Path copyContentRootPath;
 
-    public StoreImpl(ContentStore<Object, Serializable> delegate, ApplicationEventPublisher publisher) {
+    public StoreImpl(ContentStore<Object, Serializable> delegate, ApplicationEventPublisher publisher, Path copyContentRootPath) {
         this.delegate = delegate;
         this.publisher = publisher;
+        this.copyContentRootPath = copyContentRootPath;
     }
 
     @Override
     public Object setContent(Object property, InputStream content) {
 
         Object result = null;
 
+        File contentCopy = null;
+        TeeInputStream contentCopyStream = null;
         try {
-            File tmpStreamFile = Files.createTempFile("sc", "bsce").toFile();
-            TeeInputStream eventStream = new TeeInputStream(content, new FileOutputStream(tmpStreamFile), true);
-            BeforeSetContentEvent before = new BeforeSetContentEvent(property, delegate, eventStream);
+            contentCopy = Files.createTempFile(copyContentRootPath, "contentCopy", ".tmp").toFile();
+            contentCopyStream = new TeeInputStream(content, new FileOutputStream(contentCopy), true);
+            BeforeSetContentEvent before = new BeforeSetContentEvent(property, delegate, contentCopyStream);
             AfterSetContentEvent after = new AfterSetContentEvent(property, delegate);
 
             publisher.publishEvent(before);
 
-            if (eventStream != null && eventStream.isDirty()) {
-                while (eventStream.read(new byte[4096]) != -1) {
+            if (contentCopyStream != null && contentCopyStream.isDirty()) {
+                while (contentCopyStream.read(new byte[4096]) != -1) {
                 }
-                eventStream.close();
-                content = new ObservableInputStream(new FileInputStream(tmpStreamFile), new FileRemover(tmpStreamFile));
+                content = new FileInputStream(contentCopy);
             }
 
             try {
@@ -76,6 +83,17 @@ public Object setContent(Object property, InputStream content) {
             fileNotFoundException.printStackTrace();
         } catch (IOException ioException) {
             ioException.printStackTrace();
+        } finally {
+            if (contentCopyStream != null) {
+                IOUtils.closeQuietly(contentCopyStream);
+            }
+            if (contentCopy != null) {
+                try {
+                    Files.deleteIfExists(contentCopy.toPath());
+                } catch (IOException e) {
+                    logger.error(String.format("Unable to delete content copy %s", contentCopy.toPath()), e);
+                }
+            }
         }
 
         return result;

diff --git a/...java/org/springframework/content/commons/repository/factory/AbstractStoreFactoryBean.java b/...java/org/springframework/content/commons/repository/factory/AbstractStoreFactoryBean.java
@@ -4,6 +4,7 @@
 import java.lang.reflect.Method;
 import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
+import java.nio.file.Paths;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
@@ -182,7 +183,7 @@ protected Store<? extends Serializable> createContentStore() {
 
 		StoreMethodInterceptor intercepter = new StoreMethodInterceptor();
 
-		storeFragments.add(new StoreFragment(storeInterface, new StoreImpl((ContentStore<Object, Serializable>) target, publisher)));
+		storeFragments.add(new StoreFragment(storeInterface, new StoreImpl((ContentStore<Object, Serializable>) target, publisher, Paths.get(System.getProperty("java.io.tmpdir")))));
 		intercepter.setStoreFragments(storeFragments);
 
 		result.addAdvice(intercepter);

diff --git a/...t/java/internal/org/springframework/content/commons/repository/factory/StoreImplTest.java b/...t/java/internal/org/springframework/content/commons/repository/factory/StoreImplTest.java
@@ -0,0 +1,68 @@
+package internal.org.springframework.content.commons.repository.factory;
+
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.BeforeEach;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.Context;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.Describe;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.It;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.JustBeforeEach;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.mock;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.nio.file.Files;
+import java.nio.file.Path;
+
+import org.junit.runner.RunWith;
+import org.springframework.content.commons.repository.ContentStore;
+import org.springframework.context.ApplicationEventPublisher;
+
+import com.github.paulcwarren.ginkgo4j.Ginkgo4jRunner;
+
+@RunWith(Ginkgo4jRunner.class)
+public class StoreImplTest {
+
+    private StoreImpl stores;
+
+    private ContentStore store;
+    private ApplicationEventPublisher publisher;
+    private String tmpDir;
+    private Path contentCopyPathRoot;
+
+    {
+        Describe("StoreImpl", () -> {
+
+            BeforeEach(() -> {
+                store = mock(ContentStore.class);
+                publisher = mock(ApplicationEventPublisher.class);
+
+            });
+            JustBeforeEach(() -> {
+                contentCopyPathRoot = Files.createTempDirectory("storeimpltest");
+
+                for (File f : contentCopyPathRoot.toFile().listFiles()) {
+                    if (f.getName().endsWith(".tmp")) {
+                        f.delete(); // may fail mysteriously - returns boolean you may want to check
+                    }
+                }
+
+                stores = new StoreImpl(store, publisher, contentCopyPathRoot);
+            });
+
+            Context("#setContent - inputstream", () -> {
+
+                JustBeforeEach(() -> {
+                    stores.setContent(new Object(), new ByteArrayInputStream("foo".getBytes()));
+                });
+
+                It("should delete the content copy file", () -> {
+                    for (File f : contentCopyPathRoot.toFile().listFiles()) {
+                        if (f.getName().endsWith(".tmp")) {
+                            fail("Found orphaned content copy path");
+                        }
+                    }
+                });
+            });
+        });
+    }
+}
diff --git a/...al/org/springframework/content/commons/repository/factory/StoreMethodInterceptorTest.java b/...al/org/springframework/content/commons/repository/factory/StoreMethodInterceptorTest.java
@@ -1,12 +1,35 @@
 package internal.org.springframework.content.commons.repository.factory;
 
-import com.github.paulcwarren.ginkgo4j.Ginkgo4jConfiguration;
-import com.github.paulcwarren.ginkgo4j.Ginkgo4jRunner;
-import internal.org.springframework.content.commons.config.StoreFragment;
-import internal.org.springframework.content.commons.config.StoreFragments;
-import lombok.EqualsAndHashCode;
-import lombok.Getter;
-import lombok.Setter;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.BeforeEach;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.Context;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.Describe;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.It;
+import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.JustBeforeEach;
+import static org.hamcrest.CoreMatchers.instanceOf;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.isA;
+import static org.hamcrest.CoreMatchers.not;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.nullValue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyObject;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+import static org.mockito.hamcrest.MockitoHamcrest.argThat;
+
+import java.io.ByteArrayInputStream;
+import java.io.InputStream;
+import java.io.Serializable;
+import java.lang.reflect.Method;
+import java.nio.file.Paths;
+import java.util.Collections;
+import java.util.Map;
+import java.util.UUID;
+
 import org.aopalliance.intercept.MethodInvocation;
 import org.apache.commons.io.IOUtils;
 import org.junit.runner.RunWith;
@@ -20,31 +43,32 @@
 import org.springframework.content.commons.repository.ContentStore;
 import org.springframework.content.commons.repository.Store;
 import org.springframework.content.commons.repository.StoreExtension;
-import org.springframework.content.commons.repository.events.*;
+import org.springframework.content.commons.repository.events.AfterAssociateEvent;
+import org.springframework.content.commons.repository.events.AfterGetContentEvent;
+import org.springframework.content.commons.repository.events.AfterGetResourceEvent;
+import org.springframework.content.commons.repository.events.AfterSetContentEvent;
+import org.springframework.content.commons.repository.events.AfterUnassociateEvent;
+import org.springframework.content.commons.repository.events.AfterUnsetContentEvent;
+import org.springframework.content.commons.repository.events.BeforeAssociateEvent;
+import org.springframework.content.commons.repository.events.BeforeGetContentEvent;
+import org.springframework.content.commons.repository.events.BeforeGetResourceEvent;
+import org.springframework.content.commons.repository.events.BeforeSetContentEvent;
+import org.springframework.content.commons.repository.events.BeforeUnassociateEvent;
+import org.springframework.content.commons.repository.events.BeforeUnsetContentEvent;
 import org.springframework.context.ApplicationEventPublisher;
 import org.springframework.core.io.InputStreamResource;
 import org.springframework.core.io.Resource;
 import org.springframework.security.util.SimpleMethodInvocation;
 import org.springframework.util.ReflectionUtils;
 
-import java.io.ByteArrayInputStream;
-import java.io.InputStream;
-import java.io.Serializable;
-import java.lang.reflect.Method;
-import java.util.Collections;
-import java.util.Map;
-import java.util.UUID;
+import com.github.paulcwarren.ginkgo4j.Ginkgo4jConfiguration;
+import com.github.paulcwarren.ginkgo4j.Ginkgo4jRunner;
 
-import static com.github.paulcwarren.ginkgo4j.Ginkgo4jDSL.*;
-import static org.hamcrest.CoreMatchers.isA;
-import static org.hamcrest.CoreMatchers.*;
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.nullValue;
-import static org.mockito.Matchers.anyObject;
-import static org.mockito.Matchers.eq;
-import static org.mockito.Mockito.any;
-import static org.mockito.Mockito.*;
-import static org.mockito.hamcrest.MockitoHamcrest.argThat;
+import internal.org.springframework.content.commons.config.StoreFragment;
+import internal.org.springframework.content.commons.config.StoreFragments;
+import lombok.EqualsAndHashCode;
+import lombok.Getter;
+import lombok.Setter;
 
 @SuppressWarnings("unchecked")
 @RunWith(Ginkgo4jRunner.class)
@@ -96,7 +120,7 @@ public class StoreMethodInterceptorTest {
 
 			JustBeforeEach(() -> {
 				interceptor = new StoreMethodInterceptor();
-				StoreFragments fragments = new StoreFragments(Collections.singletonList(new StoreFragment(TestContentStore.class, new StoreImpl(store, publisher))));
+				StoreFragments fragments = new StoreFragments(Collections.singletonList(new StoreFragment(TestContentStore.class, new StoreImpl(store, publisher, Paths.get(System.getProperty("java.io.tmpdir"))))));
 				interceptor.setStoreFragments(fragments);
 				try {
 					interceptor.invoke(invocation);
@@ -479,7 +503,7 @@ public class StoreMethodInterceptorTest {
 				try {
 					Method m = ReflectionUtils.findMethod(TestContentStore.class, "unsetContent", Object.class);
 					assertThat(m, is(not(nullValue())));
-					Method actual = interceptor.getMethod(m, new StoreFragment(TestContentStore.class, new StoreImpl(store, publisher)));
+					Method actual = interceptor.getMethod(m, new StoreFragment(TestContentStore.class, new StoreImpl(store, publisher, Paths.get(System.getProperty("java.io.tmpdir")))));
 					assertThat(actual, is(ReflectionUtils.findMethod(StoreImpl.class, "unsetContent", Object.class)));
 				}
 				catch (Exception invokeException) {
@@ -494,7 +518,7 @@ public class StoreMethodInterceptorTest {
 				try {
 					Method m = ReflectionUtils.findMethod(TestContentStore.class, "setContent", TEntity.class, InputStream.class);
 					assertThat(m, is(not(nullValue())));
-					Method actual = interceptor.getMethod(m, new StoreFragment(TestContentStore.class, new StoreImpl(store, publisher)));
+					Method actual = interceptor.getMethod(m, new StoreFragment(TestContentStore.class, new StoreImpl(store, publisher, Paths.get(System.getProperty("java.io.tmpdir")))));
 					assertThat(actual, is(ReflectionUtils.findMethod(StoreImpl.class, "setContent", Object.class, InputStream.class)));
 				}
 				catch (Exception invokeException) {