Enhance release artifact handling andimproved ZIP validation

src/main/java/org/frankframework/insights/release/ReleaseArtifactService.java

@@ -4,6 +4,7 @@
 import java.io.InputStream;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.nio.file.AtomicMoveNotSupportedException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
@@ -12,6 +13,7 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
+import java.util.zip.ZipFile;
 import lombok.extern.slf4j.Slf4j;
 import org.frankframework.insights.common.util.TagNameSanitizer;
 import org.frankframework.insights.release.releasecleanup.FileTreeDeleter;
@@ -44,24 +46,56 @@
         Path zipPath = archiveDir.resolve(safeFileName);
 
         if (Files.exists(zipPath)) {
-            return zipPath;
+            if (isValidZip(zipPath)) {
+                return zipPath;
+            }
+            log.warn(
+                    "Cached ZIP for {} is corrupt (likely a truncated download), re-downloading: {}", tagName, zipPath);
+            Files.deleteIfExists(zipPath);
         }
 
         log.info("ZIP not found op storage, downloading: {}", tagName);
         String url = String.format(GITHUB_ZIP_URL_FORMAT, tagName);
-        try (InputStream in = new URI(url).toURL().openStream()) {
-            Files.copy(in, zipPath, StandardCopyOption.REPLACE_EXISTING);
+        Path tempZipPath = archiveDir.resolve(safeFileName + ".tmp");
+        try (InputStream in = openDownloadStream(url)) {
+            Files.copy(in, tempZipPath, StandardCopyOption.REPLACE_EXISTING);
+
+            if (!isValidZip(tempZipPath)) {
+                throw new IOException("Downloaded ZIP for " + tagName + " is not a valid archive");
+            }
+
+            moveIntoPlace(tempZipPath, zipPath);
             log.info("ZIP succesfully downloading to storage: {}", zipPath);
         } catch (IOException | URISyntaxException e) {
             try {
-                Files.deleteIfExists(zipPath);
+                Files.deleteIfExists(tempZipPath);
             } catch (IOException ignored) {
             }
             throw new IOException("Could not download release ZIP for " + tagName, e);
         }
         return zipPath;
     }
 
