fix: remove file from fileMap in online generator

.github/PULL_REQUEST_TEMPLATE.md

@@ -3,7 +3,6 @@
 - [ ] Read the [contribution guidelines](https://github.com/swagger-api/swagger-codegen/blob/master/CONTRIBUTING.md).
 - [ ] Ran the shell script under `./bin/` to update Petstore sample so that CIs can verify the change. (For instance, only need to run `./bin/{LANG}-petstore.sh` and `./bin/security/{LANG}-petstore.sh` if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in `.\bin\windows\`.
 - [ ] Filed the PR against the correct branch: `3.0.0` branch for changes related to OpenAPI spec 3.0. Default: `master`.
-- [ ] Copied the [technical committee](https://github.com/swagger-api/swagger-codegen/#swagger-codegen-technical-committee) to review the pull request if your PR is targeting a particular programming language.
 
 ### Description of the PR
 

modules/swagger-generator/pom.xml

@@ -146,7 +146,6 @@
                         <configuration>
                             <url>https://github.com/swagger-api/swagger-ui/archive/master.tar.gz</url>
                             <unpack>true</unpack>
-                            <!--<skipCache>true</skipCache>-->
                             <outputDirectory>${project.build.directory}</outputDirectory>
                         </configuration>
                     </execution>
@@ -327,6 +326,18 @@
             <version>${junit-version}</version>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>
+            <version>5.20.0</version>
+            <scope>test</scope>
+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-inline</artifactId>
+            <version>5.2.0</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
     <properties>
         <servlet-api-version>2.5</servlet-api-version>

modules/swagger-generator/src/main/java/io/swagger/generator/resource/SwaggerResource.java

@@ -3,34 +3,50 @@
 import io.swagger.annotations.Api;
 import io.swagger.annotations.ApiOperation;
 import io.swagger.annotations.ApiParam;
+import io.swagger.annotations.ApiResponse;
+import io.swagger.annotations.ApiResponses;
 import io.swagger.codegen.CliOption;
 import io.swagger.codegen.Codegen;
 import io.swagger.codegen.CodegenConfig;
 import io.swagger.codegen.CodegenType;
 import io.swagger.codegen.utils.SecureFileUtils;
+import io.swagger.generator.exception.ApiException;
 import io.swagger.generator.exception.BadRequestException;
 import io.swagger.generator.model.Generated;
 import io.swagger.generator.model.GeneratorInput;
 import io.swagger.generator.model.ResponseCode;
 import io.swagger.generator.online.Generator;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.lang3.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import javax.servlet.http.HttpServletRequest;
-import javax.ws.rs.*;
+import javax.ws.rs.GET;
+import javax.ws.rs.POST;
+import javax.ws.rs.Path;
+import javax.ws.rs.PathParam;
+import javax.ws.rs.Produces;
 import javax.ws.rs.core.Context;
 import javax.ws.rs.core.MediaType;
 import javax.ws.rs.core.Response;
 import java.io.File;
-import java.util.*;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.UUID;
 
 @Path("/gen")
-@Api(value = "/gen", description = "Resource for generating swagger components")
+@Api(value = "/gen")
 @SuppressWarnings("static-method")
 public class SwaggerResource {
-    static List<String> clients = new ArrayList<String>();
-    static List<String> servers = new ArrayList<String>();
-    private static Map<String, Generated> fileMap = new HashMap<String, Generated>();
+    private static final Logger LOGGER = LoggerFactory.getLogger(SwaggerResource.class);
+
+    static List<String> clients = new ArrayList<>();
+    static List<String> servers = new ArrayList<>();
+    private static Map<String, Generated> fileMap = new HashMap<>();
 
     static {
         List<CodegenConfig> extensions = Codegen.getExtensions();
@@ -43,8 +59,8 @@ public class SwaggerResource {
             }
         }
 
-        Collections.sort(clients, String.CASE_INSENSITIVE_ORDER);
-        Collections.sort(servers, String.CASE_INSENSITIVE_ORDER);
+        clients.sort(String.CASE_INSENSITIVE_ORDER);
+        servers.sort(String.CASE_INSENSITIVE_ORDER);
     }
 
     @GET
@@ -55,20 +71,21 @@ public class SwaggerResource {
             notes = "A valid `fileId` is generated by the `/clients/{language}` or `/servers/{language}` POST "
                     + "operations.  The fileId code can be used just once, after which a new `fileId` will need to "
                     + "be requested.", response = String.class, tags = {"clients", "servers"})
-    public Response downloadFile(@PathParam("fileId") String fileId) throws Exception {
+    @ApiResponses(value = {
+            @ApiResponse(code = 200, message = "File successfully downloaded. Response contains ZIP file bytes."),
+            @ApiResponse(code = 404, message = "File with the given fileId not found or already downloaded."),
+            @ApiResponse(code = 500, message = "Server error while reading or returning the file.")
+    })
+    public Response downloadFile(@PathParam("fileId") String fileId) {
         Generated g = fileMap.get(fileId);
-        System.out.println("looking for fileId " + fileId);
-        System.out.println("got filename " + g.getFilename());
-        if (g.getFilename() != null) {
+        LOGGER.info("Looking for fileId: {}", fileId);
+        if (g != null && g.getFilename() != null) {
+            LOGGER.info("Got filename: {}", g.getFilename());
             SecureFileUtils.validatePath(g.getFilename());
-            File file = new java.io.File(g.getFilename());
-            byte[] bytes = org.apache.commons.io.FileUtils.readFileToByteArray(file);
+            final File file = new java.io.File(g.getFilename());
 
-            try {
-                FileUtils.deleteDirectory(file.getParentFile());
-            } catch (Exception e) {
-                System.out.println("failed to delete file " + file.getAbsolutePath());
-            }
+            byte[] bytes = getFileBytes(file);
+            removeFile(fileId, file);
 
             return Response
                     .ok(bytes, "application/zip")
@@ -80,6 +97,23 @@ public Response downloadFile(@PathParam("fileId") String fileId) throws Exceptio
         }
     }
 
+    private static byte[] getFileBytes(File file) {
+        try {
+            return FileUtils.readFileToByteArray(file);
+        } catch (IOException e) {
+            throw new IllegalStateException("Cannot read the file: " + file.getAbsolutePath(), e);
+        }
+    }
+
+    private static void removeFile(String fileId, File file) {
+        try {
+            FileUtils.deleteDirectory(file.getParentFile());
+            fileMap.remove(fileId);
+        } catch (Exception e) {
+            LOGGER.error("Failed to delete file: {} ", file.getAbsolutePath());
+        }
+    }
+
     @POST
     @Path("/clients/{language}")
     @ApiOperation(
@@ -90,7 +124,7 @@ public Response generateClient(
             @Context HttpServletRequest request,
             @ApiParam(value = "The target language for the client library", required = true) @PathParam("language") String language,
             @ApiParam(value = "Configuration for building the client library", required = true) GeneratorInput opts)
-            throws Exception {
+            throws ApiException {
 
         String filename = Generator.generateClient(language, opts);
         String host = getHost(request);
@@ -101,7 +135,6 @@ public Response generateClient(
             g.setFilename(filename);
             g.setFriendlyName(language + "-client");
             fileMap.put(code, g);
-            System.out.println(code + ", " + filename);
             String link = host + "/api/gen/download/" + code;
             return Response.ok().entity(new ResponseCode(code, link)).build();
         } else {
@@ -117,7 +150,7 @@ public Response generateClient(
     public Response getClientOptions(
             @SuppressWarnings("unused") @Context HttpServletRequest request,
             @ApiParam(value = "The target language for the client library", required = true) @PathParam("language") String language)
-            throws Exception {
+            throws ApiException {
 
         Map<String, CliOption> opts = Generator.getOptions(language);
 
@@ -136,7 +169,7 @@ public Response getClientOptions(
     public Response getServerOptions(
             @SuppressWarnings("unused") @Context HttpServletRequest request,
             @ApiParam(value = "The target language for the server framework", required = true) @PathParam("framework") String framework)
-            throws Exception {
+            throws ApiException {
 
         Map<String, CliOption> opts = Generator.getOptions(framework);
 
@@ -173,14 +206,13 @@ public Response serverOptions() {
             value = "Generates a server library",
             notes = "Accepts a `GeneratorInput` options map for spec location and generation options.",
             response = ResponseCode.class, tags = "servers")
-    public Response generateServerForLanguage(@Context HttpServletRequest request, @ApiParam(
-            value = "framework", required = true) @PathParam("framework") String framework,
-            @ApiParam(value = "parameters", required = true) GeneratorInput opts) throws Exception {
+    public Response generateServerForLanguage(@Context HttpServletRequest request, @ApiParam(value = "framework", required = true) @PathParam("framework") String framework,
+                                              @ApiParam(value = "parameters", required = true) GeneratorInput opts) throws ApiException {
         if (framework == null) {
             throw new BadRequestException("Framework is required");
         }
         String filename = Generator.generateServer(framework, opts);
-        System.out.println("generated name: " + filename);
+        LOGGER.info("Generated filename: {}", filename);
 
         String host = getHost(request);
 
@@ -190,7 +222,6 @@ public Response generateServerForLanguage(@Context HttpServletRequest request, @
             g.setFilename(filename);
             g.setFriendlyName(framework + "-server");
             fileMap.put(code, g);
-            System.out.println(code + ", " + filename);
             String link = host + "/api/gen/download/" + code;
             return Response.ok().entity(new ResponseCode(code, link)).build();
         } else {

modules/swagger-generator/src/test/java/io/swagger/generator/resource/SwaggerResourceTest.java

@@ -1,21 +1,114 @@
 package io.swagger.generator.resource;
 
+import io.swagger.codegen.utils.SecureFileUtils;
+import io.swagger.generator.model.Generated;
+import org.apache.commons.io.FileUtils;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
 import org.testng.annotations.Test;
 
+import javax.ws.rs.core.Response;
+import java.io.File;
+import java.lang.reflect.Field;
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.testng.Assert.assertFalse;
+import static org.testng.AssertJUnit.assertEquals;
+
 public class SwaggerResourceTest {
 
+    private SwaggerResource resource;
+    private Map<String, Generated> fileMap;
+
+    @BeforeMethod
+    public void setUp() throws Exception {
+        resource = new SwaggerResource();
+
+        fileMap = new HashMap<>();
+        Field fm = SwaggerResource.class.getDeclaredField("fileMap");
+        fm.setAccessible(true);
+        fm.set(null, fileMap);
+    }
+
+    @AfterMethod
+    public void after() {
+        fileMap.clear();
+    }
+
+    @Test
+    public void shouldReturnSuccessWhenDownloadFileAndBadRequestAfterSecondTry() throws Exception {
+        File dir = new File("target/testng-gen1");
+        File zip = new File(dir, "client.zip");
+        FileUtils.write(zip, "TESTDATA", StandardCharsets.UTF_8);
+
+        Generated g = new Generated();
+        g.setFilename(zip.getAbsolutePath());
+        g.setFriendlyName("clientX");
+
+        fileMap.put("123", g);
+
+        Response response = resource.downloadFile("123");
+
+        assertEquals(200, response.getStatus());
+        assertEquals("TESTDATA", new String((byte[]) response.getEntity()));
+        assertFalse(zip.exists(), "File should be removed after download.");
+        assertFalse(dir.exists(), "Directory should be removed after download.");
+
+        Response response2 = resource.downloadFile("123");
+        assertEquals(404, response2.getStatus());
+    }
+
+    @Test
+    public void shouldReturnNotFoundWhenFileDoesntExist() {
+        Response response = resource.downloadFile("nope");
+        assertEquals(404, response.getStatus());
+    }
+
+    @Test(expectedExceptions = Exception.class)
+    public void testDownloadFile_missingPhysicalFile_causes500() {
+        Generated g = new Generated();
+        g.setFilename("target/no_such_dir/file.zip");
+        g.setFriendlyName("missing");
+
+        fileMap.put("777", g);
+
+        resource.downloadFile("777");
+    }
+
+    @Test(expectedExceptions = Exception.class)
+    public void shouldPathValidationFailsWhenDownloadFile() throws Exception {
+        try (MockedStatic<SecureFileUtils> mocked = Mockito.mockStatic(SecureFileUtils.class)) {
+
+            mocked.when(() -> SecureFileUtils.validatePath(Mockito.anyString()))
+                    .thenThrow(new RuntimeException("Invalid path"));
+
+            File dir = new File("target/testng-gen2");
+            File zip = new File(dir, "client.zip");
+            FileUtils.write(zip, "XYZ", StandardCharsets.UTF_8);
+
+            Generated g = new Generated();
+            g.setFilename(zip.getAbsolutePath());
+            g.setFriendlyName("clientY");
+
+            fileMap.put("xyz", g);
+
+            resource.downloadFile("xyz");
+        }
+    }
+
     @Test(expectedExceptions = SecurityException.class)
     public void testDownloadFileWithPathTraversal() throws Exception {
-        SwaggerResource resource = new SwaggerResource();
 
         io.swagger.generator.model.Generated generated = new io.swagger.generator.model.Generated();
         generated.setFilename("../../../etc/passwd");
 
         java.lang.reflect.Field fileMapField = SwaggerResource.class.getDeclaredField("fileMap");
         fileMapField.setAccessible(true);
-        @SuppressWarnings("unchecked")
-        java.util.Map<String, io.swagger.generator.model.Generated> fileMap =
-            (java.util.Map<String, io.swagger.generator.model.Generated>) fileMapField.get(null);
+
         fileMap.put("test-file-id", generated);
 
         try {