apply code review

src/main/java/de/tum/in/www1/artemis/service/FileService.java

@@ -56,8 +56,13 @@ public class FileService implements DisposableBean {
     // NOTE: this list has to be the same as in file-uploader.service.ts
     private final List<String> allowedFileExtensions = new ArrayList<>(Arrays.asList("png", "jpg", "jpeg", "svg", "pdf", "zip"));
 
+    public static final String MARKDOWN_FILE_SUBPATH = "/api/files/markdown/";
+
+    public static final String DEFAULT_FILE_SUBPATH = "/api/files/temp/";
+
     /**
      * Extends the default allowed file extensions with the provided one
+     *
      * @param fileExtension file extension to add
      */
     public void addAllowedFileExtension(String fileExtension) {
@@ -66,6 +71,7 @@ public void addAllowedFileExtension(String fileExtension) {
 
     /**
      * Removes the provided file extension from the allowed file extensions
+     *
      * @param fileExtension file extension to remove
      */
     public void removeFileExtension(String fileExtension) {
@@ -76,16 +82,9 @@ public void removeFileExtension(String fileExtension) {
      * Filenames for which the template filename differs from the filename it should have in the repository.
      */
     // @formatter:off
-    private static final Map<String, String> FILENAME_REPLACEMENTS = Map.ofEntries(
-        Map.entry("git.ignore.file", ".gitignore"),
-        Map.entry("git.attributes.file", ".gitattributes"),
-        Map.entry("Makefile.file", "Makefile"),
-        Map.entry("dune.file", "dune"),
-        Map.entry("Fast.file", "Fastfile"),
-        Map.entry("App.file", "Appfile"),
-        Map.entry("Scan.file", "Scanfile"),
-        Map.entry("gradlew.file", "gradlew")
-    );
+    private static final Map<String, String> FILENAME_REPLACEMENTS = Map.ofEntries(Map.entry("git.ignore.file", ".gitignore"), Map.entry("git.attributes.file", ".gitattributes"),
+        Map.entry("Makefile.file", "Makefile"), Map.entry("dune.file", "dune"), Map.entry("Fast.file", "Fastfile"), Map.entry("App.file", "Appfile"),
+        Map.entry("Scan.file", "Scanfile"), Map.entry("gradlew.file", "gradlew"));
     // @formatter:on
 
     /**
@@ -125,16 +124,14 @@ public void resetOnPath(String path) {
 
     /**
      * Helper method which handles the file creation for both normal file uploads and for markdown
-     * @param file The file to be uploaded
+     *
+     * @param file         The file to be uploaded with a maximum file size set in resources/config/application.yml
      * @param keepFileName specifies if original file name should be kept
-     * @param markdown boolean which is set to true, when we are uploading a file within the markdown editor
+     * @param markdown     boolean which is set to true, when we are uploading a file within the markdown editor
      * @return The path of the file
      */
     @NotNull
     public String handleSaveFile(MultipartFile file, boolean keepFileName, boolean markdown) {
-        // NOTE: Maximum file size is set in resources/config/application.yml
-        // Currently set to 10 MB
-
         // check for file type
         String filename = file.getOriginalFilename();
         if (filename == null) {
@@ -147,52 +144,13 @@ public String handleSaveFile(MultipartFile file, boolean keepFileName, boolean m
             throw new BadRequestException("Unsupported file type! Allowed file types: " + String.join(", ", this.allowedFileExtensions));
         }
 
-        final String filePath;
-        final String fileNameAddition;
-        final StringBuilder responsePath = new StringBuilder();
-
-        // set the appropriate values depending on the use case
-        if (markdown) {
-            filePath = FilePathService.getMarkdownFilePath();
-            fileNameAddition = "Markdown_";
-            responsePath.append("/api/files/markdown/");
-        }
-        else {
-            filePath = FilePathService.getTempFilePath();
-            fileNameAddition = "Temp_";
-            responsePath.append("/api/files/temp/");
-        }
+        final String filePath = markdown ? FilePathService.getMarkdownFilePath() : FilePathService.getTempFilePath();
+        final String fileNameAddition = markdown ? "Markdown_" : "Temp_";
+        final StringBuilder responsePath = new StringBuilder(markdown ? MARKDOWN_FILE_SUBPATH : DEFAULT_FILE_SUBPATH);
 
         try {
-            // create folder if necessary
-            File folder;
-            folder = new File(filePath);
-            if (!folder.exists()) {
-                if (!folder.mkdirs()) {
-                    log.error("Could not create directory: {}", filePath);
-                    throw new InternalServerErrorException("Could not create directory");
-                }
-            }
-
-            // create file (generate new filename, if file already exists)
-            boolean fileCreated;
-            File newFile;
-
-            do {
-                if (!keepFileName) {
-                    filename = fileNameAddition + ZonedDateTime.now().toString().substring(0, 23).replaceAll(":|\\.", "-") + "_" + UUID.randomUUID().toString().substring(0, 8)
-                            + "." + fileExtension;
-                }
-                String path = Path.of(filePath, filename).toString();
-
-                newFile = new File(path);
-                if (keepFileName && newFile.exists()) {
-                    newFile.delete();
-                }
-                fileCreated = newFile.createNewFile();
-            }
-            while (!fileCreated);
-            responsePath.append(filename);
+            File newFile = createNewFile(filePath, fileNameAddition, filename, fileExtension, keepFileName);
+            responsePath.append(newFile.toPath().getFileName());
 
             // copy contents of uploaded file into newly created file
             Files.copy(file.getInputStream(), newFile.toPath(), REPLACE_EXISTING);
@@ -205,6 +163,49 @@ public String handleSaveFile(MultipartFile file, boolean keepFileName, boolean m
         }
     }
 
+    /**
+     * Creates a directory for the given path if it doesn't exist yet.
+     * @param filePath the path of the directory to create
+     */
+    private void createDirectory(String filePath) {
+        File folder = new File(filePath);
+        if (!folder.exists() && !folder.mkdirs()) {
+            log.error("Could not create directory: {}", filePath);
+            throw new InternalServerErrorException("Could not create directory");
+        }
+    }
+
+    /**
+     * Creates a new file from given contents
+     * @param filePath the path to save the file to excluding the filename
+     * @param filename the filename of the file to save
+     * @param fileNameAddition the addition to the filename to make sure it is unique
+     * @param fileExtension the extension of the file to save
+     * @param keepFileName specifies if original file name should be kept
+     * @return the created file
+     */
+    private File createNewFile(String filePath, String filename, String fileNameAddition, String fileExtension, boolean keepFileName) throws IOException {
+        createDirectory(filePath);
+        boolean fileCreated = false;
+        File newFile;
+        do {
+            if (!keepFileName) {
+                filename = fileNameAddition + ZonedDateTime.now().toString().substring(0, 23).replaceAll(":|\\.", "-") + "_" + UUID.randomUUID().toString().substring(0, 8) + "."
+                        + fileExtension;
+            }
+            String path = Path.of(filePath, filename).toString();
+
+            newFile = new File(path);
+            if (keepFileName && newFile.exists()) {
+                Files.delete(newFile.toPath());
+            }
+            fileCreated = newFile.createNewFile();
+        }
+        while (!fileCreated);
+
+        return newFile;
+    }
+
     /**
      * Copies an existing file (if not a temporary file) to a target location. Returns the public path for the resulting file.
      *
@@ -239,7 +240,7 @@ public String copyExistingFileToTarget(String oldFilePath, String targetFolder,
      * @param newFilePath  the new file path (this file will be moved into its proper location, if it was a temporary file)
      * @param targetFolder the folder that a temporary file should be moved to
      * @param entityId     id of the entity this file belongs to (needed to generate
-     *                    public path). If this is null, a placeholder will be inserted where the id would be
+     *                     public path). If this is null, a placeholder will be inserted where the id would be
      * @return the resulting public path (is identical to newFilePath, if file didn't need to be moved)
      */
     public String manageFilesForUpdatedFilePath(String oldFilePath, String newFilePath, String targetFolder, Long entityId) {
@@ -362,7 +363,7 @@ public String publicPathForActualPath(String actualPath, @Nullable Long entityId
 
         // check for known path to convert
         if (actualPath.contains(FilePathService.getTempFilePath())) {
-            return "/api/files/temp/" + filename;
+            return DEFAULT_FILE_SUBPATH + filename;
         }
         if (actualPath.contains(FilePathService.getDragAndDropBackgroundFilePath())) {
             return "/api/files/drag-and-drop/backgrounds/" + id + "/" + filename;
@@ -442,8 +443,8 @@ private File generateTargetFile(String originalFilename, String targetFolder, Bo
         String filename;
         do {
             if (keepFileName) {
-                if (originalFilename.contains("/api/files/temp/")) {
-                    originalFilename = originalFilename.replace("/api/files/temp/", "");
+                if (originalFilename.contains(DEFAULT_FILE_SUBPATH)) {
+                    originalFilename = originalFilename.replace(DEFAULT_FILE_SUBPATH, "");
                 }
                 filename = originalFilename;
             }

src/main/java/de/tum/in/www1/artemis/web/rest/FileResource.java

@@ -39,6 +39,7 @@
 import de.tum.in.www1.artemis.service.ResourceLoaderService;
 import de.tum.in.www1.artemis.web.rest.errors.AccessForbiddenException;
 import de.tum.in.www1.artemis.web.rest.errors.EntityNotFoundException;
+import de.tum.in.www1.artemis.web.rest.lecture.AttachmentUnitResource;
 import io.jsonwebtoken.Claims;
 import io.jsonwebtoken.Jwts;
 
@@ -110,7 +111,8 @@ public FileResource(FileService fileService, ResourceLoaderService resourceLoade
      * @param keepFileName specifies if original file name should be kept
      * @return The path of the file
      * @throws URISyntaxException if response path can't be converted into URI
-     * @deprecated Implement your own usage of {@link FileService#handleSaveFile(MultipartFile, boolean, boolean)} with a mixed multipart request instead.
+     * @deprecated Implement your own usage of {@link FileService#handleSaveFile(MultipartFile, boolean, boolean)} with a mixed multipart request instead. An example for this is
+     * * {@link AttachmentUnitResource#updateAttachmentUnit(Long, Long, AttachmentUnit, Attachment, MultipartFile, boolean, String)}
      */
     @Deprecated
     @PostMapping("fileUpload")

src/test/java/de/tum/in/www1/artemis/service/FileServiceTest.java

@@ -249,7 +249,7 @@ void testActualPathForPublicFileUploadExercisePath_shouldThrowException() {
     void testPublicPathForActualTempFilePath() {
         Path actualPath = Path.of(FilePathService.getTempFilePath(), "test");
         String publicPath = fileService.publicPathForActualPath(actualPath.toString(), 1L);
-        assertThat(publicPath).isEqualTo("/api/files/temp/" + actualPath.getFileName());
+        assertThat(publicPath).isEqualTo(FileService.DEFAULT_FILE_SUBPATH + actualPath.getFileName());
     }
 
     @Test

src/test/java/de/tum/in/www1/artemis/util/ModelFactory.java

@@ -34,6 +34,7 @@
 import de.tum.in.www1.artemis.domain.submissionpolicy.LockRepositoryPolicy;
 import de.tum.in.www1.artemis.security.Role;
 import de.tum.in.www1.artemis.service.FilePathService;
+import de.tum.in.www1.artemis.service.FileService;
 import de.tum.in.www1.artemis.service.connectors.bamboo.dto.BambooBuildLogDTO;
 import de.tum.in.www1.artemis.service.connectors.bamboo.dto.BambooBuildPlanDTO;
 import de.tum.in.www1.artemis.service.connectors.bamboo.dto.BambooBuildResultNotificationDTO;
@@ -84,7 +85,7 @@ public static Attachment generateAttachmentWithFile(ZonedDateTime startDate) {
         catch (IOException ex) {
             fail("Failed while copying test attachment files", ex);
         }
-        attachment.setLink(Path.of("/api/files/temp/", testFileName).toString());
+        attachment.setLink(Path.of(FileService.DEFAULT_FILE_SUBPATH, testFileName).toString());
         return attachment;
     }