+    private void moveIntoPlace(Path tempZipPath, Path zipPath) throws IOException {
+        try {
+            Files.move(tempZipPath, zipPath, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
+        } catch (AtomicMoveNotSupportedException e) {
+            Files.move(tempZipPath, zipPath, StandardCopyOption.REPLACE_EXISTING);
+        }
+    }
+
+    protected InputStream openDownloadStream(String url) throws IOException, URISyntaxException {
+        return new URI(url).toURL().openStream();
+    }
+
+    protected boolean isValidZip(Path path) {
+        try (ZipFile ignored = new ZipFile(path.toFile())) {
+            return true;
+        } catch (IOException _) {
+            return false;
+        }
+    }
+
     public void deleteObsoleteReleaseArtifacts() {
         List<Release> allReleases = releaseRepository.findAll();
         Set<String> activeReleaseTags =
@@ -73,18 +107,19 @@
         try (Stream<Path> files = Files.list(dir)) {
             files.forEach(path -> {
                 String fileName = path.getFileName().toString();
-                if (fileName.endsWith(".zip")) {
-                    String tagName = fileName.replace(".zip", "");
-                    if (!activeReleaseTags.contains(tagName)) {
-                        try {
-                            log.info("Deletion of obsolete release artifact: {}", fileName);
-                            fileTreeDeleter.deleteTreeRecursively(path);
-                        } catch (InterruptedException e) {
-                            Thread.currentThread().interrupt();
-                            log.error("Interrupted while deleting file: {}", fileName, e);
-                        } catch (Exception e) {
-                            log.error("Could not delete file: {}", fileName, e);
-                        }
+                boolean isOrphanedTempFile = fileName.endsWith(".tmp");
+                boolean isObsoleteZip =
+                        fileName.endsWith(".zip") && !activeReleaseTags.contains(fileName.replace(".zip", ""));
+
+                if (isOrphanedTempFile || isObsoleteZip) {
+                    try {
+                        log.info("Deletion of obsolete release artifact: {}", fileName);
+                        fileTreeDeleter.deleteTreeRecursively(path);
+                    } catch (InterruptedException e) {
+                        Thread.currentThread().interrupt();
+                        log.error("Interrupted while deleting file: {}", fileName, e);
+                    } catch (Exception e) {
+                        log.error("Could not delete file: {}", fileName, e);
                     }
                 }
             });

src/test/java/org/frankframework/insights/release/ReleaseArtifactServiceIntegrationTest.java

@@ -2,18 +2,28 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.when;
 
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.InputStream;
+import java.net.URISyntaxException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.List;
+import java.util.zip.ZipEntry;
+import java.util.zip.ZipOutputStream;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.io.TempDir;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.junit.jupiter.MockitoExtension;
 
 /**
@@ -32,7 +42,7 @@ public class ReleaseArtifactServiceIntegrationTest {
 
     @BeforeEach
     public void setUp() {
-        releaseArtifactService = new ReleaseArtifactService(tempDir.toString(), releaseRepository);
+        releaseArtifactService = Mockito.spy(new ReleaseArtifactService(tempDir.toString(), releaseRepository));
     }
 
     private Release createRelease(String name, String tagName) {
@@ -42,6 +52,78 @@ private Release createRelease(String name, String tagName) {
         return release;
     }
 
+    private byte[] createValidZipBytes() throws IOException {
+        ByteArrayOutputStream baos = new ByteArrayOutputStream();
+        try (ZipOutputStream zos = new ZipOutputStream(baos)) {
+            zos.putNextEntry(new ZipEntry("test.txt"));
+            zos.write("hello world".getBytes());
+            zos.closeEntry();
+        }
+        return baos.toByteArray();
+    }
+
+    @Test
+    public void downloadReleaseZipToPvc_whenCorruptFileInCache_shouldRedownloadAndReplace() throws Exception {
+        String tagName = "v8.0.5";
+        Path corruptZip = tempDir.resolve("v8.0.5.zip");
+        Files.writeString(corruptZip, "this is not a zip file");
+
+        InputStream mockStream = new ByteArrayInputStream(createValidZipBytes());
+        doReturn(mockStream).when(releaseArtifactService).openDownloadStream(any());
+
+        Path downloaded = releaseArtifactService.downloadReleaseZipToPvc(tagName);
+
+        assertTrue(Files.exists(downloaded), "Downloaded zip should exist");
+        assertTrue(releaseArtifactService.isValidZip(downloaded), "Re-downloaded zip should be valid");
+        assertFalse(
+                Files.exists(tempDir.resolve("v8.0.5.zip.tmp")), "Temp file should not remain after a successful move");
+    }
+
+    @Test
+    public void downloadReleaseZipToPvc_whenDownloadIsCorrupt_shouldThrowAndCleanUpTempFile() throws Exception {
+        String tagName = "v8.0.6";
+        Path zip = tempDir.resolve("v8.0.6.zip");
+        Path tempZip = tempDir.resolve("v8.0.6.zip.tmp");
+
+        InputStream corruptStream = new ByteArrayInputStream("bad download".getBytes());
+        doReturn(corruptStream).when(releaseArtifactService).openDownloadStream(any());
+
+        assertThrows(IOException.class, () -> releaseArtifactService.downloadReleaseZipToPvc(tagName));
+
+        assertFalse(Files.exists(tempZip), "Temporary download file should be cleaned up on failure");
+        assertFalse(Files.exists(zip), "Invalid download should never be cached under its final name");
+    }
+
+    @Test
+    public void deleteObsoleteReleaseArtifacts_whenOrphanedTempFilesExist_shouldDeleteThem() throws IOException {
+        Path orphanedTemp = tempDir.resolve("v8.0.5.zip.tmp");
+        Path validZip = tempDir.resolve("v8.0.0.zip");
+
+        Files.writeString(orphanedTemp, "leftover temp");
+        Files.writeString(validZip, "valid zip content");
+
+        Release validRelease = createRelease("8.0.0", "v8.0.0");
+        when(releaseRepository.findAll()).thenReturn(List.of(validRelease));
+
+        releaseArtifactService.deleteObsoleteReleaseArtifacts();
+
+        assertFalse(Files.exists(orphanedTemp), "Orphaned temp file should be deleted");
+        assertTrue(Files.exists(validZip), "Valid active zip should still exist");
+    }
+
+    @Test
+    public void downloadReleaseZipToPvc_whenValidZipAlreadyCached_shouldReturnItWithoutDownloading()
+            throws IOException, URISyntaxException {
+        String tagName = "v8.1.0";
+        Path zipPath = tempDir.resolve("v8.1.0.zip");
+        Files.write(zipPath, createValidZipBytes());
+
+        Path result = releaseArtifactService.downloadReleaseZipToPvc(tagName);
+
+        assertEquals(zipPath, result);
+        Mockito.verify(releaseArtifactService, Mockito.never()).openDownloadStream(any());
+    }
+
     @Test
     public void deleteObsoleteReleaseArtifacts_whenObsoleteZipsExist_shouldDeleteThem() throws IOException {
         Path validZip = tempDir.resolve("v8.0.0.zip");

src/test/java/org/frankframework/insights/release/ReleaseArtifactServiceTest.java

@@ -1,14 +1,21 @@
 package org.frankframework.insights.release;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.when;
 
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.io.InputStream;
+import java.nio.file.AtomicMoveNotSupportedException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.nio.file.StandardCopyOption;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.stream.Stream;
@@ -35,8 +42,9 @@
 
     @BeforeEach
     public void setUp() {
-        releaseArtifactService = new ReleaseArtifactService("/release-archive", releaseRepository);
+        releaseArtifactService = Mockito.spy(new ReleaseArtifactService("/release-archive", releaseRepository));
         mockedFiles = Mockito.mockStatic(Files.class);
+       Mockito.lenient().doReturn(true).when(releaseArtifactService).isValidZip(any());
     }
 
     @AfterEach
@@ -553,4 +561,101 @@
         Assertions.assertTrue(result.toString().endsWith(".zip"));
         Assertions.assertTrue(result.toString().contains(tagName));
     }
+
+    @Test
+    public void downloadReleaseZipToPvc_whenValidZipAlreadyExists_shouldReturnExistingPath() throws IOException {
+        String tagName = "v7.8.0";
+        Path zipPath = ARCHIVE_DIR.resolve(tagName + ".zip");
+
+        mockedFiles.when(() -> Files.exists(ARCHIVE_DIR)).thenReturn(true);
+        mockedFiles.when(() -> Files.exists(zipPath)).thenReturn(true);
+        doReturn(true).when(releaseArtifactService).isValidZip(zipPath);
+
+        Path result = releaseArtifactService.downloadReleaseZipToPvc(tagName);
+
+        assertEquals(zipPath, result);
+    }
+
+    @Test
+    public void downloadReleaseZipToPvc_whenCachedZipIsCorrupt_shouldDeleteAndRedownload() throws Exception {
+        String tagName = "v8.0.5";
+        Path zipPath = ARCHIVE_DIR.resolve(tagName + ".zip");
+        Path tempZipPath = ARCHIVE_DIR.resolve(tagName + ".zip.tmp");
+
+        mockedFiles.when(() -> Files.exists(ARCHIVE_DIR)).thenReturn(true);
+        mockedFiles.when(() -> Files.exists(zipPath)).thenReturn(true);
+
+        doReturn(false).when(releaseArtifactService).isValidZip(zipPath);
+        doReturn(true).when(releaseArtifactService).isValidZip(tempZipPath);
+
+        InputStream dummyStream = new ByteArrayInputStream(new byte[] {1, 2, 3});
+        doReturn(dummyStream).when(releaseArtifactService).openDownloadStream(any());
+
+        Path result = releaseArtifactService.downloadReleaseZipToPvc(tagName);
+
+        assertEquals(zipPath, result);
+        mockedFiles.verify(() -> Files.deleteIfExists(zipPath));
+    }
+
+    @Test
+    public void downloadReleaseZipToPvc_whenDownloadedZipIsInvalid_shouldThrowAndCleanUpTempFile() throws Exception {
+        String tagName = "v8.0.6";
+        Path tempZipPath = ARCHIVE_DIR.resolve(tagName + ".zip.tmp");
+
+        mockedFiles.when(() -> Files.exists(ARCHIVE_DIR)).thenReturn(true);
+
+        InputStream dummyStream = new ByteArrayInputStream(new byte[] {1, 2, 3});
+        doReturn(dummyStream).when(releaseArtifactService).openDownloadStream(any());
+        doReturn(false).when(releaseArtifactService).isValidZip(tempZipPath);
+
+        assertThrows(IOException.class, () -> releaseArtifactService.downloadReleaseZipToPvc(tagName));
+        mockedFiles.verify(() -> Files.deleteIfExists(tempZipPath));
+    }
+
+    @Test
+    public void downloadReleaseZipToPvc_whenAtomicMoveUnsupported_shouldFallBackToPlainMove() throws Exception {
+        String tagName = "v8.0.7";
+        Path zipPath = ARCHIVE_DIR.resolve(tagName + ".zip");
+        Path tempZipPath = ARCHIVE_DIR.resolve(tagName + ".zip.tmp");
+
+        mockedFiles.when(() -> Files.exists(ARCHIVE_DIR)).thenReturn(true);
+
+        InputStream dummyStream = new ByteArrayInputStream(new byte[] {1, 2, 3});
+        doReturn(dummyStream).when(releaseArtifactService).openDownloadStream(any());
+        doReturn(true).when(releaseArtifactService).isValidZip(tempZipPath);
+
+        mockedFiles
+                .when(() -> Files.move(
+                        eq(tempZipPath),
+                        eq(zipPath),
+                        eq(StandardCopyOption.REPLACE_EXISTING),
+                        eq(StandardCopyOption.ATOMIC_MOVE)))
+                .thenThrow(new AtomicMoveNotSupportedException(
+                        tempZipPath.toString(), zipPath.toString(), "not supported on this filesystem"));
+        mockedFiles
+                .when(() -> Files.move(eq(tempZipPath), eq(zipPath), eq(StandardCopyOption.REPLACE_EXISTING)))
+                .thenReturn(zipPath);
+
+        Path result = releaseArtifactService.downloadReleaseZipToPvc(tagName);
+
+        assertEquals(zipPath, result);
+        mockedFiles.verify(() -> Files.move(eq(tempZipPath), eq(zipPath), eq(StandardCopyOption.REPLACE_EXISTING)));
+    }
+
+    @Test
+    public void deleteObsoleteReleaseArtifacts_whenOrphanedTempFileExists_shouldAttemptToDeleteIt() throws IOException {
+        Release release = createRelease("8.0.0", "v8.0.0");
+        List<Release> releases = List.of(release);
+
+        Path activeZip = ARCHIVE_DIR.resolve("v8.0.0.zip");
+        Path orphanedTempFile = ARCHIVE_DIR.resolve("v7.0.0.zip.tmp");
+
+        mockedFiles.when(() -> Files.exists(ARCHIVE_DIR)).thenReturn(true);
+        when(releaseRepository.findAll()).thenReturn(releases);
+        mockedFiles.when(() -> Files.list(ARCHIVE_DIR)).thenReturn(Stream.of(activeZip, orphanedTempFile));
+
+        releaseArtifactService.deleteObsoleteReleaseArtifacts();
+
+        mockedFiles.verify(() -> Files.list(ARCHIVE_DIR));
+    }
 }