From ef1600834aaa5835e5a78d7afa28b89ac442ae36 Mon Sep 17 00:00:00 2001 From: Elimelec Burghelea Date: Thu, 27 Feb 2025 00:38:45 +0200 Subject: [PATCH 1/2] Add tests Signed-off-by: Elimelec Burghelea --- .../support/AbstractFileItemWriterTest.java | 76 +++++++++++++++++++ .../batch/item/util/FileUtilsTests.java | 42 +++++++++- .../item/xml/StaxEventItemWriterTests.java | 27 ++++++- 3 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java new file mode 100644 index 0000000000..5ab1a09290 --- /dev/null +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java @@ -0,0 +1,76 @@ +/* + * Copyright 2025 the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.springframework.batch.item.support; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.Mockito.when; + +import java.io.File; + +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; +import org.springframework.batch.item.Chunk; +import org.springframework.batch.item.ExecutionContext; +import org.springframework.batch.item.ItemStreamException; +import org.springframework.core.io.FileSystemResource; + +/** + * Tests for common methods from {@link AbstractFileItemWriter}. + * + * @author Elimelec Burghelea + */ +class AbstractFileItemWriterTests { + + @Test + void testFailedFileDeletionThrowsException() { + File outputFile = new File("target/data/output.tmp"); + File mocked = Mockito.spy(outputFile); + + TestFileItemWriter writer = new TestFileItemWriter(); + + writer.setResource(new FileSystemResource(mocked)); + writer.setShouldDeleteIfEmpty(true); + writer.setName(writer.getClass().getSimpleName()); + writer.open(new ExecutionContext()); + + when(mocked.delete()).thenReturn(false); + + ItemStreamException exception = assertThrows(ItemStreamException.class, writer::close, + "Expected exception when file deletion fails"); + + assertEquals("Failed to delete empty file on close", exception.getMessage(), "Wrong exception message"); + // FIXME: update after fix, because we will have a reason + assertNull(exception.getCause(), "Exception should not have a cause"); + } + + private static class TestFileItemWriter extends AbstractFileItemWriter { + + @Override + protected String doWrite(Chunk items) { + return String.join("\n", items); + } + + @Override + public void afterPropertiesSet() { + + } + + } + +} \ No newline at end of file diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java index 311ef986ba..b81496c3b1 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2022 the original author or authors. + * Copyright 2008-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -27,6 +27,7 @@ import org.springframework.batch.item.ItemStreamException; import org.springframework.util.Assert; +import static org.junit.Assert.assertNull; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -36,6 +37,7 @@ * Tests for {@link FileUtils} * * @author Robert Kasanicky + * @author Elimelec Burghelea */ class FileUtilsTests { @@ -178,6 +180,44 @@ public boolean exists() { } } + @Test + void testCannotDeleteFile() { + + File file = new File("new file") { + + @Override + public boolean createNewFile() { + return true; + } + + @Override + public boolean exists() { + return true; + } + + @Override + public boolean delete() { + return false; + } + + }; + try { + FileUtils.setUpOutputFile(file, false, false, true); + fail("Expected ItemStreamException because file cannot be deleted"); + } + catch (ItemStreamException ex) { + String message = ex.getMessage(); + assertTrue(message.startsWith("Unable to create file"), "Wrong message: " + message); + assertTrue(ex.getCause() instanceof IOException); + assertTrue(ex.getCause().getMessage().startsWith("Could not delete file"), "Wrong message: " + message); + // FIXME: update after fix, because we will have a reason + assertNull(ex.getCause().getCause()); + } + finally { + file.delete(); + } + } + @BeforeEach void setUp() { file.delete(); diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java index f904c59441..d996cf8f0f 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2008-2023 the original author or authors. + * Copyright 2008-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -30,6 +30,7 @@ import org.springframework.batch.item.Chunk; import org.springframework.batch.item.ExecutionContext; +import org.springframework.batch.item.ItemStreamException; import org.springframework.batch.item.UnexpectedInputException; import org.springframework.batch.item.WriterNotOpenException; import org.springframework.batch.support.transaction.ResourcelessTransactionManager; @@ -47,9 +48,11 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; /** @@ -57,6 +60,7 @@ * * @author Parikshit Dutta * @author Mahmoud Ben Hassine + * @author Elimelec Burghelea */ class StaxEventItemWriterTests { @@ -831,6 +835,27 @@ void testOpenAndCloseTagsInComplexCallbacksRestart() throws Exception { + "", content, "Wrong content: " + content); } + /** + * Tests that if file.delete() returns false, an appropriate exception is thrown to + * indicate the deletion attempt failed. + */ + @Test + void testFailedFileDeletionThrowsException() throws IOException { + File mockedFile = spy(resource.getFile()); + writer.setResource(new FileSystemResource(mockedFile)); + writer.setShouldDeleteIfEmpty(true); + writer.open(executionContext); + + when(mockedFile.delete()).thenReturn(false); + + ItemStreamException exception = assertThrows(ItemStreamException.class, () -> writer.close(), + "Expected exception when file deletion fails"); + + assertEquals("Failed to delete empty file on close", exception.getMessage(), "Wrong exception message"); + // FIXME: update after fix, because we will have a reason + assertNull(exception.getCause(), "Exception should not have a cause"); + } + private void initWriterForSimpleCallbackTests() throws Exception { writer = createItemWriter(); writer.setHeaderCallback(writer -> { From cb87c5e29cbd7a1cde8f68670c9ed85ba3cb9b97 Mon Sep 17 00:00:00 2001 From: Elimelec Burghelea Date: Thu, 27 Feb 2025 23:04:10 +0200 Subject: [PATCH 2/2] Use Files.delete() for better error reporting Signed-off-by: Elimelec Burghelea --- .../batch/item/support/AbstractFileItemWriter.java | 10 +++++----- .../springframework/batch/item/util/FileUtils.java | 11 ++++++++--- .../batch/item/xml/StaxEventItemWriter.java | 10 +++++----- .../item/support/AbstractFileItemWriterTest.java | 5 ++--- .../batch/item/util/FileUtilsTests.java | 5 ++--- .../batch/item/xml/StaxEventItemWriterTests.java | 5 ++--- 6 files changed, 24 insertions(+), 22 deletions(-) diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/support/AbstractFileItemWriter.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/support/AbstractFileItemWriter.java index 0396ca8cc7..c3d3a00bbb 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/support/AbstractFileItemWriter.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/support/AbstractFileItemWriter.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2023 the original author or authors. + * Copyright 2006-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -25,6 +25,7 @@ import java.nio.channels.FileChannel; import java.nio.charset.StandardCharsets; import java.nio.charset.UnsupportedCharsetException; +import java.nio.file.Files; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; @@ -61,6 +62,7 @@ * @author Mahmoud Ben Hassine * @author Glenn Renfro * @author Remi Kaeffer + * @author Elimelec Burghelea * @since 4.1 */ public abstract class AbstractFileItemWriter extends AbstractItemStreamItemWriter @@ -268,11 +270,9 @@ public void close() { state.close(); if (state.linesWritten == 0 && shouldDeleteIfEmpty) { try { - if (!resource.getFile().delete()) { - throw new ItemStreamException("Failed to delete empty file on close"); - } + Files.delete(resource.getFile().toPath()); } - catch (IOException e) { + catch (IOException | SecurityException e) { throw new ItemStreamException("Failed to delete empty file on close", e); } } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/FileUtils.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/FileUtils.java index 1b82ae1634..c14d9470b3 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/FileUtils.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/util/FileUtils.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2024 the original author or authors. + * Copyright 2006-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -18,6 +18,7 @@ import java.io.File; import java.io.IOException; +import java.nio.file.Files; import org.springframework.batch.item.ItemStreamException; import org.springframework.util.Assert; @@ -28,6 +29,7 @@ * @author Peter Zozom * @author Mahmoud Ben Hassine * @author Taeik Lim + * @author Elimelec Burghelea */ public abstract class FileUtils { @@ -57,8 +59,11 @@ public static void setUpOutputFile(File file, boolean restarted, boolean append, if (!overwriteOutputFile) { throw new ItemStreamException("File already exists: [" + file.getAbsolutePath() + "]"); } - if (!file.delete()) { - throw new IOException("Could not delete file: " + file); + try { + Files.delete(file.toPath()); + } + catch (IOException | SecurityException e) { + throw new IOException("Could not delete file: " + file, e); } } diff --git a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/xml/StaxEventItemWriter.java b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/xml/StaxEventItemWriter.java index fef239f809..2c6e803773 100644 --- a/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/xml/StaxEventItemWriter.java +++ b/spring-batch-infrastructure/src/main/java/org/springframework/batch/item/xml/StaxEventItemWriter.java @@ -1,5 +1,5 @@ /* - * Copyright 2006-2023 the original author or authors. + * Copyright 2006-2025 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -24,6 +24,7 @@ import java.io.UnsupportedEncodingException; import java.io.Writer; import java.nio.channels.FileChannel; +import java.nio.file.Files; import java.util.Collections; import java.util.List; import java.util.Map; @@ -75,6 +76,7 @@ * @author Michael Minella * @author Parikshit Dutta * @author Mahmoud Ben Hassine + * @author Elimelec Burghelea */ public class StaxEventItemWriter extends AbstractItemStreamItemWriter implements ResourceAwareItemWriterItemStream, InitializingBean { @@ -726,11 +728,9 @@ public void close() { } if (currentRecordCount == 0 && shouldDeleteIfEmpty) { try { - if (!resource.getFile().delete()) { - throw new ItemStreamException("Failed to delete empty file on close"); - } + Files.delete(resource.getFile().toPath()); } - catch (IOException e) { + catch (IOException | SecurityException e) { throw new ItemStreamException("Failed to delete empty file on close", e); } } diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java index 5ab1a09290..aacc67e716 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/support/AbstractFileItemWriterTest.java @@ -17,7 +17,7 @@ package org.springframework.batch.item.support; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.when; @@ -55,8 +55,7 @@ void testFailedFileDeletionThrowsException() { "Expected exception when file deletion fails"); assertEquals("Failed to delete empty file on close", exception.getMessage(), "Wrong exception message"); - // FIXME: update after fix, because we will have a reason - assertNull(exception.getCause(), "Exception should not have a cause"); + assertNotNull(exception.getCause(), "Exception should have a cause"); } private static class TestFileItemWriter extends AbstractFileItemWriter { diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java index b81496c3b1..6faae21e61 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/util/FileUtilsTests.java @@ -27,8 +27,8 @@ import org.springframework.batch.item.ItemStreamException; import org.springframework.util.Assert; -import static org.junit.Assert.assertNull; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -210,8 +210,7 @@ public boolean delete() { assertTrue(message.startsWith("Unable to create file"), "Wrong message: " + message); assertTrue(ex.getCause() instanceof IOException); assertTrue(ex.getCause().getMessage().startsWith("Could not delete file"), "Wrong message: " + message); - // FIXME: update after fix, because we will have a reason - assertNull(ex.getCause().getCause()); + assertNotNull(ex.getCause().getCause(), "Exception should have a cause"); } finally { file.delete(); diff --git a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java index d996cf8f0f..08fada774e 100644 --- a/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java +++ b/spring-batch-infrastructure/src/test/java/org/springframework/batch/item/xml/StaxEventItemWriterTests.java @@ -48,7 +48,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; @@ -852,8 +852,7 @@ void testFailedFileDeletionThrowsException() throws IOException { "Expected exception when file deletion fails"); assertEquals("Failed to delete empty file on close", exception.getMessage(), "Wrong exception message"); - // FIXME: update after fix, because we will have a reason - assertNull(exception.getCause(), "Exception should not have a cause"); + assertNotNull(exception.getCause(), "Exception should have a cause"); } private void initWriterForSimpleCallbackTests() throws Exception {